8000 [DependencyInjection] Fix autowiring tagged arguments from attributes by okhoshi · Pull Request #43579 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[DependencyInjection] Fix autowiring tagged arguments from attributes #43579

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
Oct 26, 2021
Merged

[DependencyInjection] Fix autowiring tagged arguments from attributes #43579

merged 1 commit into from
Oct 26, 2021

Conversation

@okhoshi
Copy link
Contributor
@okhoshi okhoshi commented Oct 19, 2021 •
edited
Loading
Q A
Branch? 5.3
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #43272
License MIT
Doc PR no

Reimplement #40406 with AttributeAutoconfigurationPass to avoid the BC following the change in CompilerPass ordering in PassConfig.

Also revert the various fix made in an attempt to recover the BC introduced by #40406.

Note: 5.4 branch was needed because AttributeAutoconfigurationPass is not handling parameters in 5.3

To-do:

@okhoshi okhoshi requested a review from dunglas as a code owner October 19, 2021 10:03
@carsonbot carsonbot added this to the 5.4 milestone Oct 19, 2021
@carsonbot
Copy link
carsonbot commented Oct 19, 2021

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 5.4 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@carsonbot carsonbot changed the title Fix 43272 [DependencyInjection] Fix 43272 Oct 19, 2021
@okhoshi okhoshi force-pushed the fix-43272 branch 5 times, most recently from 6a33b9b to b064cca Compare October 19, 2021 21:47
@nicolas-grekas
Copy link
Member
nicolas-grekas commented Oct 20, 2021

Thanks for the PR, I think moving attributes resolution from AutowirePass to AttributeAutoconfigurationPass is a nice approach.

Yet I see three issues that should be fixed to me:

  • this targets 5.4 while 5.3 is affected
  • this requires enabling autoconfiguration (as seen in patched tests)
  • this moves registering the attributes to FrameworkBundle (as seen in patched tests and highlighted by @stof).

I think all 3 issues can be solved by patching AttributeAutoconfigurationPass on 5.3 so that it hardcodes a special behavior for those two attributes (they were already hardcoded in AutowirePass anyway.). This should allow not patching most test cases, which is desired.

@okhoshi
Copy link
Contributor Author
okhoshi commented Oct 20, 2021

Thanks for the comments !

Is it possible to target 5.3 since the AttributeAutoconfigurationPass has drastically changed with 5.4 ? AFAIK, there's no logic to apply attributes from parameters, only on the classes.

When you say that you would hardcode the attributes in the AttributeAutoconfigurationPass, do you mean that you would register them in the $parameterAttributeConfigurators or that you would hardcode the transformation in processValue directly ?
As a personal taste, I would do the former, but it would still require enabling the autoconfiguration. By passing the autoconfiguration would likely require deep changes in AttributeAutoconfiguration.

As a side note, I had to change the behavior of ResolveInstanceofConditionalsPass to recursively resolve nested Definition (without that, one of the tests was not passing anymore).

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Oct 20, 2021 •
edited
Loading

We need to fix this issue in 5.3, and I don't have a better idea.
We should not require autconfiguration to be enabled as that'd be a behavior change (booo scary BC breaks 👻).
It'd suggest starting with a copy/paste from 5.4 and then removing everything that is not strictly needed to fix this issue, while keeping the common parts as near as possible to ease merging up.

I had to change the behavior of ResolveInstanceofConditionalsPass to recursively resolve nested Definition

Yes, I didn't full grasp this part but I'll review more carefully after the rest is fixed :)

@okhoshi
Copy link
Contributor Author
okhoshi commented Oct 20, 2021

@nicolas-grekas I pushed a rework of the branch. It works with the IntegrationTest, but it breaks some tests on the side, mainly because the AttributeAutoconfigurationPass needs to run every time now (I spotted errors in the tests because getConstructor was called on a Definition for which the factory was not existing, not sure how I should handle that ?)

I'll fix the other tests tomorrow.

Also, I noticed that #43386 has been merged on 5.4 only and it's modifying the tags, so conflicts will arise :(.

@okhoshi okhoshi changed the base branch from 5.4 to 5.3 October 20, 2021 21:33
@nicolas-grekas nicolas-grekas changed the title [DependencyInjection] Fix 43272 [DependencyInjection] Fix autowiring tagged arguments from attributes Oct 22, 2021
@nicolas-grekas nicolas-grekas force-pushed the fix-43272 branch 4 times, most recently from 9aa90b8 to 55a39c0 Compare October 22, 2021 17:26
nicolas-grekas
nicolas-grekas approved these changes Oct 22, 2021
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

While reviewing more carefully, I found that patching AttributeAutoconfigurationPass would not work well with autowiring of @required methods, or with named arguments, etc.

Instead, I pushed a new pass that is dedicated to autowiring tagged arguments from attributes. This pass runs after AutowireRequired*Pass and before ServiceLocatorTagPass, thus fixing all concerns.

@nicolas-grekas nicolas-grekas force-pushed the fix-43272 branch 2 times, most recently from 42ca1ca to b2783a7 Compare October 22, 2021 18:06
@nicolas-grekas
Copy link
Member
nicolas-grekas commented Oct 22, 2021

I figured out an even simpler patch, PR updated.

@nicolas-grekas nicolas-grekas modified the milestones: 5.4, 5.3 Oct 26, 2021
@nicolas-grekas
Copy link
Member
nicolas-grekas commented Oct 26, 2021

Thank you @okhoshi.

@nicolas-grekas nicolas-grekas merged commit 97c32a1 into symfony:5.3 Oct 26, 2021
@okhoshi okhoshi deleted the fix-43272 branch October 26, 2021 08:32
@xabbuh xabbuh mentioned this pull request Oct 26, 2021
Merged
@fabpot fabpot mentioned this pull request Oct 29, 2021
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

@dunglas dunglas Awaiting requested review from dunglas dunglas is a code owner

@stof stof Awaiting requested review from stof

Assignees

No one assigned

Labels

Bug DependencyInjection Status: Reviewed

Projects

None yet

Milestone

5.3

Development

Successfully merging this pull request may close these issues.

[DependencyInjection] Non-existent "inner" service when decorated and autowired

5 participants

@okhoshi @carsonbot @nicolas-grekas @stof @derrabus
0