Skip to content

[2.8] Make service not shared when prototype scope is set #15091

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

Merged
merged 1 commit into from
Jun 24, 2015

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Jun 24, 2015

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

Deprecating the scopes was introducing a BC break in Symfony 2.8 when using the prototype scope. As $shared wasn't updated if the scope was set to prototype, people will always get an error because of the CheckDefinitionValidityPass.

@fabpot
Copy link
Member

fabpot commented Jun 24, 2015

👍

@wouterj
Copy link
Member Author

wouterj commented Jun 24, 2015

Tests should be fixed now as well :)

@dosten
Copy link
Contributor

dosten commented Jun 24, 2015

👍

@@ -33,6 +33,6 @@
<configurator class="%baz_class%" method="configureStatic1"/>
</service>
<service id="factory_service" class="Bar" factory-method="getInstance" factory-service="foo.baz"/>
<service id="foo_bar" class="%foo_class%" scope="prototype"/>
<service id="foo_bar" class="%foo_class%" shared="false" scope="prototype"/>
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the additional prototype attribute?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is to test the output of the XmlDumper. As this service is a legacy service, it only sets the scope to prototype (which remains the same). The only thing this PR changed was also setting shared to false in these cases

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is needed, see here and here. All services of prototype scope are non-shared by default.

@dosten
Copy link
Contributor

dosten commented Jun 24, 2015

@fabpot btw, do you think that we need to keep the correct shared value when use the DefinitionDecorator?

@xabbuh
Copy link
Member

xabbuh commented Jun 24, 2015

And shouldn't we add a dedicated test for this to avoid regressions?

@dosten
Copy link
Contributor

dosten commented Jun 24, 2015

@xabbuh Yes, we need a test to avoid regressions

@xabbuh
Copy link
Member

xabbuh commented Jun 24, 2015

👍 with an additional test

@wouterj
Copy link
Member Author

wouterj commented Jun 24, 2015

Added a very simple legacy test

@xabbuh
Copy link
Member

xabbuh commented Jun 24, 2015

👍

1 similar comment
@dosten
Copy link
Contributor

dosten commented Jun 24, 2015

👍

@Tobion
Copy link
Contributor

Tobion commented Jun 24, 2015

Thank you @wouterj.

@Tobion Tobion merged commit b7030cc into symfony:2.8 Jun 24, 2015
Tobion added a commit that referenced this pull request Jun 24, 2015
…t (WouterJ)

This PR was merged into the 2.8 branch.

Discussion
----------

[2.8] Make service not shared when prototype scope is set

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Deprecating the scopes was introducing a BC break in Symfony 2.8 when using the prototype scope. As `$shared` wasn't updated if the scope was set to `prototype`, people will always get an error because of [the `CheckDefinitionValidityPass`](https://github.com/symfony/DependencyInjection/blob/04d5d925a987f35854f51b1c468c0ca81e1d61b9/Compiler/CheckDefinitionValidityPass.php#L54-L57).

Commits
-------

b7030cc Make service not shared when prototype scope is set
@wouterj wouterj deleted the patch-10 branch June 25, 2015 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants