Skip to content

Conversation

HypeMC
Copy link
Member

@HypeMC HypeMC commented Jul 28, 2024

Q A
Branch? 3.x
Bug fix? no
New feature? yes
Deprecations? no
Issues -
License MIT

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 {}

Probably better to review 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.

@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
Copy link

@HypeMC, do you have time to fix this?

@HypeMC
Copy link
Member Author

HypeMC commented Aug 22, 2025

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 Sorry for the late reply, I missed your feedback. The issue should be fixed with 1ad6fc7.

Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

LGTM, with some improvement that can be done later.

} else {
if ($this->channelPass) {
$definitions = [];
foreach ($this->channelPass->getChannels() as $channel) {
Copy link
Member

Choose a reason for hiding this comment

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

The compiler passes should be stateless. We can add a tag to the logger definitions so that we can retrieve them with $container->findTaggedServiceIds('monolog.channel_logger')

Comment on lines +82 to +83
} else {
if ($this->channelPass) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: you can combine else and if into elseif

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

Successfully merging this pull request may close these issues.

8 participants