Don't undo what you haven't done

Turn something off, do a thing, turn it back on. Seems so simple! But... you might be undoing something that you never did in the first place. You might be committing a catastrophic error.

(Prefer to watch instead of read? Check out the video version of this article on YouTube: Poppin Off: Don't undo what you haven't done)

A few weeks after publishing this, Nuno Maduro ran across this exact bug, and it took his site down. Check out his tweet here.

The problem

Let’s jump into the code. We’re working with a Laravel command where we have two actions: superSecret and notSecret. The goal is to log a message in superSecret that shouldn’t be visible and another in notSecret that’s okay to show.

{
    protected $signature = 'app:run';
 
    protected bool $loggingEnabled = true;
 
    public function handle(): void
    {
        $this->superSecret();
 
        $this->notSecret();
    }
 
    public function superSecret(): void
    {
        $this->log('Uh oh, you shouldn\'t see this');
    }
 
    public function notSecret(): void
    {
        $this->log('It\'s ok to see this.');
    }
 
    public function log($msg)
    {
        if ($this->loggingEnabled) {
            $this->line($msg);
        }
    }
}

If we run this, we’ll see both messages:

Uh oh, you shouldn't see this!
It's ok to see this.

Let's first work on hiding the super secret message. We can do this by setting a flag to disable logging before calling the super secret method and then re-enabling it after.

{
    protected $signature = 'app:run';
 
    protected bool $loggingEnabled = true;
 
    public function handle(): void
    {
        $this->loggingEnabled = false; 
        $this->superSecret();
        $this->loggingEnabled = true; 
 
        $this->notSecret();
    }
 
}

So if we run this now, we’ll see only the not secret message.

This solution isn't good enough. We don't want to be doing turning things off and on like this. It's error-prone and can lead to some pretty nasty bugs.

Often you'll see folks do this within a dedicated callback:

{
    public function handle(): void
    {
        $this->withoutLogging(function () { 
            $this->superSecret();
        });
 
        $this->notSecret();
    }
 
    public function withoutLogging($cb)
    {
        // Turn logging off
        $this->loggingEnabled = false;
 
        try {
            // Call the user function
            $cb();
        } finally {
            // Turn logging back on
            $this->loggingEnabled = true;
        }
    }
}

This is nice because it's wrapped up into a clean callback, and if there's an error we turn logging back on and then we throw the exception. So now if we run this, we'll see only the not secret message.

However, we just made a tragic mistake! Imagine we've turned off logging in some other class, or we've turned logging off twice:

{
    public function handle(): void
    {
        // Turn it off
        $this->withoutLogging(function () {
            // Turn it off again!
            $this->withoutLogging(function () {
                $this->superSecret(); // This stays hidden 
            });
 
            $this->superSecret(); // This gets revealed! 
 
        });
 
        $this->notSecret();
    }
}

Now we're in a situation where we've turned off logging twice but then turned it back on after we exit the first callback.

So we're going to leak the super secret message.

We've undone something that we never did in the first place.

The first time we enter this closure we set loggingEnabled to false:

{
    public function handle(): void
    {
        $this->withoutLogging(function () { 
            $this->withoutLogging(function () {
                $this->superSecret();
            });
 
            $this->superSecret();
 
        });
 
        $this->notSecret();
    }
}

Then immediately after that we set loggingEnabled to false again:

{
    public function handle(): void
    {
        $this->withoutLogging(function () {
            $this->withoutLogging(function () { 
                $this->superSecret();
            });
 
            $this->superSecret();
 
        });
 
        $this->notSecret();
    }
}

Then we exit the closure and we set loggingEnabled to true:

{
    public function handle(): void
    {
        $this->withoutLogging(function () {
            $this->withoutLogging(function () {
                $this->superSecret();
            }); // loggingEnabled set back to true 
 
            $this->superSecret();
 
        });
 
        $this->notSecret();
    }
}

However, we shouldn't really reset it until we exit the second closure.

{
    public function handle(): void
    {
        $this->withoutLogging(function () {
            $this->withoutLogging(function () {
                $this->superSecret();
            });
 
            $this->superSecret();
 
        }); // where we should ultimately set loggingEnabled back to true 
 
        $this->notSecret();
    }
}

The solution

The way around this is to cache the state of the logging at the beginning of the withoutLogging function, explicitly turn logging off, perform the action, and then set it back to the cached state instead of explicitly turning it on.

public function withoutLogging($cb)
{
    $loggingEnabled = $this->loggingEnabled; 
 
    $this->loggingEnabled = false;
 
    try {
        // Make sure to return the callback in
        // case anyone else is relying on it.
        return $cb();
    } finally {
        // Restore it to the previous state. Not explicitly `true`.
        $this->loggingEnabled = $loggingEnabled; 
    }
}

Now if we run this, we'll see only the notSecret message. We've fixed our problem!

The pattern in Laravel

This is a pattern that you'll see in many frameworks. For example, in Laravel, you'll see this in the withoutBroadcasting method and withoutEvents method.

public static function withoutBroadcasting(callable $callback)
{
    $isBroadcasting = static::$isBroadcasting;
 
    static::$isBroadcasting = false;
 
    try {
        return $callback();
    } finally {
        static::$isBroadcasting = $isBroadcasting;
    }
}

It's a good indication that if a big framework like Laravel is doing this, it's probably a good idea to do it in your own code as well.

An alternative: returning an undo function

If you don't like the nesting of the closures, you can return an undo function. (I covered this pattern in Clean up after yourself.)

Here we'll update our code to save the result of the withoutLogging function to an $undo variable. After we handle the superSecret methods, we'll call $undo to put it back to whatever it was originally.

{
    public function handle(): void
    {
        $undo = $this->withoutLogging(); 
 
        $this->superSecret();
        $this->superSecret();
 
        $undo(); 
 
        $this->notSecret();
    }
}

We'll need to make a few updates to our withoutLogging function. First, we'll default the callback to null, so we can call it without passing anything in. Then we save the current state of loggingEnabled to a variable.

Lastly, we'll return the undo function at the end of the method. The undo function will set loggingEnabled back to whatever it was originally.

public function withoutLogging($cb = null) 
{
    $loggingEnabled = $this->loggingEnabled;
    $this->loggingEnabled = false;
 
    $undo = function () use ($loggingEnabled) { 
        $this->loggingEnabled = $loggingEnabled;
    };
 
    if (is_null($cb)) {
        return $undo;
    }
 
    try {
        return $cb();
    } finally {
        $undo();
    }
}

Now if we have some other distant class where we're doing and undoing something else, like logging, we will still respect who has done what and not undo someone else's action.

Wrapping Up

Don't undo what you haven't done. A simple but crucial pattern. Respect the state of the world when you enter a function, do your thing, and then return it back to the state it was in when you entered.

Youtube video