Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[DependencyInjection] Compiler Pass Cusomization Type#10778

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed

Conversation

@timglabisch
Copy link

QA
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets
LicenseMIT
Doc PR

i stumbled around the problem that i wasn't able to create a compiler pass that looks for tags and create new tags.

For example if you want to provide a tag that allows you to register an amqp worker,
you won't be able to register a command by using a tag.
The main problem is that the default pass type (TYPE_BEFORE_OPTIMIZATION) is the one that will be executed.

The pull request will add a compiler pass type (+ pre and after) that runs before the default one.
This will allow you for example to create other tags by tags.

I added a bunch of tests (also for the existing passes), because the tests passed even if i broke some passes and api methods.

I called the new type CUSTOMIZE because it will allow you to customize the default behaviour, before any kind of OPTIMIZATION.

This is my first symfony related PR, so pls review carefully.

Copy link
Member

Choose a reason for hiding this comment

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

we don't use phpdocs for tests

Copy link
Author

Choose a reason for hiding this comment

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

i removed the phpdocs.

@timglabischtimglabisch changed the titleCompiler Pass Cusomization Type[DependencyInjection] Compiler Pass Cusomization TypeApr 24, 2014
@jakzal
Copy link
Contributor

I'm not convinced this change is really necessary. Could you elaborate more on your use case?

This will allow you for example to create other tags by tags.

What do you mean by creating tags by other tags? Tags cannot be created in the container. That's services which are being tagged.

@timglabisch
Copy link
Author

just try to write a compiler pass that adds a service with a console.command tag on the fly.

for example you want to tag something as a amqp worker using tags and you would like your bundle to create a console command for this worker.

Using this patch i could create a definition on the fly in my compiler pass and add the tag console.command. The service that was tagged as amqp worker could be injected to the service i created on the fly and could tagged as a command.

indeed you could just look at the console.command compiler pass and modify the console.command.idshttps://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/AddConsoleCommandPass.php#L47
but this makes your bundle depending on internals of the ConsoleCommandPass. I would like to tag a service with the console.command tag using a compiler pass.

this is just the example i stumbled around. it's all about modifying tags before other (like the default) bundles will consume them.

the problem is that the console.command compiler pass runs before yours, and there is no higher priority than the default one.
i think there should be something greater than the default priority, especially if the framework passes runs before yours.

Also doing such stuff is not about "Optimization".
its about tweaking / customization the container in a (i think) legal way using a compiler pass.

Bundles offer tags as some kind of "public api", other bundles should be able to customise these tags as easy as possible, even if it depends on other tags from other bundles.

@wouterj
Copy link
Member

The main problem with the PassConfig is that the current "priority" is the highest "priority" there is nothing you can use to execute somethingbefore the default.

Besides this, the "optimalization" phrase of the PassConfig is quite misused these days by passes like the console command pass. They aren't optimalizations, they are customizations.

So I would also propose to change the standard priority to the Customization "event" and add a lot of the current passes to this CUSTOMIZATION event.

However, there will be a problem with the optimization. You might want to use some optimization things in the customization (e.g. the parameters pass), but you also want to optimize the newly added things of the CUSTOMIZATION.

@timglabisch
Copy link
Author

However, there will be a problem with the optimization. You might want to use some optimization things in the customization (e.g. the parameters pass), but you also want to optimize the newly added things of the CUSTOMIZATION.

This is true. The problem is that the optimization passes are used for resolving values.
A solution could be splitting the optimization passes to resolving and optimization passes?
this would change the result of the api method getOptimizationPasses.

@jrdnrc
Copy link

+1, I've just stumbled into this problem too. My application creates form types dynamically, and I wanted to put them in a CompilerPass, however upon doing that they no longer register properly. The tags do register in the BundleExtension classes, however.

@Lumbendil
Copy link

What about adding a third parameter which is an integer, such as->addCompilerPass($compilerPass, $step, $priority)

This would work the same way that priority for event listeners.

@jrdnrc
Copy link

@Lumbendil that sounds good actually, and then you could always be sure to hook in at the right time

@timglabisch
Copy link
Author

What about adding a third parameter which is an integer, such as ->addCompilerPass($compilerPass, > $step, $priority)

adding the CUSTOMIZE step + using priorities would be a good idea but

  1. right now there _BEFORE, _AFTER types. Removing them would be an unnecessary BC.
    so we would end up adding a priority for _BEFORE and _AFTER types.
    this is fine but is some kind of different concern and should end up in a different PR.
  2. this PR is about the step 'CUSTOMIZE', it must be a dedicated step.
    it would be so wrong using TYPE_BEFORE_OPTIMIZATION because it's not about optimizing, it's about customizing.

@timglabisch
Copy link
Author

should i close this PR? it's hanging around for a while

@Lumbendil
Copy link

@timglabisch it isn't incorrect, because the pass isn't for optimization, is to do things before optimization. It's the default because you usually want to do things on this step, so they can be optimized/removed. Maybe it could have a different name, but I don't feel it's a bad name.

@wouterj
Copy link
Member

Ping, anything left to do in this PR?

@egeloen
Copy link

I'm not sure adding new passes before the optimization one will solve all use cases. If someone register services according to tags in a compiler pass registered on the very first pass we are still stuck due to the missing execution order control of the comiler passes inside a specific pass.

@timglabisch
Copy link
Author

@wouterj i am not sure, i still think this PR is useful =)

@FlyingDR
Copy link
Contributor

I have similar requirement in one of my projects that usesSonataAdminBundle. In that project I end up with moving registration of my application bundle inAppKernel::registerBundles beforeSonataAdminBundle registration.

However adding more compiler passes may not solve the problem since in each of new passes it is possible to get undesirable order of registered compiler passes.

It would be much better (at my sight) to add$priority argument toContainerBuilder::addCompilerPass to achieve same flexibility asEventDispatcher::addListener already have.

@docteurklein
Copy link
Contributor

This would be a very good addition. Currently, most of the compiler passes (even FrameworkBundle ones) dealing with tags are registered with TYPE_BEFORE_OPTIMIZATION, while they should really be at TYPE_AFTER_REMOVING.

Because of that, a compiler pass registered after (because bundle is registered after) will not be able to remove a service definition before another pass started to reference it.

The only solution would be to sneak into the processbefore they are called. Since the default type is the highest priority, it makes sense to add at least one type with an even higher priority.

A temporary solution for an application is to register it before all the others, in the kernel:

protectedfunctionprepareContainer(ContainerBuilder$container){$container->addCompilerPass(newIAmFirst);parent::prepareContainer($container);}

@fabpot
Copy link
Member

Closing in favor of#18022

jrdnrc reacted with thumbs up emoji

@fabpotfabpot closed thisJun 21, 2016
fabpot added a commit that referenced this pull requestJun 21, 2016
…y (Ener-Getick)This PR was squashed before being merged into the 3.2-dev branch (closes#18022).Discussion----------[DependencyInjection] Sort the CompilerPass by priority| 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 ?Commits-------d17c1a9 [DependencyInjection] Sort the CompilerPass by priority
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@timglabisch@jakzal@wouterj@jrdnrc@Lumbendil@egeloen@FlyingDR@docteurklein@fabpot@javiereguiluz

[8]ページ先頭

©2009-2025 Movatter.jp