[DependencyInjection] Container::set() is useless if the entry is an alias #16461

mnapoli · 2015-11-04T14:33:37Z

In my integration tests I want to replace a service by a fake implementation. For example I want to temporarily replace the event_dispatcher by a mock that will store all triggered events:

$container->set('event_dispatcher', $fakeDispatcher);

The problem is that event_dispatcher is an alias, and Container::get() first resolves aliases before trying to find the entry.

That makes Container::set() silently useless when setting an entry that is an alias.

IMO this is a bug, and I see 2 solutions (by order of preference), feel free to suggest alternatives:

set() should erase any existing alias because it should just work
set() should throw an exception if setting an alias

I can implement the solution if one is chosen.

The text was updated successfully, but these errors were encountered:

ro0NL · 2015-11-16T08:28:15Z

Set the actually aliassed key?

mnapoli · 2015-11-16T09:59:09Z

Yes but:

it's silently useless, which is very unhelpful
it leads to this for example (what I use in some tests):

$container->set('event_dispatcher', $dispatcher); // if debug=false
$container->set('debug.event_dispatcher', $dispatcher); // if debug=true

No great… At all.

I don't see a reason why Container::set() should not work in that case.

Taluu · 2015-11-16T13:38:43Z

I think the proper option should be the be able to erase that when building the container (ContainerBuilder) but on the Container it should be forbidden to do so

mnapoli · 2015-11-16T14:34:42Z

Why?

Taluu · 2015-11-16T14:45:17Z

Once the container has been built, it shouldn't be changed so easily IMO (I may be wrong though ; if it is the case, the option of replacing the service is still a good one IMO)

stof · 2015-11-16T15:20:45Z

@Taluu set makes sense at runtime, not during container building (the ContainerBuilder has the method, because you can keep using it as the runtime container when you don't dump an optimized container). during container building, you should be setting definitions

wpeereboom · 2016-02-09T19:12:59Z

@mnapoli Did you find or build a work arround ? I run into the same problems. I suggest the option to set and overwrite the existing alias makes most sense. Let me know if I can be at any help.

…existing aliases `Container::set()` now overrides any previously alias defined with the same name.

mnapoli · 2016-02-09T20:03:59Z

@wpeereboom I just opened a pull request to fix this: #17742

wpeereboom · 2016-02-10T08:03:13Z

@mnapoli Great, thanks

…g aliases `Container::set()` now overrides any previously alias defined with the same name.

…aliases (mnapoli) This PR was submitted for the master branch but it was merged into the 2.3 branch instead (closes #17742). Discussion ---------- [DependencyInjection] Fix #16461 Container::set() replace aliases | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #16461 | License | MIT | Doc PR | - `Container::set()` now overrides any previously alias defined with the same name. Please see #16461 for the background. Example: - given `event_dispatcher` is an alias to `debug.event_dispatcher` - when I run: `$container->set('event_dispatcher', new FakeEventDispatcher)` - *before this patch*: nothing happens - *after this patch*: the `event_dispatcher` is now my fake event dispatcher Commits ------- be85d16 [DependencyInjection] Fix #16461 Let Container::set() replace existing aliases

* 2.3: [DomCrawler] Clarify the value returned by getPhpFiles() [DependencyInjection] Fix #16461 Let Container::set() replace existing aliases Added more exceptions to singularify method

* 2.7: [VarDumper] Fix tests on PHP 7 [DomCrawler] Clarify the value returned by getPhpFiles() [DependencyInjection] Fix #16461 Let Container::set() replace existing aliases avoid (string) catchable fatal error for instances of __PHP_Incomplete_Class remove unnecessary retrieval and setting of data avoid (string) catchable fatal error for __PHP_Incomplete_Class instances sendContent return as parent. [FrameworkBundle] Fix a typo Added more exceptions to singularify method

* 2.8: [VarDumper] Fix tests on PHP 7 [DomCrawler] Clarify the value returned by getPhpFiles() [DependencyInjection] Fix #16461 Let Container::set() replace existing aliases avoid (string) catchable fatal error for instances of __PHP_Incomplete_Class remove unnecessary retrieval and setting of data Update validators.fr.xlf avoid (string) catchable fatal error for __PHP_Incomplete_Class instances sendContent return as parent. [DomCrawler] Remove the overridden getHash() method to prevent problems when cloning the crawler [FrameworkBundle] Fix a typo Added more exceptions to singularify method Add width attribute on SVG [FrameworkBundle] Support autowiring for TranslationInterface [WebProfiler] Fixed styles for search block and menu profiler for IE Edge Conflicts: src/Symfony/Component/DependencyInjection/Tests/ContainerTest.php src/Symfony/Component/DomCrawler/Crawler.php

* 3.0: [VarDumper] Fix tests on PHP 7 [DomCrawler] Clarify the value returned by getPhpFiles() [DependencyInjection] Fix #16461 Let Container::set() replace existing aliases avoid (string) catchable fatal error for instances of __PHP_Incomplete_Class remove unnecessary retrieval and setting of data Update validators.fr.xlf avoid (string) catchable fatal error for __PHP_Incomplete_Class instances sendContent return as parent. [DomCrawler] Remove the overridden getHash() method to prevent problems when cloning the crawler [FrameworkBundle] Fix a typo Added more exceptions to singularify method Add width attribute on SVG [FrameworkBundle] Support autowiring for TranslationInterface [Validator] remove obsolete context and PropertyAccess code [WebProfiler] Fixed styles for search block and menu profiler for IE Edge

In Symfony 2.7.0 - 2.7.9, this line did nothing, since `ezpublish_legacy.kernel` is an alias and `Container::set` never did anything with aliases. In Symfony 2.7.10, setting an alias to a new value basically converts the alias definition to service definition, meaning that `ezpublish_legacy.kernel` will not point to `ezpublish_legacy.kernel.lazy` any more, thus making the alias invalid next time `ezpublish_legacy.kernel.lazy` is rebuilt. See symfony/symfony#16461 for details

Jean85 · 2016-03-16T16:57:55Z

This introduced an error in my case.

When you have an alias of an alias, it overwrites just the first alias, without cascading.
This helped me uncover a bug, and I fixed it setting directly the root service instead of the alias; but I'm not sure if it creates a BC.

Example: Doctrine default EM

$container->set('doctrine.orm.entity_manager', $fake);
$container->get('doctrine')->getManager(); // !== $fake, it uses 'doctrine.orm.default_entity_manager', which aliases to the default connection EM

…ny#16461)

* 2.8: [VarDumper] Fix tests on PHP 7 [DomCrawler] Clarify the value returned by getPhpFiles() [DependencyInjection] Fix symfony#16461 Let Container::set() replace existing aliases avoid (string) catchable fatal error for instances of __PHP_Incomplete_Class remove unnecessary retrieval and setting of data Update validators.fr.xlf avoid (string) catchable fatal error for __PHP_Incomplete_Class instances sendContent return as parent. [DomCrawler] Remove the overridden getHash() method to prevent problems when cloning the crawler [FrameworkBundle] Fix a typo Added more exceptions to singularify method Add width attribute on SVG [FrameworkBundle] Support autowiring for TranslationInterface [WebProfiler] Fixed styles for search block and menu profiler for IE Edge Conflicts: src/Symfony/Component/DependencyInjection/Tests/ContainerTest.php src/Symfony/Component/DomCrawler/Crawler.php

* 3.0: [VarDumper] Fix tests on PHP 7 [DomCrawler] Clarify the value returned by getPhpFiles() [DependencyInjection] Fix symfony#16461 Let Container::set() replace existing aliases avoid (string) catchable fatal error for instances of __PHP_Incomplete_Class remove unnecessary retrieval and setting of data Update validators.fr.xlf avoid (string) catchable fatal error for __PHP_Incomplete_Class instances sendContent return as parent. [DomCrawler] Remove the overridden getHash() method to prevent problems when cloning the crawler [FrameworkBundle] Fix a typo Added more exceptions to singularify method Add width attribute on SVG [FrameworkBundle] Support autowiring for TranslationInterface [Validator] remove obsolete context and PropertyAccess code [WebProfiler] Fixed styles for search block and menu profiler for IE Edge

Taken from ezsystems/LegacyBridge@c5bf6c2 This fixes the broken content preview as described in https://jira.ez.no/browse/EZP-25551 Original comment: In Symfony 2.7.0 - 2.7.9, this line did nothing, since `ezpublish_legacy.kernel` is an alias and `Container::set` never did anything with aliases. In Symfony 2.7.10, setting an alias to a new value basically converts the alias definition to service definition, meaning that `ezpublish_legacy.kernel` will not point to `ezpublish_legacy.kernel.lazy` any more, thus making the alias invalid next time `ezpublish_legacy.kernel.lazy` is rebuilt. See symfony/symfony#16461 for details

aitboudad added the DependencyInjection label Nov 16, 2015

javiereguiluz added the Feature label Feb 6, 2016

mnapoli added a commit to mnapoli/symfony that referenced this issue Feb 9, 2016

[DependencyInjection] Fix symfony#16461 Let Container::set() replace …

2694fe4

…existing aliases `Container::set()` now overrides any previously alias defined with the same name.

mnapoli mentioned this issue Feb 9, 2016

[DependencyInjection] Fix #16461 Container::set() replace aliases #17742

Closed

fabpot pushed a commit that referenced this issue Feb 12, 2016

[DependencyInjection] Fix #16461 Let Container::set() replace existin…

be85d16

…g aliases `Container::set()` now overrides any previously alias defined with the same name.

fabpot closed this as completed Feb 12, 2016

nicolas-grekas added a commit that referenced this issue Feb 13, 2016

Merge branch '2.3' into 2.7

ec7b3f2

* 2.3: [DomCrawler] Clarify the value returned by getPhpFiles() [DependencyInjection] Fix #16461 Let Container::set() replace existing aliases Added more exceptions to singularify method

This was referenced Feb 28, 2016

Release v2.3.38 #17950

Merged

Release v2.7.10 #17951

Merged

Release v2.8.3 #17952

Merged

Release v3.0.3 #17953

Merged

emodric mentioned this issue Mar 1, 2016

EZP-25529: Remove uneeded resetting of ezpublish_legacy.kernel alias ezsystems/LegacyBridge#51

Merged

Jean85 added a commit to facile-it/paraunit-testcase that referenced this issue Mar 16, 2016

Fixing an issue related to a BC from Symfony 3.0.3 (see symfony/symfo…

eb158b1

…ny#16461)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DependencyInjection] Container::set() is useless if the entry is an alias #16461

[DependencyInjection] Container::set() is useless if the entry is an alias #16461

mnapoli commented Nov 4, 2015

ro0NL commented Nov 16, 2015

mnapoli commented Nov 16, 2015

Taluu commented Nov 16, 2015

mnapoli commented Nov 16, 2015

Taluu commented Nov 16, 2015

stof commented Nov 16, 2015

wpeereboom commented Feb 9, 2016

mnapoli commented Feb 9, 2016

wpeereboom commented Feb 10, 2016

Jean85 commented Mar 16, 2016

[DependencyInjection] Container::set() is useless if the entry is an alias #16461

[DependencyInjection] Container::set() is useless if the entry is an alias #16461

Comments

mnapoli commented Nov 4, 2015

ro0NL commented Nov 16, 2015

mnapoli commented Nov 16, 2015

Taluu commented Nov 16, 2015

mnapoli commented Nov 16, 2015

Taluu commented Nov 16, 2015

stof commented Nov 16, 2015

wpeereboom commented Feb 9, 2016

mnapoli commented Feb 9, 2016

wpeereboom commented Feb 10, 2016

Jean85 commented Mar 16, 2016