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
Merged
GromNaN merged 1 commit into from
Oct 19, 2025
Merged

Add priority field to processor tags #485

GromNaN merged 1 commit into from
Oct 19, 2025

Conversation

@HypeMC
Copy link
Member

@HypeMC HypeMC commented Jul 28, 2024 •
edited
Loading

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.

@HypeMC HypeMC mentioned this pull request Jul 28, 2024
Closed
94noni
94noni reviewed Jul 30, 2024
View reviewed changes
stof
stof reviewed Oct 8, 2024
View reviewed changes
@alexndlm
Copy link

alexndlm commented Feb 17, 2025

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

@derrabus
Copy link
Member

derrabus commented Feb 17, 2025

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

@alexndlm
Copy link

alexndlm commented Feb 17, 2025

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

@derrabus
Copy link
Member

derrabus commented Feb 17, 2025

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

nicolas-grekas commented Apr 8, 2025

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

alexndlm commented Apr 14, 2025

@derrabus I have tested it, and it works perfectly

@alexndlm
Copy link

alexndlm commented Apr 15, 2025

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

alexndlm commented Apr 15, 2025

@HypeMC, do you have time to fix this?

@alexndlm alexndlm mentioned this pull request May 14, 2025
Closed
@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.

@HypeMC HypeMC changed the base branch from 4.x to 3.x September 25, 2025 06:01
@HypeMC HypeMC force-pushed the add-priority branch 2 times, most recently from 514aa72 to 62fae47 Compare October 2, 2025 22:05
GromNaN
GromNaN approved these changes Oct 4, 2025
View reviewed changes
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.

@GromNaN
Copy link
Member

GromNaN commented Oct 19, 2025

Thank you @HypeMC.

@GromNaN GromNaN merged commit c6b64c3 into symfony:3.x Oct 19, 2025
8 checks passed
@HypeMC HypeMC deleted the add-priority branch October 19, 2025 18:24
@jmikola jmikola mentioned this pull request Oct 23, 2025
Merged
This was referenced Dec 7, 2025
Closed
Merged
nicolas-grekas added a commit to symfony/symfony that referenced this pull request Dec 8, 2025
@nicolas-grekas
bug #62685 [DependencyInjection] Fix PriorityTaggedServiceTrait whe…
16389f6 
…n tag attributes are not a list (GromNaN)

This PR was merged into the 7.3 branch.

Discussion
----------

[DependencyInjection] Fix `PriorityTaggedServiceTrait` when tag attributes are not a list

| Q             | A
| ------------- | ---
| Branch?       | 7.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | -
| License       | MIT

Alternative to #62683

Ensure the attributes of the tag are in a list before iterating on them using sequential indexes.

> In symfony/monolog-bundle#485, the tag attributes are filtered, which generates gaps in the keys. The PR mentioned above causes MonologBundle to fail. This can be fixed by symfony/monolog-bundle#563. However I believe the issue should also be addressed in Symfony.

Commits
-------

5a4a036 [DependencyInjection] Fix PriorityTaggedServiceTrait when tag attributes are not a list
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

@GromNaN GromNaN GromNaN approved these changes

+2 more reviewers

@alexndlm alexndlm alexndlm requested changes

@94noni 94noni 94noni approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@HypeMC @alexndlm @derrabus @nicolas-grekas @stof @GromNaN @94noni @OskarStark