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

Appearance settings

[DependencyInjection] Don't autoconfigure decorators #24229

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

dunglas
Copy link
Member

@dunglas dunglas commented Sep 16, 2017

Q A
Branch? 3.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets api-platform/api-platform#414
License MIT
Doc PR n/a

Autoconfiguring decorators create strange problems like this one api-platform/api-platform#414.
As autoconfiguring is on by default when using Symfony SE 3.3, it's better to give an explicit error to the end user.

TODO:

  • Add tests

@@ -432,6 +433,10 @@ public function getInstanceofConditionals()
*/
public function setAutoconfigured($autoconfigured)
{
if (null !== $this->decoratedService && $autoconfigured) {
Copy link
Member Author

@dunglas dunglas Sep 16, 2017

Choose a reason for hiding this comment

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

Maybe should we also throw in setDecoratedService if $autoconfigured is true?

@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Sep 16, 2017
@nicolas-grekas
Copy link
Member

I barely see the logic link between the original issue and making autoconfig/decoration conflict. Can you explain why they should? What is causing the reported exception precisely?

@nicolas-grekas
Copy link
Member

So, debugging this, I'm 👎 on this patch, but would propose another solution:

--- a/src/Symfony/Component/DependencyInjection/Compiler/ResolveInstanceofConditionalsPass.php
+++ b/src/Symfony/Component/DependencyInjection/Compiler/ResolveInstanceofConditionalsPass.php
@@ -119,6 +119,7 @@ class ResolveInstanceofConditionalsPass implements CompilerPassInterface
             $abstract
                 ->setArguments(array())
                 ->setMethodCalls(array())
+                ->setDecoratedService(null)
                 ->setTags(array())
                 ->setAbstract(true);
         }

I'll let you do the PR if you don't mind?

@dunglas
Copy link
Member Author

dunglas commented Sep 16, 2017

Closing in favor of #24234. Thanks for debugging this @nicolas-grekas.

@dunglas dunglas closed this Sep 16, 2017
@dunglas dunglas deleted the dont-autoconfigure-decorators branch September 16, 2017 18:56
fabpot added a commit that referenced this pull request Sep 17, 2017
…tionalsPass (dunglas)

This PR was merged into the 3.3 branch.

Discussion
----------

[DI] Fix decorated service merge in ResolveInstanceofConditionalsPass

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | api-platform/api-platform#414, #24229
| License       | MIT
| Doc PR        | n/a

Follows #24229. It's [the patch](#24229 (comment)) proposed by @nicolas-grekas + a test.

Commits
-------

2a30908 [DI] Fix decorated service merge in ResolveInstanceofConditionalsPass
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.