Skip to content

Commit eee1a44

Browse files
fabpotfabpot
committed
feature #20113 Use the method map as authoritative list of factories for dumped containers (stof)
This PR was merged into the 3.2-dev branch. Discussion ---------- Use the method map as authoritative list of factories for dumped containers | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | only for weird broken cases | Deprecations? | yes (but only for people doing weird things) | Tests pass? | yes | Fixed tickets | #11761, #19690 | License | MIT | Doc PR | n/a The initial implementation of the method factory discovery was based on a naming convention for factory methods. However, this naming convention allowed to generate the same name for multiple ids. In the meantime, a method map was introduced to solve this issue (and others). When accessing a service with a different id than the official one (thanks to ambiguities), this breaks the sharing of the service, as it creates a new instance each time and replaces the existing shared instance. This was also inconsistent between a dumped container (affected by this) and a non-dumped container (reporting a service not found error for the other id). The method map is now the authoritative way to discover available service factories. When the dumped container was generated with a method map (which is the case when using the dumper shipped in the component), the logic based on defined methods is not executed anymore. This forbids using another id than the real one to access the service (preventing to trigger the broken behavior). So this breaks BC for people being lucky (i.e. they were using the broken id only once and *before* any usage of the official id) and fixes a WTF bug for all others. When using a dumper which does not fill the method map, the old logic is still applied, but deprecation warnings are triggered on access to dumped services. Currently, this will trigger a deprecation warning for each new service instantiation. I have not found an easy way to trigger it only once (except adding a private property to remember we already triggered it, but is it worth it ?), but only people writing a project container by hand or writing their own dumper would ever see such deprecation anyway (as the core dumper generates the method map). Additionally, this makes ``getServiceIds`` faster by avoiding doing a regex match for each method in the class. Commits ------- 03b9108 Use the method map as authoritative list of factories for dumped containers
2 parents 451cdac + 03b9108 commit eee1a44

File tree

4 files changed

+190
-13
lines changed

4 files changed

+190
-13
lines changed

‎src/Symfony/Bridge/ProxyManager/Tests/LazyProxy/Fixtures/php/lazy_service.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,10 @@
1717
*/
1818
class LazyServiceProjectServiceContainer extends Container
1919
{
20+
protected $methodMap = array(
21+
'foo' => 'getFooService',
22+
);
23+
2024
/**
2125
* Constructor.
2226
*/

‎src/Symfony/Bridge/ProxyManager/Tests/LazyProxy/Fixtures/php/lazy_service_with_hints.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,10 @@
1717
*/
1818
class LazyServiceProjectServiceContainer extends Container
1919
{
20+
protected $methodMap = array(
21+
'foo' => 'getFooService',
22+
);
23+
2024
/**
2125
* Constructor.
2226
*/

‎src/Symfony/Component/DependencyInjection/Container.php‎

Lines changed: 34 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -206,15 +206,29 @@ public function has($id)
206206
) {
207207
return true;
208208
}
209+
210+
if (isset($this->privates[$id])) {
211+
@trigger_error(sprintf('Checking for the existence of the "%s" private service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0.', $id), E_USER_DEPRECATED);
212+
}
213+
214+
if (isset($this->methodMap[$id])) {
215+
return true;
216+
}
217+
209218
if (--$i && $id !== $lcId = strtolower($id)) {
210219
$id = $lcId;
211-
} else {
212-
if (isset($this->privates[$id])) {
213-
@trigger_error(sprintf('Checking for the existence of the "%s" private service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0.', $id), E_USER_DEPRECATED);
214-
}
220+
continue;
221+
}
222+
223+
// We only check the convention-based factory in a compiled container (i.e. a child class other than a ContainerBuilder,
224+
// and only when the dumper has not generated the method map (otherwise the method map is considered to be fully populated by the dumper)
225+
if (!$this->methodMap && !$this instanceof ContainerBuilder && __CLASS__ !== static::class && method_exists($this, 'get'.strtr($id, $this->underscoreMap).'Service')) {
226+
@trigger_error('Generating a dumped container without populating the method map is deprecated since 3.2 and will be unsupported in 4.0. Update your dumper to generate the method map.', E_USER_DEPRECATED);
215227

216-
return method_exists($this, 'get'.strtr($id, $this->underscoreMap).'Service');
228+
return true;
217229
}
230+
231+
return false;
218232
}
219233
}
220234

@@ -262,7 +276,10 @@ public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE
262276
} elseif (--$i && $id !== $lcId = strtolower($id)) {
263277
$id = $lcId;
264278
continue;
265-
} elseif (method_exists($this, $method = 'get'.strtr($id, $this->underscoreMap).'Service')) {
279+
} elseif (!$this->methodMap && !$this instanceof ContainerBuilder && __CLASS__ !== static::class && method_exists($this, $method = 'get'.strtr($id, $this->underscoreMap).'Service')) {
280+
// We only check the convention-based factory in a compiled container (i.e. a child class other than a ContainerBuilder,
281+
// and only when the dumper has not generated the method map (otherwise the method map is considered to be fully populated by the dumper)
282+
@trigger_error('Generating a dumped container without populating the method map is deprecated since 3.2 and will be unsupported in 4.0. Update your dumper to generate the method map.', E_USER_DEPRECATED);
266283
// $method is set to the right value, proceed
267284
} else {
268285
if (self::EXCEPTION_ON_INVALID_REFERENCE === $invalidBehavior) {
@@ -341,14 +358,21 @@ public function reset()
341358
public function getServiceIds()
342359
{
343360
$ids = array();
344-
foreach (get_class_methods($this) as $method) {
345-
if (preg_match('/^get(.+)Service$/', $method, $match)) {
346-
$ids[] = self::underscore($match[1]);
361+
362+
if (!$this->methodMap && !$this instanceof ContainerBuilder && __CLASS__ !== static::class) {
363+
// We only check the convention-based factory in a compiled container (i.e. a child class other than a ContainerBuilder,
364+
// and only when the dumper has not generated the method map (otherwise the method map is considered to be fully populated by the dumper)
365+
@trigger_error('Generating a dumped container without populating the method map is deprecated since 3.2 and will be unsupported in 4.0. Update your dumper to generate the method map.', E_USER_DEPRECATED);
366+
367+
foreach (get_class_methods($this) as $method) {
368+
if (preg_match('/^get(.+)Service$/', $method, $match)) {
369+
$ids[] = self::underscore($match[1]);
370+
}
347371
}
348372
}
349373
$ids[] = 'service_container';
350374

351-
return array_unique(array_merge($ids, array_keys($this->services)));
375+
return array_unique(array_merge($ids, array_keys($this->methodMap), array_keys($this->services)));
352376
}
353377

354378
/**

‎src/Symfony/Component/DependencyInjection/Tests/ContainerTest.php‎

Lines changed: 148 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,25 @@ public function testGetServiceIds()
126126

127127
$sc = new ProjectServiceContainer();
128128
$sc->set('foo', $obj = new \stdClass());
129-
$this->assertEquals(array('internal', 'bar', 'foo_bar', 'foo.baz', 'circular', 'throw_exception', 'throws_exception_on_service_configuration', 'service_container', 'foo'), $sc->getServiceIds(), '->getServiceIds() returns defined service ids by getXXXService() methods, followed by service ids defined by set()');
129+
$this->assertEquals(array('service_container', 'internal', 'bar', 'foo_bar', 'foo.baz', 'circular', 'throw_exception', 'throws_exception_on_service_configuration', 'foo'), $sc->getServiceIds(), '->getServiceIds() returns defined service ids by factory methods in the method map, followed by service ids defined by set()');
130+
}
131+
132+
/**
133+
* @group legacy
134+
* @requires function Symfony\Bridge\PhpUnit\ErrorAssert::assertDeprecationsAreTriggered
135+
*/
136+
public function testGetLegacyServiceIds()
137+
{
138+
$sc = new LegacyProjectServiceContainer();
139+
$sc->set('foo', $obj = new \stdClass());
140+
141+
$deprecations = array(
142+
'Generating a dumped container without populating the method map is deprecated since 3.2 and will be unsupported in 4.0. Update your dumper to generate the method map.',
143+
);
144+
145+
ErrorAssert::assertDeprecationsAreTriggered($deprecations, function () use ($sc) {
146+
$this->assertEquals(array('internal', 'bar', 'foo_bar', 'foo.baz', 'circular', 'throw_exception', 'throws_exception_on_service_configuration', 'service_container', 'foo'), $sc->getServiceIds(), '->getServiceIds() returns defined service ids by getXXXService() methods, followed by service ids defined by set()');
147+
});
130148
}
131149

132150
public function testSet()
@@ -160,7 +178,6 @@ public function testGet()
160178
$this->assertEquals($sc->__bar, $sc->get('bar'), '->get() returns the service for the given id');
161179
$this->assertEquals($sc->__foo_bar, $sc->get('foo_bar'), '->get() returns the service if a get*Method() is defined');
162180
$this->assertEquals($sc->__foo_baz, $sc->get('foo.baz'), '->get() returns the service if a get*Method() is defined');
163-
$this->assertEquals($sc->__foo_baz, $sc->get('foo\\baz'), '->get() returns the service if a get*Method() is defined');
164181

165182
$sc->set('bar', $bar = new \stdClass());
166183
$this->assertEquals($bar, $sc->get('bar'), '->get() prefers to return a service defined with set() than one defined with a getXXXMethod()');
@@ -174,6 +191,43 @@ public function testGet()
174191
$this->assertNull($sc->get('', ContainerInterface::NULL_ON_INVALID_REFERENCE), '->get() returns null if the service is empty');
175192
}
176193

194+
/**
195+
* @group legacy
196+
* @requires function Symfony\Bridge\PhpUnit\ErrorAssert::assertDeprecationsAreTriggered
197+
*/
198+
public function testLegacyGet()
199+
{
200+
$sc = new LegacyProjectServiceContainer();
201+
$sc->set('foo', $foo = new \stdClass());
202+
203+
$deprecations = array(
204+
'Generating a dumped container without populating the method map is deprecated since 3.2 and will be unsupported in 4.0. Update your dumper to generate the method map.',
205+
'Generating a dumped container without populating the method map is deprecated since 3.2 and will be unsupported in 4.0. Update your dumper to generate the method map.',
206+
'Generating a dumped container without populating the method map is deprecated since 3.2 and will be unsupported in 4.0. Update your dumper to generate the method map.',
207+
'Generating a dumped container without populating the method map is deprecated since 3.2 and will be unsupported in 4.0. Update your dumper to generate the method map.',
208+
);
209+
210+
ErrorAssert::assertDeprecationsAreTriggered($deprecations, function () use ($sc, $foo) {
211+
$this->assertEquals($foo, $sc->get('foo'), '->get() returns the service for the given id');
212+
$this->assertEquals($foo, $sc->get('Foo'), '->get() returns the service for the given id, and converts id to lowercase');
213+
$this->assertEquals($sc->__bar, $sc->get('bar'), '->get() returns the service for the given id');
214+
$this->assertEquals($sc->__foo_bar, $sc->get('foo_bar'), '->get() returns the service if a get*Method() is defined');
215+
$this->assertEquals($sc->__foo_baz, $sc->get('foo.baz'), '->get() returns the service if a get*Method() is defined');
216+
$this->assertEquals($sc->__foo_baz, $sc->get('foo\\baz'), '->get() returns the service if a get*Method() is defined');
217+
218+
$sc->set('bar', $bar = new \stdClass());
219+
$this->assertEquals($bar, $sc->get('bar'), '->get() prefers to return a service defined with set() than one defined with a getXXXMethod()');
220+
221+
try {
222+
$sc->get('');
223+
$this->fail('->get() throws a \InvalidArgumentException exception if the service is empty');
224+
} catch (\Exception $e) {
225+
$this->assertInstanceOf('Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException', $e, '->get() throws a ServiceNotFoundException exception if the service is empty');
226+
}
227+
$this->assertNull($sc->get('', ContainerInterface::NULL_ON_INVALID_REFERENCE), '->get() returns null if the service is empty');
228+
});
229+
}
230+
177231
public function testGetThrowServiceNotFoundException()
178232
{
179233
$sc = new ProjectServiceContainer();
@@ -230,7 +284,32 @@ public function testHas()
230284
$this->assertTrue($sc->has('bar'), '->has() returns true if a get*Method() is defined');
231285
$this->assertTrue($sc->has('foo_bar'), '->has() returns true if a get*Method() is defined');
232286
$this->assertTrue($sc->has('foo.baz'), '->has() returns true if a get*Method() is defined');
233-
$this->assertTrue($sc->has('foo\\baz'), '->has() returns true if a get*Method() is defined');
287+
}
288+
289+
/**
290+
* @group legacy
291+
* @requires function Symfony\Bridge\PhpUnit\ErrorAssert::assertDeprecationsAreTriggered
292+
*/
293+
public function testLegacyHas()
294+
{
295+
$sc = new LegacyProjectServiceContainer();
296+
$sc->set('foo', new \stdClass());
297+
298+
$deprecations = array(
299+
'Generating a dumped container without populating the method map is deprecated since 3.2 and will be unsupported in 4.0. Update your dumper to generate the method map.',
300+
'Generating a dumped container without populating the method map is deprecated since 3.2 and will be unsupported in 4.0. Update your dumper to generate the method map.',
301+
'Generating a dumped container without populating the method map is deprecated since 3.2 and will be unsupported in 4.0. Update your dumper to generate the method map.',
302+
'Generating a dumped container without populating the method map is deprecated since 3.2 and will be unsupported in 4.0. Update your dumper to generate the method map.',
303+
);
304+
305+
ErrorAssert::assertDeprecationsAreTriggered($deprecations, function () use ($sc) {
306+
$this->assertFalse($sc->has('foo1'), '->has() returns false if the service does not exist');
307+
$this->assertTrue($sc->has('foo'), '->has() returns true if the service exists');
308+
$this->assertTrue($sc->has('bar'), '->has() returns true if a get*Method() is defined');
309+
$this->assertTrue($sc->has('foo_bar'), '->has() returns true if a get*Method() is defined');
310+
$this->assertTrue($sc->has('foo.baz'), '->has() returns true if a get*Method() is defined');
311+
$this->assertTrue($sc->has('foo\\baz'), '->has() returns true if a get*Method() is defined');
312+
});
234313
}
235314

236315
public function testInitialized()
@@ -385,6 +464,72 @@ public function testRequestAnInternalSharedPrivateServiceIsDeprecated()
385464
}
386465

387466
class ProjectServiceContainer extends Container
467+
{
468+
public $__bar;
469+
public $__foo_bar;
470+
public $__foo_baz;
471+
public $__internal;
472+
protected $methodMap = array(
473+
'internal' => 'getInternalService',
474+
'bar' => 'getBarService',
475+
'foo_bar' => 'getFooBarService',
476+
'foo.baz' => 'getFoo_BazService',
477+
'circular' => 'getCircularService',
478+
'throw_exception' => 'getThrowExceptionService',
479+
'throws_exception_on_service_configuration' => 'getThrowsExceptionOnServiceConfigurationService',
480+
);
481+
482+
public function __construct()
483+
{
484+
parent::__construct();
485+
486+
$this->__bar = new \stdClass();
487+
$this->__foo_bar = new \stdClass();
488+
$this->__foo_baz = new \stdClass();
489+
$this->__internal = new \stdClass();
490+
$this->privates = array('internal' => true);
491+
$this->aliases = array('alias' => 'bar');
492+
}
493+
494+
protected function getInternalService()
495+
{
496+
return $this->__internal;
497+
}
498+
499+
protected function getBarService()
500+
{
501+
return $this->__bar;
502+
}
503+
504+
protected function getFooBarService()
505+
{
506+
return $this->__foo_bar;
507+
}
508+
509+
protected function getFoo_BazService()
510+
{
511+
return $this->__foo_baz;
512+
}
513+
514+
protected function getCircularService()
515+
{
516+
return $this->get('circular');
517+
}
518+
519+
protected function getThrowExceptionService()
520+
{
521+
throw new \Exception('Something went terribly wrong!');
522+
}
523+
524+
protected function getThrowsExceptionOnServiceConfigurationService()
525+
{
526+
$this->services['throws_exception_on_service_configuration'] = $instance = new \stdClass();
527+
528+
throw new \Exception('Something was terribly wrong while trying to configure the service!');
529+
}
530+
}
531+
532+
class LegacyProjectServiceContainer extends Container
388533
{
389534
public $__bar;
390535
public $__foo_bar;

0 commit comments

Comments
 (0)