Skip to content

[DependencyInjection] Allow extensions to register compiler passes #50444

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
Closed
wouterj wants to merge 1 commit into from
Closed

[DependencyInjection] Allow extensions to register compiler passes #50444

wouterj wants to merge 1 commit into from

Conversation

@wouterj
Copy link
Member

@wouterj wouterj commented May 26, 2023 •
edited
Loading

Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? yes
Tickets -
License MIT
Doc PR -

Without the bundle system, the DI extension is sometimes used for plugins/add-ons in projects. I've recently introduced this to the phpdocumentor/guides package (which will probably be the new RST parser that we're going to use at symfony.com some time in the future).

To make the DI extension fully workable as a plugin class, it would be great if it could also register compiler passes. This PR introduces a new method for this purpose: ExtensionInterface::build() Most users will probably not get a deprecation, as Extension has a default implementation.

Build precedence is: KernelInterface::build() > BundleInterface::build() > ExtensionInterface::build()

@carsonbot carsonbot added this to the 6.4 milestone May 26, 2023
derrabus
derrabus reviewed May 26, 2023
View reviewed changes
derrabus
derrabus requested changes May 26, 2023
View reviewed changes
Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

Blocking the merge because there are no tests. 😉

yceruto
yceruto reviewed Jun 9, 2023
View reviewed changes
Copy link
Member

@yceruto yceruto left a comment

Choose a reason for hiding this comment

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

I've the same need in some projects. Thanks for working on it!

@wouterj
Copy link
Member Author

wouterj commented Jun 10, 2023

Thanks for the reviews. PR is updated by only introducing an ExtensionInterface::build() method. Somehow, I forgot that implementing CompilerPassInterface from a DI extension already works.

There is one decision left: Should we move the build() method to a specific interface (like we do for configuration), to remove the deprecation notice?
I think it's safe like this, most of the users will extend Extension or AbstractExtension, which already implement this method for them. Having yet another interface that you can implement in an extension seems a bit overkill.

derrabus
derrabus reviewed Jun 10, 2023
View reviewed changes
@wouterj
Copy link
Member Author

wouterj commented Jun 10, 2023 •
edited
Loading

Please keep merge blocked: One test failure is related (need a BC layer for KernelInterface classes implementing ExtensionInterface).

@wouterj wouterj force-pushed the extension-pass branch 2 times, most recently from 76e8814 to a1b35cf Compare June 12, 2023 12:39
@wouterj
Copy link
Member Author

wouterj commented Jun 12, 2023

Status: Needs review

The tests failures that are remaining are unrelated.

@derrabus
Copy link
Member

derrabus commented Jun 13, 2023

Would it make sense to deprecate BundleInterface::build()?

@stof
Copy link
Member

stof commented Jun 13, 2023

Would it make sense to deprecate BundleInterface::build()?

Clearly not yet. Having the same version adding a new way to register compiler passes and deprecating the existing way would be a nightmare for the ecosystem.
If we want to deprecate it, it should be done after the EOM of Symfony 5.4 so that bundles don't need to drop support of the 5.4 LTS as soon as 6.4 is released.

Side note, the comment in #50444 (comment) makes me think that we might not want to deprecate it at all.

nicolas-grekas
nicolas-grekas reviewed Jun 21, 2023
View reviewed changes
@wouterj
Copy link
Member Author

wouterj commented Aug 12, 2023

Precedence fixed and updated the PR description to match current state. Ready to merge as far as I'm concerned.

@wouterj
Copy link
Member Author

wouterj commented Sep 27, 2023 •
edited
Loading

The failing test revealed a conflict when you have a class Kernel&ExtensionInterface. In this case, the build() method of that class is called twice (once for both interfaces).

As the primary use-case of this PR is already supported (#13761), I'll close here. If someone else is interested in this feature, feelf free to continue with what's available in this PR :)

@wouterj wouterj closed this Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@stof stof stof requested changes

@derrabus derrabus derrabus approved these changes

@yceruto yceruto yceruto approved these changes

+1 more reviewer

@ro0NL ro0NL ro0NL left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

DependencyInjection Feature Status: Reviewed

Projects

None yet

Milestone

6.4

Development

Successfully merging this pull request may close these issues.

7 participants

@wouterj @derrabus @stof @nicolas-grekas @ro0NL @yceruto @carsonbot