Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix nested Log::withContext #37860

Closed
wants to merge 3 commits into from
Closed

Conversation

cykirsch
Copy link

This builds on @chasenyc's recent log context PR: github.com//pull/37847

We have a custom context service to handle this in our own application, and I was glad to see it added to the framework. But I have a couple changes to propose based on our experience.

First, log context doesn't have to be flat. Say you want to have a couple values nested under user and then only change one. The following is how that would play out with array_merge(), array_merge_recursive(), and array_replace_recursive().

// setup -- assume each example starts clean and then runs this first
Log::withContext(['user' => ['id' => 1, 'name' => 'me']]); // ['user' => ['id' => 1, 'name' => 'me']]


// with array_merge it loses the unchanged key
Log::withContext(['user' => ['id' => 2]]); // ['user' => ['id' => 2]]

// with array_merge_recursive it keeps the unchanged key but combines the changed one into an array
Log::withContext(['user' => ['id' => 2]]); // ['user' => ['id' => [1, 2], 'name' => 'me']]

// with array_replace_recursive it replaces only the item specified
Log::withContext(['user' => ['id' => 2]]); // ['user' => ['id' => 2, 'name' => 'me']]

So array_replace_recursive() is what we landed on, and is what I have included in this PR. The TL;DR on this is that you can now change an individual nested key in the log context.

Second, I added a getContext() to get what is currently in the Log context. If you pass in some local overriding context to this method it will not apply to the future logs, only the current method response. This is useful if you want to use your Log context for another purpose. For example, you could pass this context to a feature flag provider.

@taylorotwell
Copy link
Member

This would now be considered a breaking change. The feature has already been released.

@cykirsch cykirsch changed the title Context replace Fix nested Log::withContext Jun 29, 2021
@chasenyc
Copy link
Contributor

@cykirsch well, getContext wouldn't be a breaking change I don't think, then at least within your code you can properly change the context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants