[DependencyInjection] deprecated synchronized services #13289

fabpot · 2015-01-06T16:19:59Z

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 · 2015-01-06T16:53:27Z

No test to "Legacy" split ?

stof · 2015-01-07T13:48:57Z

src/Symfony/Component/DependencyInjection/Definition.php

@@ -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 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)

nicolas-grekas · 2015-01-07T16:22:29Z

👍

stof · 2015-01-07T16:38:20Z

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

stof · 2015-01-07T16:40:00Z

src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php

            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);


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

… (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

hhamon · 2015-01-10T16:04:30Z

src/Symfony/Component/DependencyInjection/Definition.php

     */
-    public function setSynchronized($boolean)
+    public function setSynchronized($boolean, $triggerDeprecationError = true)


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

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.

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.

tkleinhakisa · 2015-01-12T12:15:50Z

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 · 2015-01-12T15:09:46Z

@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 · 2015-01-13T08:38:59Z

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 · 2015-01-13T08:45:37Z

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 · 2015-01-13T14:00:31Z

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

lolautruche · 2015-04-13T09:07:01Z

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 · 2015-08-02T00:27:18Z

@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 · 2015-08-02T15:09:28Z

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.

…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

stof reviewed Jan 7, 2015
View reviewed changes

fabpot force-pushed the synchronize-deprecation branch 4 times, most recently from 8d76faf to 9a0ac34 Compare January 7, 2015 16:21

fabpot force-pushed the synchronize-deprecation branch from 9a0ac34 to 98abc7b Compare January 7, 2015 16:27

stof reviewed Jan 7, 2015
View reviewed changes

fabpot force-pushed the synchronize-deprecation branch 4 times, most recently from d0fdccc to 7df443f Compare January 7, 2015 21:47

fabpot force-pushed the 2.7 branch from 77b6cca to dcfc338 Compare January 8, 2015 13:06

fabpot force-pushed the synchronize-deprecation branch 4 times, most recently from af64908 to d7cd280 Compare January 9, 2015 17:19

[DependencyInjection] deprecated synchronized services

82db9c2

fabpot force-pushed the synchronize-deprecation branch from d7cd280 to 82db9c2 Compare January 9, 2015 17:43

fabpot merged commit 82db9c2 into symfony:2.7 Jan 9, 2015

fabpot deleted the synchronize-deprecation branch January 9, 2015 18:03

hhamon reviewed Jan 10, 2015
View reviewed changes

andrerom mentioned this pull request Apr 9, 2015

[Symfony 2.7] Fix deprecation warnings ezsystems/ezpublish-kernel#1221

Merged

5 tasks

ogizanagi mentioned this pull request Aug 13, 2016

[DependencyInjection] ContainerBuilder: Remove obsolete definitions #19608

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DependencyInjection] deprecated synchronized services #13289

[DependencyInjection] deprecated synchronized services #13289

fabpot commented Jan 6, 2015

nicolas-grekas commented Jan 6, 2015

stof Jan 7, 2015

nicolas-grekas commented Jan 7, 2015

stof commented Jan 7, 2015

stof Jan 7, 2015

hhamon Jan 10, 2015

stloyd Jan 10, 2015

stof Jan 12, 2015

hhamon Jan 13, 2015

tkleinhakisa commented Jan 12, 2015

stof commented Jan 12, 2015

tkleinhakisa commented Jan 13, 2015

stof commented Jan 13, 2015

tkleinhakisa commented Jan 13, 2015

lolautruche commented Apr 13, 2015

stof commented Aug 2, 2015

lolautruche commented Aug 2, 2015

[DependencyInjection] deprecated synchronized services #13289

[DependencyInjection] deprecated synchronized services #13289

Conversation

fabpot commented Jan 6, 2015

nicolas-grekas commented Jan 6, 2015

stof Jan 7, 2015

Choose a reason for hiding this comment

nicolas-grekas commented Jan 7, 2015

stof commented Jan 7, 2015

stof Jan 7, 2015

Choose a reason for hiding this comment

hhamon Jan 10, 2015

Choose a reason for hiding this comment

stloyd Jan 10, 2015

Choose a reason for hiding this comment

stof Jan 12, 2015

Choose a reason for hiding this comment

hhamon Jan 13, 2015

Choose a reason for hiding this comment

tkleinhakisa commented Jan 12, 2015

stof commented Jan 12, 2015

tkleinhakisa commented Jan 13, 2015

stof commented Jan 13, 2015

tkleinhakisa commented Jan 13, 2015

lolautruche commented Apr 13, 2015

stof commented Aug 2, 2015

lolautruche commented Aug 2, 2015