Skip to content

[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

Closed
wants to merge 3 commits into from

Conversation

GuilhemN
Copy link
Contributor

@GuilhemN GuilhemN commented Mar 5, 2016

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #10778
License MIT
Doc PR

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:

  • the constants value
  • should we create a new function to get/set passes for a specific priority ?

@xabbuh
Copy link
Member

xabbuh commented Mar 7, 2016

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).

@GuilhemN
Copy link
Contributor Author

GuilhemN commented Mar 7, 2016

Well that's still possible @xabbuh but only through the class constants.

@xabbuh
Copy link
Member

xabbuh commented Mar 7, 2016

But technically I would now only be able to add 100 compiler passes per type (after that priorities cannot be distinguished).

@Taluu
Copy link
Contributor

Taluu commented Mar 7, 2016

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.

@GuilhemN
Copy link
Contributor Author

GuilhemN commented Mar 7, 2016

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.
And your solution @Taluu won't respect bc.

@fabpot
Copy link
Member

fabpot commented Mar 7, 2016

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.

@GuilhemN
Copy link
Contributor Author

GuilhemN commented Mar 7, 2016

@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.
A typical use case is when a bundle uses a compiler pass to change the attributes of a definition. If another bundle needs to access this definition attributes before the container optimization, it isn't able to be sure which version of the definition it has.

Even the core depends on the compiler passes ordering : would symfony still work if we shuffled this array ?

@egeloen
Copy link

egeloen commented Mar 7, 2016

I also dig into this issue in #13609 for the exact same use case @Ener-Getick describes.

@GuilhemN
Copy link
Contributor Author

Did you take a decision about this ?

@GuilhemN
Copy link
Contributor Author

BTW the build fail is unrelated.

@GuilhemN
Copy link
Contributor Author

GuilhemN commented Apr 1, 2016

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.
So instead I added a new $priority attribute which is completely BC as they proposed.

@fabpot are you still against this change?
As an example, I often see this kind of code:

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);
}

@GuilhemN
Copy link
Contributor Author

I'd love to see this merged.
Is the repository still in a feature freeze ? or is it possible to decide whether or not to merge this ?

@fabpot
Copy link
Member

fabpot commented Jun 14, 2016

Adding a priority like done here looks good to me.

@GuilhemN
Copy link
Contributor Author

@fabpot I'll add some tests then :)

BTW maybe we will have to deprecate the set* functions now ?
They were useful to put a pass before the built-in ones but now I don't know if we should keep them. If we keep them, we should then decide if it should be possible to define priorities or if the passes should always be added with a priority of 0 as done for now.

@fabpot
Copy link
Member

fabpot commented Jun 16, 2016

I would indeed deprecate the set* methods.

@GuilhemN
Copy link
Contributor Author

@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 Compiler or the ContainerBuilder ?

@GuilhemN
Copy link
Contributor Author

I think if we really want to deprecate the set* methods we should probably do that later in another PR as we need to find a cleaner substitute which is out of the scope of this PR

3.2.0
-----

* allowed to prioritize compiler passes by introducing a third argument to `PassConfig::addPass()`, to `Compiler::addPass` and to `ContainerBuilder::addCompilerPass()`
Copy link
Member

Choose a reason for hiding this comment

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

missing () on addPass

@fabpot
Copy link
Member

fabpot commented Jun 21, 2016

Thank you @Ener-Getick.

@fabpot fabpot closed this in 81f4f63 Jun 21, 2016
@GuilhemN GuilhemN deleted the PASSCONFIG branch August 22, 2016 12:52
@fabpot fabpot mentioned this pull request Oct 27, 2016
fabpot added a commit that referenced this pull request Nov 4, 2016
…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
@TomasVotruba
Copy link
Contributor

Great feature. Just needed it :) Thank you @GuilhemN

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

Successfully merging this pull request may close these issues.

8 participants