[DI] Throw when a service name or an alias contains dynamic values (prevent an infinite loop) #24673
Conversation
11afeff to
1cdb2bc
Compare
|
Why the hell would |
| $this->set($id, new ServiceLocator(array())); | ||
| // We don't call set() to prevent an infinite loop: | ||
| // set() calls getRemovedIds(), getRemovedIds() calls getEnv() in some cases | ||
| $this->services[$id] = new ServiceLocator(array()); |
There was a problem hiding this comment.
assigning directly is not good, as it bypasses a bunch of validation and processing (you don't normalize the id for instance)
There was a problem hiding this comment.
Here it's a very special case. The id is known and is already normalized and validated. We may add a third parameter to set to prevent the circular env resolution, but it looks overkill for this special case.
Here is an extract of the return array(
// ...
'api_platform.http_cache.purger.varnish' => true,
'api_platform.http_cache.purger.varnish_client' => true,
'api_platform.http_cache.purger.varnish_client.'.$this->getEnv('string:VARNISH_URL') => true,
// ...
);It looks legit to me or the error generation cannot work properly: if (isset($this->getRemovedIds()[$id])) {
throw new ServiceNotFoundException($id, null, null, array(), sprintf('The "%s" service or alias has been removed or inlined when the container was compiled. You should either make it public, or stop using the container directly and use dependency injection instead.', $id));
} |
|
@stof is right: service ids cannot be dynamic, this is unsupported and should throw instead. |
|
Ok got it, I'll change the patch to throw |
|
Status: Needs Review |
| public function testDynamicServiceName() | ||
| { | ||
| $container = new ContainerBuilder(); | ||
| $env = ($container->getParameterBag()->get('env(BAR)')); |
|
Broken tests are unrelated, rebasing will make this green. Fabbot complains though |
|
Do we need to do the check for aliases also? |
59a400e to
e4218a9
Compare
|
@nicolas-grekas done |
|
Also the commit+PR title should be updated |
…revent an infinite loop)
37bd70b to
51e16bb
Compare
|
Errors not related |
b456b2b to
949c6da
Compare
949c6da to
0034cc1
Compare
|
Thank you @dunglas. |
…c values (prevent an infinite loop) (dunglas) This PR was squashed before being merged into the 3.3 branch (closes #24673). Discussion ---------- [DI] Throw when a service name or an alias contains dynamic values (prevent an infinite loop) | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files --> | BC breaks? | no | Deprecations? | no <!-- don't forget to update UPGRADE-*.md files --> | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a If an environment variable is used to build a service name (like in [this snippet](https://github.com/api-platform/core/blame/4b3d1abfe595d507c9064ef86fd8ef6881b7e5b5/src/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php#L471)), an infinite loop occurs. It's common to build dynamic service names (in a compiler pass), if the dynamic part comes from a parameter, this bug can occurs. Commits ------- 14e3085 [DI] Throw when a service name or an alias contains dynamic values (prevent an infinite loop)
If an environment variable is used to build a service name (like in this snippet), an infinite loop occurs.
It's common to build dynamic service names (in a compiler pass), if the dynamic part comes from a parameter, this bug can occurs.