[DependencyInjection] call synchronize{$id} from get{$id}Service method #8545

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
3 participants
@jaypea
Contributor

jaypea commented Jul 23, 2013

this calls the synchronize method for services that are marked as synchronized but are not synthetic.

when the scope of the requested service is not active, the container won't create the instance nor call the synchronize method.
when the scope gets entered and the service is fetched from container afterwards, the synchronize{$id}Service method is called to update all dependent services.

Jan Prieser
call synchronize{$id} from get{$id}Service method
this calls the synchronize method for services that are marked as synchronized but are not synthetic.
when the scope of the requested service is not active, the container won't create the instance nor call the synchronize method.
when the scope gets entered and the service is injected afterwards, the synchronize{$id}Service method is called to update all dependent services.
@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Jul 23, 2013

Member

Please test this

Member

stof commented Jul 23, 2013

Please test this

@jaypea

This comment has been minimized.

Show comment
Hide comment
@jaypea

jaypea Jul 23, 2013

Contributor

the tests fail with a segmentation fault for php 5.3.3. its running in the other versions jobs.
but this has nothing to do with my code, the master branch is failing since yesterday: https://travis-ci.org/symfony/symfony/builds

Contributor

jaypea commented Jul 23, 2013

the tests fail with a segmentation fault for php 5.3.3. its running in the other versions jobs.
but this has nothing to do with my code, the master branch is failing since yesterday: https://travis-ci.org/symfony/symfony/builds

@@ -420,6 +420,18 @@ private function addServiceProperties($id, $definition, $variableName = 'instanc
return $code;
}
+ private function addServiceSynchronizeCall($id, $definition)

This comment has been minimized.

@stof

stof Jul 23, 2013

Member

could you typehint the Definition ?

@stof

stof Jul 23, 2013

Member

could you typehint the Definition ?

This comment has been minimized.

@jaypea

jaypea Jul 23, 2013

Contributor

there are lots of similar methods in this class without typehints.
if was thinking about it, but decided to stick with the convention in the class.

@jaypea

jaypea Jul 23, 2013

Contributor

there are lots of similar methods in this class without typehints.
if was thinking about it, but decided to stick with the convention in the class.

This comment has been minimized.

@stof

stof Jul 23, 2013

Member

I think typehinting the object arguments is better (and SensioLabs Insight agrees with me as it checks for it). So I think the new code should use the typehint (changing the existing code is probably outside the scope of this PR).

@stof

stof Jul 23, 2013

Member

I think typehinting the object arguments is better (and SensioLabs Insight agrees with me as it checks for it). So I think the new code should use the typehint (changing the existing code is probably outside the scope of this PR).

This comment has been minimized.

@fabpot

fabpot Jul 23, 2013

Member

Type hinting is indeed a good idea.

@fabpot

fabpot Jul 23, 2013

Member

Type hinting is indeed a good idea.

@@ -181,4 +182,20 @@ public function testOverrideServiceWhenUsingADumpedContainerAndServiceIsUsedFrom
$this->assertSame($bar, $container->get('foo')->bar, '->set() overrides an already defined service');
}
+
+ public function testSynchronizedServiceUpdatesDependencies() {

This comment has been minimized.

@stof

stof Jul 23, 2013

Member

the curly brace should be on its own line

@stof

stof Jul 23, 2013

Member

the curly brace should be on its own line

@@ -11,6 +11,7 @@
namespace Symfony\Component\DependencyInjection\Tests\Dumper;
+use Symfony\Component\DependencyInjection\Container;

This comment has been minimized.

@stof

stof Jul 23, 2013

Member

is it used ?

@stof

stof Jul 23, 2013

Member

is it used ?

+ ->setScope('request')
+;
+$container
+ ->register('dependsOnSynchronized', 'FooClass')

This comment has been minimized.

@stof

stof Jul 23, 2013

Member

Please use underscored names instead of camel cased names to follow the DIC conventions (camelCase does nto really make sense for case insensitive ids)

@stof

stof Jul 23, 2013

Member

Please use underscored names instead of camel cased names to follow the DIC conventions (camelCase does nto really make sense for case insensitive ids)

+ ->register('synchronized_service', 'BarClass')
+ ->setSynchronized(true)
+ ->setScope('request')
+;

This comment has been minimized.

@fabpot

fabpot Sep 27, 2013

Member

If a service is synchronized, it should not be in the request scope. A service should be either in the request scope or synchrnonized, not both.

@fabpot

fabpot Sep 27, 2013

Member

If a service is synchronized, it should not be in the request scope. A service should be either in the request scope or synchrnonized, not both.

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Dec 16, 2013

Member

Closing this one as there is no feedback and because there is a better way in Symfony 2.4 with the request stack service.

Member

fabpot commented Dec 16, 2013

Closing this one as there is no feedback and because there is a better way in Symfony 2.4 with the request stack service.

@fabpot fabpot closed this Dec 16, 2013

@jaypea jaypea deleted the jaypea:synchronize branch Dec 17, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment