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] Add a warning about tagged services needing to be after the glob definition#16888

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
Tofandel wants to merge1 commit intosymfony:6.1fromTofandel:patch-1

Conversation

Tofandel
Copy link

@TofandelTofandel commentedJun 17, 2022
edited
Loading

It took me 2hours to debug and figure out why my listener wasn't working while following the doc, when it's just because the tag definition was not being applied

Basically just declaring it like this

App\EventListener\MyListener:tags:      -name:'doctrine.orm.entity_listener'event:'postUpdate'entity:'App\Entity\MyEntity'# makes classes in src/ available to be used as services# this creates a service per class whose id is the fully-qualified class nameApp\:resource:'../src/'exclude:      -'../src/DependencyInjection/'      -'../src/Entity/'      -'../src/Kernel.php'

When for it to work it needs to be like

# makes classes in src/ available to be used as services# this creates a service per class whose id is the fully-qualified class nameApp\:resource:'../src/'exclude:      -'../src/DependencyInjection/'      -'../src/Entity/'      -'../src/Kernel.php'App\EventListener\MyListener:tags:      -name:'doctrine.orm.entity_listener'event:'postUpdate'entity:'App\Entity\MyEntity'

This mistake is so easy to make it deserves a proper warning

@OskarStark
Copy link
Contributor

I am not sure, but this sounds like a bug to me 🧐 it should not matter in which order you define your services.

@nicolas-grekas can you please give your opinion on this? Thanks

@nicolas-grekas
Copy link
Member

The last comment of the default services.yaml already explains this.

HeahDude reacted with thumbs up emoji

@OskarStark
Copy link
Contributor

Indeed 👍

# add more service definitions when explicit configuration is needed# please note that last definitions always *replace* previous ones

@Tofandel
Copy link
Author

Well I didn't have this comment in my default one 😕

But I still think this comment is also not explicit enough because it's not really clear that the service would be replaced byApp: because they don'texplicitly have the same name and it takes some brainstorming to understand thatApp: is in fact creating one service for each of it's matched files in it's place (one of which having the same name as a previously defined service)

@javiereguiluzjaviereguiluz added this to the4.4 milestoneOct 21, 2022
@carsonbotcarsonbot changed the titleAdd a warning about tagged services needing to be after the glob definition[DependencyInjection] Add a warning about tagged services needing to be after the glob definitionOct 21, 2022
@javiereguiluz
Copy link
Member

@Tofandel thanks for this contribution. However, instead of adding this message in one of the many articles that show service config, we've decided to add that message in the main article about service config. That's why we've created#17385 to replace this PR.

But your PR was great because it made us rethink about this. Thanks!

javiereguiluz added a commit that referenced this pull requestOct 21, 2022
…service definitions replace previous ones (javiereguiluz)This PR was merged into the 4.4 branch.Discussion----------[DependencyInjection] [DependencyInjecion] Mention that service definitions replace previous onesReplaces#16888 to add the warning message in the main article about services config.Commits-------c9f83a4 [DependencyInjecion] Mention that service definitions replace previous ones
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
4.4
Development

Successfully merging this pull request may close these issues.

5 participants
@Tofandel@OskarStark@nicolas-grekas@javiereguiluz@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp