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

[9.x] Names for on-demand log channels #39463

Closed
wants to merge 1 commit into from

Conversation

Air-Petr
Copy link

@Air-Petr Air-Petr commented Nov 3, 2021

This PR is connected with "On-Demand Channels" (https://laravel.com/docs/8.x/logging#on-demand-channels).
"On-Demand Channels" have been introduced by #39273

Problem

Every call of a Log::build(...) uses the same log channel. Example (can be executed in tinker):

use Illuminate\Support\Facades\Log;

// ... some code

Log::build([
  'driver' => 'single',
  'path' => storage_path('foo/custom.log'),
])->info('foo');

// Now we have a "foo" record in a "foo" directory

// ... some code

Log::build([
  'driver' => 'single',
  'path' => storage_path('bar/custom.log'),
])->info('bar');

// New "bar" record, but still in a "foo" directory

Case

We have a queue running. Some jobs need certain channels (based on the names from a database). We've tried to use "On-Demand Channels". And now all information goes to one channel (the one is used by a first job appeared in a queue).

Possible solution

We can extend a "build" method by adding second optional parameter - the name of a channel.
For me it's hard to say is it good or bad from architectural perspective. But it certainly can be useful in some situations.
I've prepared a code change and a test method.
I've run tests by calling:

./vendor/bin/phpunit --filter=LogManagerTest

@taylorotwell
Copy link
Member

Eh, probably this should be fixed by simply making the manager not cache on-demand instances. I think that was the intended goal but was probably overlooked in the original PR.

@taylorotwell
Copy link
Member

It's likely this problem exists for on-demand disks as well.

@taylorotwell
Copy link
Member

Fixed by bc50a9b

@Air-Petr
Copy link
Author

Air-Petr commented Nov 4, 2021

Thank you.

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.

2 participants