Skip to content

Add priority field to processor tags #485

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add priority field to processor tags #485

wants to merge 5 commits into from

Conversation

HypeMC
Copy link
Contributor

@HypeMC HypeMC commented Jul 28, 2024

Continuation of #455.

Foo\Bar\Log\Processors\FirstProcessor:
    tags:
        - { name: monolog.processor, channel: http }

Foo\Bar\Log\Processors\SecondProcessor:
    tags:
        - { name: monolog.processor, channel: http, priority: -10 } # last

Foo\Bar\Log\Processors\ThirdProcessor:
    tags:
        - { name: monolog.processor, channel: http, priority: -20 } # very last
#[AsMonologProcessor(priority: -10)]
class SecondProcessor {}

#[AsMonologProcessor(priority: -20)]
class ThirdProcessor {}

@HypeMC HypeMC mentioned this pull request Jul 28, 2024
Closed
94noni
94noni reviewed Jul 30, 2024
View reviewed changes
DependencyInjection/Compiler/AddProcessorsPass.php Outdated
public function process(ContainerBuilder $container)
{
if (!$container->hasDefinition('monolog.logger')) {
return;
}

foreach ($container->findTaggedServiceIds('monolog.processor') as $id => $tags) {
foreach (array_reverse($this->findAndSortTaggedServices('monolog.processor', $container)) as $reference) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps comment why array_reverse is used ?
otherwise its smart :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, done

Roy and others added 3 commits August 6, 2024 12:06
@HypeMC
Add priority field to processor tags
7bd4ddc 
So that the order in which the processors are used by monolog can be
determined.
@HypeMC
spaceship!
aebea64
@HypeMC
Use PriorityTaggedServiceTrait
b95c929
stof
stof reviewed Oct 8, 2024
View reviewed changes
DependencyInjection/Compiler/AddProcessorsPass.php Outdated
public function process(ContainerBuilder $container)
{
if (!$container->hasDefinition('monolog.logger')) {
return;
}

foreach ($container->findTaggedServiceIds('monolog.processor') as $id => $tags) {
// array_reverse is used because ProcessableHandlerTrait::pushProcessor prepends processors to the beginning of the stack
foreach (array_reverse($this->findAndSortTaggedServices('monolog.processor', $container)) as $reference) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does this behave when we have multiple tags (for instance when registering the processor in specific channels or in specific handlers ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stof Good catch, it didn't work. I've managed to fix it. Please let me know what you think of the fix. It's probably easiest to review it with whitespace changes ignored.

@alexndlm
Copy link

@stof @derrabus Can we have this merged and released? This is an extremely needed feature.

@derrabus
Copy link
Member

@alexndlm Did you review and test the change in your projects? I agree that we should try to get it merged.

@alexndlm
Copy link

Of course not, I have unlimited trust in Symfony 😎 But I will do this soon to double-check.

@derrabus
Copy link
Member

But I will do this soon to double-check.

Thank you! I personally don't need this feature at the moment. So getting feedback from someone who'd actually use it is more than valuable to us.

@jon-ht jon-ht mentioned this pull request Apr 8, 2025
Open
2 tasks
@nicolas-grekas
Copy link
Member

Looks like PR is needed for symfony/symfony#54445?

@stof
Copy link
Member

stof commented Apr 8, 2025

@nicolas-grekas no. this PR does not affect the Symfony PR (see my comment on the Symfony PR)

@alexndlm
Copy link

@derrabus I have tested it, and it works perfectly

@alexndlm
Copy link

@derrabus, my bad, it does not work; I have found one issue.

When we add some processor only for channel app with the lowest priority, it will always be first. This is because in \Symfony\Bundle\MonologBundle\DependencyInjection\Compiler\AddProcessorsPass for the app channel, we use monolog.logger, and when there is no channel, we use monolog.logger_prototype.

And in symfony/monolog-bundle/Resources/config/monolog.xml the monolog.logger has parent monolog.logger_prototype.

So, when DI create monolog.logger service, it first calls push pushProcessor for monolog.logger_prototype and then calls for monolog.logger.

The same issue should be for any channel.

alexndlm
alexndlm suggested changes Apr 15, 2025
View reviewed changes
Tests/DependencyInjection/Compiler/AddProcessorsPassTest.php
$this->assertEquals(['pushProcessor', [new Reference('test2')]], $calls[0]);
$this->assertEquals(['pushProcessor', [new TypedReference('test2', 'TestClass')]], $calls[0]);

$service = $container->getDefinition('monolog.handler.priority_test');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a case when the processor has a channel?

@alexndlm
Copy link

@HypeMC, do you have time to fix this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@alexndlm alexndlm alexndlm requested changes

@94noni 94noni 94noni approved these changes

Assignees
No one assigned
Labels
Feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@HypeMC @alexndlm @derrabus @nicolas-grekas @stof @94noni @OskarStark