[DI] Add ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE #24033

nicolas-grekas · 2017-08-30T07:42:34Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

As spotted in #23984 and #18244, supporting php-pm and the likes requires us to have a way to reset services, typically on kernel.terminate. Yet, only actually instantiated services need to be resetted.

Knowing if a service is initialized or not is doable for public ones thanks to Container::initialized(). But for private ones, there is no way to achieve it.

I propose to add a new way to reference services via this new const. When a reference has this behavior, two things happens:

at runtime, if the referenced service is not initialized, it behave the same as "ignore on invalid", pretending the service doesn't exist
at compile time, such references are weak, meaning their referenced services can be removed, but cannot be inlined

stof · 2017-08-30T10:37:12Z

I think this is useful. It makes sense in several cases to iterate over initialized services for cleanup tasks. Having this kind of reference would allow to use IteratorArgument and private services instead of having to use public services and a list of ids as today.

nicolas-grekas · 2017-08-30T19:35:19Z

Implementation pushed, not yet tested.

nicolas-grekas · 2017-08-31T09:08:17Z

PR is now ready, with pretty good test coverage.
(failures are false positives)

Simperfit

Just some minor comments

Simperfit · 2017-08-31T09:10:29Z

src/Symfony/Component/DependencyInjection/Container.php

@@ -304,9 +305,9 @@ public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE

            try {
                if (isset($this->fileMap[$id])) {
-                    return $this->load($this->fileMap[$id]);
+                    return self::IGNORE_ON_UNINITIALIZED_REFERENCE === $invalidBehavior ? null : $this->load($this->fileMap[$id]);


can't the value self::IGNORE_ON_UNINITIALIZED_REFERENCE === $invalidBehavior be stored in a variable ?

it can, but it's better as is to me (wouldn't help readability, and the extra var is just overhead, so that's two reasons)

Simperfit · 2017-08-31T09:13:45Z

src/Symfony/Component/DependencyInjection/Dumper/YamlDumper.php

@@ -304,8 +304,12 @@ private function dumpValue($value)
     */
    private function getServiceCall($id, Reference $reference = null)
    {
-        if (null !== $reference && ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE !== $reference->getInvalidBehavior()) {
-            return sprintf('@?%s', $id);
+        if (null !== $reference) {


can't we do ?

if (null === $reference) { return sprintf('@%s', $id); } switch...

that would force duplicating the return sprintf('@%s', $id); line.

stof · 2017-08-31T09:22:30Z

...fony/Component/DependencyInjection/Compiler/CheckExceptionOnInvalidReferenceBehaviorPass.php

+            $destId = (string) $value;
+
+            if ($this->container->has($destId) && !$this->container->getDefinition($destId)->isShared()) {
+                throw new InvalidArgumentException(sprintf('Invalid ignore-on-uninitialized reference found in service "%s": target service "%s" is non-shared.', $this->currentId, $destId));


I would say is not shared

stof · 2017-08-31T09:26:20Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

        $conditions = array();
-        foreach ($services as $service) {
+        foreach (ContainerBuilder::getInitializedConditionals($value) as $service) {
+            $conditions[] = sprintf("isset(\$this->services['%s'])", $service);


this needs to be updated based on service visibility when merging into master, right ? Does you testing covers this case for both private and public services ?

done - and allowed me to spot an issue: now, the service graph is aware of those "weak" edges

stof · 2017-08-31T09:30:09Z

src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/container_uninitialized_ref.php

+        };
+        $instance->iter = new RewindableGenerator(function () {
+            if (isset($this->services['foo'])) {
+                yield 'foo' => ${($_ = isset($this->services['foo']) ? $this->services['foo'] : null) && false ?: '_'};


can't we optimize this, as it repeats the wrapping condition ? Or is it not worth it ?

would add complexity, not worth it imho

stof · 2017-08-31T12:05:59Z

...fony/Component/DependencyInjection/Compiler/CheckExceptionOnInvalidReferenceBehaviorPass.php

+        if (ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE === $value->getInvalidBehavior() && !$this->container->has($id = (string) $value)) {
+            throw new ServiceNotFoundException($id, $this->currentId);
+        }
+        if (ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE === $value->getInvalidBehavior() && $this->container->has($id = (string) $value) && !$this->container->getDefinition($id)->isShared()) {


should be findDefinition, to be compatible with references pointing to an alias

stof · 2017-08-31T12:09:24Z

src/Symfony/Component/DependencyInjection/Compiler/ServiceReferenceGraph.php

     */
-    public function connect($sourceId, $sourceValue, $destId, $destValue = null, $reference = null/*, bool $lazy = false*/)
+    public function connect($sourceId, $sourceValue, $destId, $destValue = null, $reference = null/*, bool $lazy = false, bool $weak = false*/)


should we tag this class as @final to allow us to avoid having to do such magic for future changes to the container ? I'm not sure extending the ServiceReferenceGraph actually makes sense.

Tagging it as @internal would be even more strict, but tools wanting to analyze the DIC to display debugging info may be a valid use case for using this class so I would not go that far (for instance, https://github.com/schmittjoh/JMSDebuggingBundle relies on it, even though I'm not sure it plays well with recent versions as I haven't used this bundle since years)

I agree, @final it is now

stof · 2017-08-31T12:13:32Z

src/Symfony/Component/DependencyInjection/Compiler/ServiceReferenceGraphEdge.php

@@ -78,4 +81,15 @@ public function isLazy()
    {
        return $this->lazy;
    }
+
+


you have an extra line here. There should be only 1 empty line 😄 (see fabbot)

stof · 2017-08-31T12:16:39Z

src/Symfony/Component/DependencyInjection/Container.php

@@ -43,6 +43,7 @@
 *  * NULL_ON_INVALID_REFERENCE:      Returns null
 *  * IGNORE_ON_INVALID_REFERENCE:    Ignores the wrapping command asking for the reference
 *                                    (for instance, ignore a setter if the service does not exist)
+ *  * IGNORE_ON_UNINITIALIZED_REFERENCE: Ignores/returns null for uninitialized services


I would say for uninitialized services or invalid references, to be clear that it also includes the behavior of IGNORE_ON_INVALID_REFERENCE.

Btw, the phpdoc looks weird now, as the sentence before the bullet list says it impacts only cases where the service does not exist, which is not the case anymore. You may want to reword it a bit (btw, we should also rework the part about factory methods, as this talks about the deprecated API)

docblock fixed

stof · 2017-08-31T13:07:10Z

src/Symfony/Component/DependencyInjection/Container.php

- * </ul>
- *
- * The container can have three possible behaviors when a service does not exist:
+ * The container can have three possible behaviors when a service


There are 4 behaviors now

nicolas-grekas · 2017-08-31T21:27:10Z

fabbot patch applied

fabpot · 2017-08-31T21:53:56Z

Thank you @nicolas-grekas.

…EFERENCE (nicolas-grekas) This PR was merged into the 3.4 branch. Discussion ---------- [DI] Add ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - As spotted in #23984 and #18244, supporting php-pm and the likes requires us to have a way to reset services, typically on `kernel.terminate`. Yet, only actually instantiated services need to be resetted. Knowing if a service is initialized or not is doable for public ones thanks to `Container::initialized()`. But for private ones, there is no way to achieve it. I propose to add a new way to reference services via this new const. When a reference has this behavior, two things happens: - at runtime, if the referenced service is not initialized, it behave the same as "ignore on invalid", pretending the service doesn't exist - at compile time, such references are weak, meaning their referenced services *can* be removed, but *cannot* be inlined Commits ------- 0db3358 [DI] Add ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE

…e services (derrabus) This PR was merged into the 3.4 branch. Discussion ---------- [FrameworkBundle][HttpKernel] Add DI tag for resettable services | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23984 | License | MIT | Doc PR | TODO This PR uses #24033 to introduce a DI tag for resettable services. TODO after merge: * Add an interface, make the "method" attribute optional and enable autoconfiguration. * Consider adding a config option to enable/disable this feature. * Configure leaking services of the core bundles as resettable. Commits ------- d9a6b76 A DI tag for resettable services.

This PR was merged into the 3.4 branch. Discussion ---------- [DI] Remove unreachable code | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | #24033 added the ability to ignore uninitialized references, but the regex above the conditional would lead to an `InvalidArgumentException` being thrown. Commits ------- ced0857 Remove unreachable code

nicolas-grekas added this to the 3.4 milestone Aug 30, 2017

carsonbot added Status: Needs Review DependencyInjection Feature labels Aug 30, 2017

nicolas-grekas mentioned this pull request Aug 30, 2017

[DX] Promote phppm and add support for resettable services #23984

Closed

nicolas-grekas force-pushed the di-ref-weak branch from dacecf9 to 3506f34 Compare August 30, 2017 18:19

nicolas-grekas force-pushed the di-ref-weak branch 5 times, most recently from 5b34f42 to 0908ea6 Compare August 31, 2017 09:07

Simperfit approved these changes Aug 31, 2017

View reviewed changes

stof reviewed Aug 31, 2017

View reviewed changes

nicolas-grekas force-pushed the di-ref-weak branch 5 times, most recently from 5ad4e57 to 01137d4 Compare August 31, 2017 12:16

stof requested changes Aug 31, 2017

View reviewed changes

nicolas-grekas force-pushed the di-ref-weak branch 3 times, most recently from 5c0da4a to 474fb82 Compare August 31, 2017 12:44

stof requested changes Aug 31, 2017

View reviewed changes

nicolas-grekas force-pushed the di-ref-weak branch 2 times, most recently from fa97a16 to b584db1 Compare August 31, 2017 13:42

stof approved these changes Aug 31, 2017

View reviewed changes

dunglas approved these changes Aug 31, 2017

View reviewed changes

ogizanagi approved these changes Aug 31, 2017

View reviewed changes

[DI] Add ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE

0db3358

nicolas-grekas force-pushed the di-ref-weak branch from b584db1 to 0db3358 Compare August 31, 2017 21:26

fabpot approved these changes Aug 31, 2017

View reviewed changes

fabpot merged commit 0db3358 into symfony:3.4 Aug 31, 2017

nicolas-grekas deleted the di-ref-weak branch September 1, 2017 06:57

javiereguiluz mentioned this pull request Sep 6, 2017

[DI] Add ContainerInterface :: IGNORE_ON_UNINITIALIZED_REFERENCE symfony/symfony-docs#8360

Closed

derrabus mentioned this pull request Sep 11, 2017

[FrameworkBundle][HttpKernel] Add DI tag for resettable services #24155

Merged

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

GwendolenLynch mentioned this pull request Nov 26, 2017

[DI] Remove unreachable code #25158

Merged

nicolas-grekas mentioned this pull request Jul 1, 2018

[DI] ContainerInterface lacks documentation regarding behavior constants symfony/symfony-docs#8759

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI] Add ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE #24033

[DI] Add ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE #24033

nicolas-grekas commented Aug 30, 2017 •

edited

Loading

stof commented Aug 30, 2017

nicolas-grekas commented Aug 30, 2017

nicolas-grekas commented Aug 31, 2017 •

edited

Loading

Simperfit left a comment

Simperfit Aug 31, 2017

nicolas-grekas Aug 31, 2017 •

edited

Loading

Simperfit Aug 31, 2017

Simperfit Aug 31, 2017

nicolas-grekas Aug 31, 2017

stof Aug 31, 2017

nicolas-grekas Aug 31, 2017

stof Aug 31, 2017

nicolas-grekas Aug 31, 2017

stof Aug 31, 2017

nicolas-grekas Aug 31, 2017

stof Aug 31, 2017

nicolas-grekas Aug 31, 2017

stof Aug 31, 2017

nicolas-grekas Aug 31, 2017

stof Aug 31, 2017

stof Aug 31, 2017

nicolas-grekas Aug 31, 2017

stof Aug 31, 2017

nicolas-grekas commented Aug 31, 2017

fabpot commented Aug 31, 2017

[DI] Add ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE #24033

[DI] Add ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE #24033

Conversation

nicolas-grekas commented Aug 30, 2017 • edited Loading

stof commented Aug 30, 2017

nicolas-grekas commented Aug 30, 2017

nicolas-grekas commented Aug 31, 2017 • edited Loading

Simperfit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Aug 31, 2017

fabpot commented Aug 31, 2017

nicolas-grekas commented Aug 30, 2017 •

edited

Loading

nicolas-grekas commented Aug 31, 2017 •

edited

Loading

nicolas-grekas Aug 31, 2017 •

edited

Loading