Skip to content
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

[DependencyInjection] deprecated synchronized services #13289

Merged
merged 1 commit into from Jan 9, 2015

Conversation

@fabpot
Copy link
Member

fabpot commented Jan 6, 2015

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 6, 2015

No test to "Legacy" split ?

@@ -669,9 +673,13 @@ public function setSynchronized($boolean)
* @return bool
*
* @api
*
* @deprecated since version 2.7, will be removed in 3.0.
*/
public function isSynchronized()

This comment has been minimized.

Copy link
@stof

stof Jan 7, 2015

Member

this should get the extra $triggerDeprecationWarning argument to allow the dumper to avoid triggering the warning for all services while still supporting the feature (similar to #13292)

@fabpot fabpot force-pushed the fabpot:synchronize-deprecation branch 4 times, most recently from 8d76faf to 9a0ac34 Jan 7, 2015
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 7, 2015

👍

@fabpot fabpot force-pushed the fabpot:synchronize-deprecation branch from 9a0ac34 to 98abc7b Jan 7, 2015
@stof

This comment has been minimized.

Copy link
Member

stof commented Jan 7, 2015

We should probably set a special case for the request service in synchronize() to avoid triggering the deprecation warning when setting the request service in the container in HttpKernel

if ($value = $service->getAttribute($key)) {
$method = 'set'.str_replace('-', '', $key);
$definition->$method(XmlUtils::phpize($value));
}
}

if ($value = $service->getAttribute('synchronized')) {
$definition->setSynchronized(XmlUtils::phpize($value), false);

This comment has been minimized.

Copy link
@stof

stof Jan 7, 2015

Member

the second argument should be not be used here. We want other bundles to be notified if they define a synchronized service. FrameworkBundle should handle it it by setting it as synchronized in the DI extension rather than in the XML file to be able to ignore the deprecation warning

@@ -171,7 +171,7 @@ private function parseDefinition($id, $service, $file)
}

if (isset($service['synchronized'])) {
$definition->setSynchronized($service['synchronized']);
$definition->setSynchronized($service['synchronized'], false);

This comment has been minimized.

Copy link
@stof

stof Jan 7, 2015

Member

same issue here

@fabpot fabpot force-pushed the fabpot:synchronize-deprecation branch 4 times, most recently from d0fdccc to 7df443f Jan 7, 2015
@fabpot fabpot force-pushed the symfony:2.7 branch from 77b6cca to dcfc338 Jan 8, 2015
@fabpot fabpot force-pushed the fabpot:synchronize-deprecation branch 4 times, most recently from af64908 to d7cd280 Jan 9, 2015
@fabpot fabpot force-pushed the fabpot:synchronize-deprecation branch from d7cd280 to 82db9c2 Jan 9, 2015
@fabpot fabpot merged commit 82db9c2 into symfony:2.7 Jan 9, 2015
1 of 2 checks passed
1 of 2 checks passed
fabbot.io Doing some proofreading and checking your coding style.
Details
continuous-integration/travis-ci The Travis CI build passed
Details
fabpot added a commit that referenced this pull request Jan 9, 2015
… (fabpot)

This PR was merged into the 2.7 branch.

Discussion
----------

[DependencyInjection] deprecated synchronized services

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

Commits
-------

82db9c2 [DependencyInjection] deprecated synchronized services
@fabpot fabpot deleted the fabpot:synchronize-deprecation branch Jan 9, 2015
*/
public function setSynchronized($boolean)
public function setSynchronized($boolean, $triggerDeprecationError = true)

This comment has been minimized.

Copy link
@hhamon

hhamon Jan 10, 2015

Contributor

Isn't it a BC break to add an extra argument to a public method signature? The method is flagged with @api...

This comment has been minimized.

Copy link
@stloyd

stloyd Jan 10, 2015

Contributor

It's not on interface so if you overwrite class you don't need to copy params, yet this argument is optional, so I guess it's not real BC break.

This comment has been minimized.

Copy link
@stof

stof Jan 12, 2015

Member

thus, the Definition class is not really meant to be a class extended by inheritance. I don't see any use case where this would bring any benefit actually.

This comment has been minimized.

Copy link
@hhamon

hhamon Jan 13, 2015

Contributor

Sure.

*/
public function isSynchronized()
public function isSynchronized($triggerDeprecationError = true)

This comment has been minimized.

Copy link
@hhamon

hhamon Jan 10, 2015

Contributor

Same here.

@tkleinhakisa

This comment has been minimized.

Copy link

tkleinhakisa commented Jan 12, 2015

Hi,
I am using a synthetic synchronized service (not the request) in my application
Is there any replacement for this feature or suggested way to accomplish a similar behavior ?
Can you suggest any reading to understand the benefit of removing it ?

Thank you

@stof

This comment has been minimized.

Copy link
Member

stof commented Jan 12, 2015

@tkleinhakisa what is your use case for the synchronized service ?

The benefit of removal is that this feature is quite complex for something which is not really needed and was the wrong way to solve our problem. We were not really satisfied when introducing it, but we needed a way to fix the handling of requests. We then figured out that it was not even fixing all our cases, so we continued searching for a better architecture, which is when we introduced the RequestStack in 2.6.
And the benefit of this new architecture is that it makes it possible to use the RequestStack with any DIC (or without DIC). The fact that we started discussing the way to introduce synchronized services in Pimple to solve the same request issue in Silex was what made clear that synchronized services were the wrong way to fix the issue.

@tkleinhakisa

This comment has been minimized.

Copy link

tkleinhakisa commented Jan 13, 2015

Hi @stof and thank you for the explanations. I will have a look at the request stack (we are not using 2.6 yet)

Our application can be accessed from multiples domain, if you access it from www.site1.com you're a "site1" user, www.site2.com "site2" user ...
The synchronized service represent which site the application is working for
If the site is changed, then the routing is changed (different urls), some translators parameters are changed (site.name)

Exemple: user1 from site1 take action on user2 from site2, i can swich the site, generate the good urls and messages for user2, then rollback the site to its original value and redirect user1 to the correct url with his domain.

Do you think a similar design to the RequestStack could be applied ?
Thanks

@stof

This comment has been minimized.

Copy link
Member

stof commented Jan 13, 2015

The RequestStack is our way to solve the issue of getting the current Request object in Symfony 2.4+. You don't need to use 2.6 to get it. See the UPGRADE-2.4 file

@tkleinhakisa

This comment has been minimized.

Copy link

tkleinhakisa commented Jan 13, 2015

Thank you, i think i can implement something similar for my use case

@andrerom andrerom mentioned this pull request Apr 9, 2015
5 of 5 tasks complete
@lolautruche

This comment has been minimized.

Copy link
Contributor

lolautruche commented Apr 13, 2015

Hi

Sorry to come after the battle... Unfortunately, we use synchronized services in eZ for our dynamic settings (not for request). Is there some way to workaround this deprecation and future removal?

@stof

This comment has been minimized.

Copy link
Member

stof commented Aug 2, 2015

@lolautruche please open a dedicated issue to discuss it, describing your exact use case so that we can help you find a solution.
And couldn't you solve it in a way similar to the RequestStack for the case of the Request ?

@lolautruche

This comment has been minimized.

Copy link
Contributor

lolautruche commented Aug 2, 2015

I managed to refactor our dynamic settings using expression language (see ezsystems/ezpublish-kernel#1302). Services that depend on those settings are now synchronized manually.

fabpot added a commit that referenced this pull request Aug 15, 2016
…definitions (ogizanagi)

This PR was merged into the 3.1 branch.

Discussion
----------

[DependencyInjection] ContainerBuilder: Remove obsolete definitions

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

IIUC, [the obsolete definitions thing was tied to scoped & sync services](#7007).
Thus, this code [is not meant to be used since 3.0 and can be removed](#13289).

However, it may be considered as a BC break and would require introducing a new deprecation instead, because the current code allows to set a service on a frozen container if it has an obsolete synthetic definition...which is weird, isn't it ? (I doubt this feature was explicitly intended to allow setting a synthetic service more than once, and it wasn't before sync services introduction)

I suggest this as a patch for 3.1, under the pretext of forgotten code tied to a previous deprecation & feature removal, but let me know if it should be considered differently.

Commits
-------

daa7d00 [DependencyInjection] ContainerBuilder: Remove obsolete definitions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.