Skip to content

[DependencyInjection] Do not ignore tags name attribute when it does not define their name #50088

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
Merged
merged 1 commit into from
Apr 21, 2023
Merged

[DependencyInjection] Do not ignore tags name attribute when it does not define their name #50088

merged 1 commit into from
Apr 21, 2023
+24 −2

Conversation

MatTheCat
Copy link
Contributor

@MatTheCat MatTheCat commented Apr 21, 2023 •
edited
Loading

Q A
Branch? 6.2
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #50081
License MIT
Doc PR N/A

Tags name attribute is ignored using XML if the tag name is its node content. That means <tag name="name_attribute">tag_name</tag> will return a tag_name tag without any attribute.

This seems to be a regression from #36586.

@carsonbot
Copy link

Hey!

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

@MatTheCat MatTheCat mentioned this pull request Apr 21, 2023
Closed
@MatTheCat MatTheCat changed the base branch from 6.3 to 6.2 April 21, 2023 10:56
@MatTheCat MatTheCat marked this pull request as ready for review April 21, 2023 10:57
@carsonbot carsonbot added this to the 6.2 milestone Apr 21, 2023
@MatTheCat MatTheCat force-pushed the xml-tag-name-attribute branch from f506d77 to e682868 Compare April 21, 2023 11:14
@MatTheCat MatTheCat changed the title [DependencyInjection] XML tags name attribute ignored if the tag’s name is its node content [DependencyInjection] Do not ignore tags name attribute when it does not define their name Apr 21, 2023
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Apr 21, 2023 •
edited
Loading

Thanks; Can you add a test case please?

nicolas-grekas
nicolas-grekas approved these changes Apr 21, 2023
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

nvm, there are test cases already :)

@PhilETaylor
Copy link
Contributor

I have taken these proposed changes to src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php and applied them, manually, in my live project that identified this issue originally,

Then, after a cache:clear, this problem has been resolved. So I confirm this PR fixes my original issue, in the original app that was complaining.

Thanks @MatTheCat - please consider signing up to GitHub Sponsors :)

@MatTheCat
Copy link
Contributor Author

Thanks for testing on a real project @PhilETaylor! Will look at this sponsorship thing 👀

@nicolas-grekas nicolas-grekas force-pushed the xml-tag-name-attribute branch from 5a88934 to 4c9c688 Compare April 21, 2023 15:07
@nicolas-grekas
Copy link
Member

Thank you @MatTheCat.

@nicolas-grekas nicolas-grekas merged commit 6f77fa0 into symfony:6.2 Apr 21, 2023
@MatTheCat MatTheCat deleted the xml-tag-name-attribute branch April 21, 2023 15:13
@fabpot fabpot mentioned this pull request Apr 28, 2023
Merged
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 approved these changes

Assignees
No one assigned
Labels
Bug DependencyInjection Status: Reviewed
Projects
None yet
Milestone
6.2
Development

Successfully merging this pull request may close these issues.

[6.3] non-existent resolver Symfony\Bridge\Doctrine\ArgumentResolver\EntityValueResolver
4 participants
@MatTheCat @carsonbot @nicolas-grekas @PhilETaylor