-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DependencyInjection] Sort the CompilerPass by priority #18022
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
Conversation
What about instead of removing the type support adding support for adding a priority within a type? In my opinion, being able to choose the compiler pass type makes it much easier to determine when a compiler pass will be executed than it would be when there were only priorities (and some things even could not fully be accomplished when using only priorities). |
Well that's still possible @xabbuh but only through the class constants. |
But technically I would now only be able to add 100 compiler passes per type (after that priorities cannot be distinguished). |
Well, this PR does not bring a solution... it just basically rename the key. As @xabbuh has proposed, I think it would be better to have a notion a priority inside each types, which would be sorted out before the big merge. |
Well you could add more than 100 passes per type (a type corresponds to a priority which is not incremented) the only restriction is that you have only 99 differents priorities available between each type for doing whatever you want but that's the same thing for event listeners. |
I think this idea has been proposed in the past and rejected. Basically, I don't like to give too much flexibility here. People should not rely on precise ordering within one pass. Adding new passes where it makes sense seems better and could be considered if we have enough real-world use cases. |
@fabpot I've searched PRs solving the same issue but I only found #10778 which propose a different approach which doesn't solve all issues. Even the core depends on the compiler passes ordering : would symfony still work if we shuffled this array ? |
I also dig into this issue in #13609 for the exact same use case @Ener-Getick describes. |
Did you take a decision about this ? |
BTW the build fail is unrelated. |
I've been thinking how to improve this PR and I finally agree with @xabbuh and @Taluu that this could be confusing to replace the types by a priority system. @fabpot are you still against this change? public function build(ContainerBuilder $container) {
$passConfig = $container->getCompilerPassConfig();
$passes = $passConfig->getBeforeOptimizationPasses();
// This allows to be certain that your pass will be executed before a pass
// located in a third party bundle
array_unshift($passes, new MyPass());
$passConfig->setBeforeOptimizationPasses($passes);
} This could be simplified with this PR to: public function build(ContainerBuilder $container) {
$container->addCompilerPass(new MyPass(), PassConfig::TYPE_BEFORE_OPTIMIZATION, 50);
} |
I'd love to see this merged. |
Adding a priority like done here looks good to me. |
@fabpot I'll add some tests then :) BTW maybe we will have to deprecate the |
I would indeed deprecate the set* methods. |
@fabpot it looks like these methods are often used in tests to remove the default passes and have a non altered/fast container. What do you think about moving them to another class such as the |
I think if we really want to deprecate the |
3.2.0 | ||
----- | ||
|
||
* allowed to prioritize compiler passes by introducing a third argument to `PassConfig::addPass()`, to `Compiler::addPass` and to `ContainerBuilder::addCompilerPass()` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing () on addPass
Thank you @Ener-Getick. |
…pilerPass (nicolas-grekas) This PR was merged into the 3.2-dev branch. Discussion ---------- [DI] Add missing deprecation in ContainerBuilder::addCompilerPass | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #18022 | License | MIT | Doc PR | - ping @Ener-Getick Commits ------- c0e880e [DI] Add missing deprecation in ContainerBuilder::addCompilerPass
Great feature. Just needed it :) Thank you @GuilhemN |
This PR replaces the CompilerPass types by a new priority argument.
Sometimes we want to be sure that a CompilerPass will be executed after another but we can't do that because we don't know when the other pass will be added.
This PR fixes this by allowing people to simply choose when their compiler passes will be executed.
Things to debate: