[12.x] Add Context::scope()

[cosmastech]

First time using Context outside of request level values, and I have a particularly hairy function that can throw lots of exceptions and calls lots of dependencies. Just for the duration of this action I need logs to contain a particular id, and then it can restore the context once the action is completed.

An example similar to what is in our app code

class PublishSnsNotificationAction
{
    public function __construct(private SnsClient $snsClient) {}

    public function handle($event)
    {
        Context::add('event_id', $event->id);

        try {
            $this->innerHandle($event);
        } finally {
            Context::forget(['event_id', 'sns_message_id']);
        }
    }

    private function innerHandle($event)
    {
        $payload = $this->convertToSnsPayload($event);
        \Log::debug("Created payload", ['payload' => $payload]);

        $result = $this->snsClient->publish($payload);

        \Log::debug("Message ID received: " . $result->get('MessageId'));
        Context::add('sns_message_id', $result->get('MessageId'));

        $this->updateEvent($event, $result);

        \Log::debug("Event updated in DB.");
    }
}

Which could become something like this instead:

public function handle($event)
{
    Context::scope(
        fn () => $this->innerHandle($event),
        ['event_id' => $event->id]
    );
}

Not sure how much other people would use this, or how much our team would even use it outside of this particular use case.

It dawns on me as I'm typing this up I have the ability to macro it in, which will be a workable solution if this isn't accepted.

Documentation

I'm happy to take a run at updating the docs for this, if it is accepted.

[github-actions]

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

[timacdonald]

Although discussed at the time, I dropped this feature from the original PR because I ran into blockers that I didn't have time to deep dive on.

What happens, as an example, if someone mutates an object in context during the with call?

Then context would not really be restored to their original state.

Might need some more thought on how we could handle this or if we even need to?

I'm about to head off. I can circle back tomorrow with some examples to illustrate the issue further.

[timacdonald]

I'd like to make this feature happen, btw. I'm into it.

[cosmastech]

Although discussed at the time, I dropped this feature from the original PR because I ran into blockers that I didn't have time to deep dive on.

💪 Nice. Glad to know the idea wasn't dropped because it was regarded as a patently bad idea 😆

What happens, as an example, if someone mutates an object in context during the with call?

Then context would not really be restored to their original state.

Might need some more thought on how we could handle this or if we even need to?

In my opinion, this probably isn't preferable. I think reverting objects back to their original state would cause more confusion and be a significantly larger computational overhead.

My first thought would be to serialize/unserialize non-scalars. How would we handle a Model in the context data?

If we leveraged SerializesModels, that would mean extra database queries to reload that model and its relationships. That also wouldn't work if the Model was dirty, since unpersisted changes aren't stored in the serialization. So I think we would have to serialize each of them individually.

But I think for most developers who store Models in Context, they probably want to have the Model state match the DB. If the Model is modified inside the callback, I personally can't picture a time where I would want the original version of the model back.

Context::addHidden('user', $user);
Context::with(data: ['email' => $user->email], callback: function () use ($user) {
    Log::debug("Deleting user.");
    $user->notify(new AccountDeleted());
    $user->delete();
});
// Should $user->exists be true or false?

A new app my team is building has a RequestContext object that is bound as a singleton and synchronizes with the Context Repository. It contains a handful of properties, most of which are Models.

Here's an abbreviated example.

class RequestContext
{
    private AppModel $callingApp; // the names here are confusing, but these are both DB models
    private RequestModel $request;
    
    public function __set($name, $value)
    {
        $this->{$name} = $value;

        Context::add(['request' => $this->request?->id, 'app' => $this->callingApp->name]);
        Context::addHidden(['requestContext' => $this]);
    }
}

If Context::with() restored the exact state of the Context to its original state, this would make it unusable for us.

There's also the question of what it would look like, if inside the with() callback, an object from Context was referenced to build a Closure or set as a property on another object. About to start my workday, but I think we could probably cook up a toy example that would show how frustrating that could be.

---

@timacdonald let me know your thoughts on how we can improve this. As always, appreciate your insight and collaboration!

[rodrigopedra]

What about just auto-forgetting the temporary keys?

public function with(callable $callback, array $data = [], array $hidden = [])
{
    if ($data !== []) {
        $this->add($data);
    }

    if ($hidden !== []) {
        $this->addHidden($hidden);
    }

    try {
        return $callback();
    } finally {
        $this->forget(array_keys($data));
        $this->forgetHidden(array_keys($hidden));
    }
}

Any other keys present before would work just fine, even allowing one to replace/forget them.

The method would only allow for some temporary keys to be present during the callback.

[cosmastech]

What about just auto-forgetting the temporary keys?

public function with(callable $callback, array $data = [], array $hidden = [])
{
    if ($data !== []) {
        $this->add($data);
    }

    if ($hidden !== []) {
        $this->addHidden($hidden);
    }

    try {
        return $callback();
    } finally {
        $this->forget(array_keys($data));
        $this->forgetHidden(array_keys($hidden));
    }
}

Any other keys present before would work just fine, even allowing one to replace/forget them.

The method would only allow for some temporary keys to be present during the callback.

That was the original approach I took. But I considered an example like this.

Context::add('value', 123);

Context::with(function() {
    // do something
}, data: ['value' => 127]);

Context::missing('value'); // is true

[rodrigopedra]

@cosmastech I just figured that out and was editing my previous comment, haha =)

We could:

• warn users on docs
• keep/restore only the original intersecting keys' values, and forget the ones not present

[cosmastech]

• keep/restore only the original intersecting keys' values, and forget the ones not present

Would that be better than just resetting to the original value? I'm not sure I follow why we would do that.

I think a docs warning would be necessary with whichever approach we take. 😅

[rodrigopedra]

@cosmastech, how about this:

Artisan::command('local:test', function (\Illuminate\Events\Dispatcher $events) {
    $context = new class($events) extends \Illuminate\Log\Context\Repository
    {
        public function with(callable $callback, array $data = [], array $hidden = [])
        {
            $dataToRestore = array_intersect_key($this->data, $data);
            $hiddenToRestore = array_intersect_key($this->hidden, $hidden);

            $this->add($data);
            $this->addHidden($hidden);

            try {
                return $callback();
            } finally {
                $this->forget(array_keys($data));
                $this->forgetHidden(array_keys($hidden));

                $this->add($dataToRestore);
                $this->addHidden($hiddenToRestore);
            }
        }
    };

    $context->add('one', 'one');
    $context->add('two', 'two');

    dump(['before' => $context->all()]);

    $context->with(function () use ($context) {
        dump(['within' => $context->all()]);
    }, ['one' => 'uno', 'two' => 'dos', 'three' => 'tres']);

    dump(['after' => $context->all()]);
});

Output:

$ php artisan local:test
array:1 [
  "before" => array:2 [
    "one" => "one"
    "two" => "two"
  ]
] // routes/local.php:218
array:1 [
  "within" => array:3 [
    "one" => "uno"
    "two" => "dos"
    "three" => "tres"
  ]
] // routes/local.php:221
array:1 [
  "after" => array:2 [
    "one" => "one"
    "two" => "two"
  ]
] // routes/local.php:224

[cosmastech]

@cosmastech, how about this:

Does this cover a use case that the current implementation doesn't? I'm not at my personal laptop to check, but I'm trying to see edge case this covers?

[rodrigopedra]

Would that be better than just resetting to the original value? I'm not sure I follow why we would do that.

That would be to avoid copying large data. PHP assigns arrays by copy, for example.

Also, if a user mutates, or adds something into the context within the callback, they would keep that:

$context->with(function () use ($context) {
    dump(['within:' => $context->all()]);

    $context->add('four', 'four'); // this is kept after the callback
}, ['one' => 'uno', 'two' => 'dos', 'three' => 'tres']);

[rodrigopedra]

Does this cover a use case that the current implementation doesn't?

See my previous comment, anything added into context within the callback would be discarded.

[rodrigopedra]

Ok, after reading with more attention, I think using a decorator object, wrapping the original context, and swapping the decorator into the façade, could be a better solution:

• the decorator would forward all mutations to the original context
• temporary data is discarded, unless mutated within callback
• object references are unchanged, such as $user->exists

Code:

<?php // ./routes/console.php

use App\Models\User;
use Illuminate\Log\Context\Repository;
use Illuminate\Support\Facades\Artisan;
use Illuminate\Support\Facades\Context;

// Decorator class
class TemporaryContext extends Repository
{
    protected Repository $original;

    public function __construct(Repository $context, array $data = [], array $hidden = [])
    {
        parent::__construct($context->events);

        $this->original = $context;

        $this->data = array_merge($context->data, $data);
        $this->hidden = array_merge($context->hidden, $hidden);
    }

    public function add($key, $value = null)
    {
        $this->original->add($key, $value);

        return parent::add($key, $value);
    }

    public function addHidden($key, #[\SensitiveParameter] $value = null)
    {
        $this->original->addHidden($key, $value);

        return parent::add($key, $value);
    }

    public function forget($key)
    {
        $this->original->forget($key);

        return parent::forget($key);
    }

    public function forgetHidden($key)
    {
        $this->original->forgetHidden($key);

        return parent::forgetHidden($key);
    }

    public function push($key, ...$values)
    {
        $this->original->push($key, ...$values);

        return parent::push($key, ...$values);
    }

    public function pop($key)
    {
        $this->original->pop($key);

        return parent::pop($key);
    }

    public function pushHidden($key, ...$values)
    {
        $this->original->pushHidden($key, ...$values);

        return parent::pushHidden($key, ...$values);
    }

    public function popHidden($key)
    {
        $this->original->popHidden($key);

        return parent::popHidden($key);
    }

    public function flush()
    {
        $this->original->flush();

        return parent::flush();
    }
}

Context::macro('with', function (callable $callback, array $data = [], array $hidden = []) {
    // replace façade instance by temporary context decorator
    Context::swap(new TemporaryContext($this, $data, $hidden));

    try {
        return $callback();
    } finally {
        // swap back façade instance to the original one
        Context::swap($this);
    }
});

Artisan::command('app:test', function () {
    Context::add('one', 'one');

    dump(['before' => Context::all()]);

    Context::with(function () {
        Context::add('three', 'tres');

        dump(['within' => Context::all()]);

        Context::add('two', 'two');
        Context::add('three', 'three');
        Context::add('four', 'four');
    }, ['one' => 'uno', 'two' => 'dos']);

    dump(['after' => Context::all()]);

    Context::flush();

    $user = User::factory()->create();

    Context::addHidden('user', $user);

    dump([
        'before' => Context::all(), // no data
        'user exists?' => Context::getHidden('user')->exists,
    ]);

    Context::with(function () {
        $user = Context::getHidden('user');
        $user->delete();

        // mutating within reflects outside
        Context::add('name', 'Unknown');

        dump([
            'within' => Context::all(),
            'user exists?' => Context::getHidden('user')->exists,
        ]);
    }, ['email' => $user->email, 'name' => $user->name]);

    dump([
        'after' => Context::all(),
        'user exists?' => Context::getHidden('user')->exists,
    ]);
});

Output:

$ php artisan app:test
array:1 [
  "before" => array:1 [
    "one" => "one"
  ]
] // routes/console.php:102
array:1 [
  "within" => array:3 [
    "one" => "uno"
    "two" => "dos"
    "three" => "tres"
  ]
] // routes/console.php:107
array:1 [
  "after" => array:4 [
    "one" => "one"
    "three" => "three"
    "two" => "two"
    "four" => "four"
  ]
] // routes/console.php:114
array:2 [
  "before" => []
  "user exists?" => true
] // routes/console.php:122
array:2 [
  "within" => array:2 [
    "email" => "bo.walsh@example.net"
    "name" => "Unknown"
  ]
  "user exists?" => false
] // routes/console.php:134
array:2 [
  "after" => array:1 [
    "name" => "Unknown"
  ]
  "user exists?" => false
] // routes/console.php:140

[timacdonald]

I like the minimalist implementation that we currently have.

I'd like to see this function renamed to scope. That allows it to read nicely well when the data is not known at call time and is therefore not able to be passed in.

Context::has('foo'); // false

Context::scope(function () {
    Context::has('foo'); // true

    // Do some stuff...

    Context::add('foo', 'bar');

    // Do some stuff...

    Context::has('foo'); // true
});

Context::has('foo'); // false

Full usage API:

Context::scope(function () {
    // ...
});

Context::scope(function () {
    // ...
}, [/* ... */]);

Context::scope(function () {
    // ...
}, hidden: [/* ... */]);

Context::scope(function () {
    // ...
}, data: [/* ... */], hidden: [/* ... */]);

My main concern here is the following mutation scenario where an object already in Context is mutated and the mutation persists outside of the Closure. The more I think about it, though, the more it is likely just something we have to accept. This isn't really about locking down the values in a deeply nested way. People can use immutable objects if they want to protect against this kind of thing.

Context::add('user', literal(
    name: 'Tim',
));

Context::get('user')->name; // Tim

Context::scope(function () {
    $user = Context::get('user');

    $user->name = 'Taylor';
});

Context::get('user')->name; // Taylor

I don't want to go down the path of serializing or anything. That is nasty as. Think I just needed to step through that out loud.

Thoughts?

[cosmastech]

I like the minimalist implementation that we currently
Thoughts?

I think I agree with you on all the points. Just so I am clear, the only suggested change is to rename the method?

[timacdonald]

the only suggested change is to rename the method

Yea.

[cosmastech]

Yea.

Great! Thanks @timacdonald. This has been updated.

@taylorotwell let me know if you want me to take a run at documentation for this (if merged).

[taylorotwell]

@cosmastech Thanks! If you want to give the docs a shot I'll give it a review! 🫡