[DI][Config] Add & use ReflectionClassResource #21419

nicolas-grekas · 2017-01-26T15:16:04Z

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

With new changes comming to 3.3, we need a more generic reflection tracking logic than the one already managed by the autowiring subsystem.

This PR adds a new ReflectionClassResource in the Config component, and a new ContainerBuilder::getReflectionClass() method in the DI one (for fetching+tracking reflection-related info).

ReflectionClassResource tracks changes to any public or protected properties/method.

PR updated and ready, best viewed ignoring whitespaces.

changelog:

added ReflectionClassResource class
added second $exists constructor argument to ClassExistenceResource - with a special mode that prevents fatal errors from happening when some parent class is broken (logic generalized from AutowiringPass)
made ClassExistenceResource also work with interfaces and traits
added ContainerBuilder::getReflectionClass() for retrieving and tracking reflection class info
deprecated ContainerBuilder::getClassResource(), use ContainerBuilder::getReflectionClass() or ContainerBuilder::addObjectResource() instead

stof · 2017-01-26T15:21:44Z

src/Symfony/Component/Config/Tests/Resource/ReflectionClassResourceTest.php

+
+        unlink($tmp);
+        $this->assertFalse($res->isFresh($now), '->isFresh() returns false if the resource does not exist');
+    }


is there a way to test the handling of the hash ?

I split the logic to allow for testing signature mutations - tests added

stof · 2017-01-26T15:23:23Z

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

@@ -459,6 +424,9 @@ private function addServiceToAmbiguousType($id, $type)
        $this->ambiguousServiceTypes[$type][] = $id;
    }

+    /**
+     * @deprecated since version 3.3, to be removed in 4.0.


deprecating a private method does not make sense. Either we don't use it anymore, and we should remove it already, or we use it in deprecated method and it will become dead code when removing the other method (which is catched by IDEs and any static analysis tool, for instance SensioLabsInsight)

It's just a friendly reminder for the one who will cleanup the code preparing 4.0

stof · 2017-01-26T15:28:26Z

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

+     *
+     * @param string $class
+     *
+     * @return \ReflectionClass|false


Can we use \ReflectionClass|null instead ? It would be cleaner (PHP 7.1 has nullable types, not falsable types)

would only add boilerplate (isset(false) is used in the code)

Well, if this was a private API, I would be OK with this argument about simplifying the internal implementation.
But this is a public API.

@nicolas-grekas I think we need to try to not use mixed return args in our public APIs (anymore). With \ReflectionClass|null, we could add a native PHP return type in 7.1, but \ReflectionClass|false, we cannot.

stof · 2017-01-26T15:29:29Z

src/Symfony/Component/DependencyInjection/composer.json

@@ -20,7 +20,7 @@
    },
    "require-dev": {
        "symfony/yaml": "~3.2",
-        "symfony/config": "~2.8|~3.0",
+        "symfony/config": "~3.3",


should we add a conflict rule to avoid issues when getting a lower version for people using standalone components ?

good idea, added

dunglas · 2017-01-26T15:30:45Z

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

     */
    public static function createResourceForClass(\ReflectionClass $reflectionClass)
    {
+        @trigger_error('The '.__METHOD__.'() method is deprecated since version 3.3 and will be removed in 4.0. Use ContainerBuilder::getReflectionClass() instead.', E_USER_DEPRECATED);


Not sure if it's a good idea or not. The current implementation will more efficient because some changes doesn't impact autowiring (like properties changes).

In practice, I don't think there are that more situations where the new code would invalidate where the existing wouldn't. And I see the benefit for having only one place where this tracking logic is implemented. So I think it's in fact a desirable thing yes. Allows better aggregation of resources across passes also - and maybe faster checking (with simpler/optimized logic.)

dunglas · 2017-01-26T15:33:15Z

👍

nicolas-grekas · 2017-01-28T10:36:15Z

addObjectResource and ReflectionClassResource now track mtimes of parents, traits and interfaces.

Hash computation, file and class existence is now done lazily - ie after the resources are deduplicated.

And to prevent checking the hash again when it was found valid in a previous run, files are "touched" when needed.

nicolas-grekas · 2017-01-30T23:17:25Z

Rebased
ping @stof :)

stof · 2017-02-01T16:09:50Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/LoggingTranslatorPass.php

-            if (is_subclass_of($class, 'Symfony\Component\Translation\TranslatorInterface') && is_subclass_of($class, 'Symfony\Component\Translation\TranslatorBagInterface')) {
+            if (!$r = $container->getReflectionClass($class)) {
+                throw new InvalidArgumentException(sprintf('Class "%s" used for service "%s" cannot be found.', $class, $translatorAlias));
+            } elseif ($r->isSubclassOf(TranslatorInterface::class) && $r->isSubclassOf(TranslatorBagInterface::class)) {


just if here, as the previous case is throwing

stof · 2017-02-01T16:13:02Z

src/Symfony/Component/Config/Resource/ClassExistenceResource.php

     */
-    public function __construct($resource)
+    public function __construct($resource, $exists = null)


the name is confusing here. It is not a value telling whether the class exists (which is what I thought when seeing the param name, compared to the level), but a checking level

stof · 2017-02-01T16:13:32Z

src/Symfony/Component/Config/Resource/ClassExistenceResource.php

    {
        $this->resource = $resource;
-        $this->exists = class_exists($resource);
+        if (null !== $exists) {
+            $this->exists = (int) $exists;


I would rename the property, as this is not storing whether the class exists (unlike previously)

stof · 2017-02-01T17:11:37Z

src/Symfony/Component/Config/Resource/ReflectionClassResource.php

+        yield $class->getDocComment().$class->getModifiers();
+
+        if ($class->isTrait()) {
+            yield print_r(class_uses($class->name), true);


shouldn't this be always included ?

no need: traits are not directly part of the signature - the methods they provide will be seen as any other methods, thus tracked

stof · 2017-02-01T17:15:42Z

src/Symfony/Component/Config/Tests/Resource/ClassExistenceResourceTest.php

+
+    public function testExistsKo()
+    {
+        spl_autoload_register(function ($class) use (&$loadedClass) { $loadedClass = $class; });


you should unregister it at the end of the test (even when it fails)

stof · 2017-02-01T17:18:07Z

src/Symfony/Component/Console/DependencyInjection/AddConsoleCommandPass.php

@@ -30,13 +32,17 @@ public function process(ContainerBuilder $container)
            $definition = $container->getDefinition($id);

            if ($definition->isAbstract()) {
-                throw new \InvalidArgumentException(sprintf('The service "%s" tagged "console.command" must not be abstract.', $id));
+                continue;


why changing this ?

stof · 2017-02-01T17:20:43Z

src/Symfony/Component/Config/Resource/ReflectionClassResource.php

+                // ensure the next checks won't require a hash computation again,
+                // use a local mtime cache so that the current process still sees
+                // the original mtime and validates the hash for child classes
+                @touch($file, $timestamp);


this might break other systems relying on the real mtime of the file because they are concerned about more things than the ones involved in the Symfony hash

true, removed

stof · 2017-02-01T17:23:47Z

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

@@ -251,7 +253,32 @@ public function setResources(array $resources)
    public function addObjectResource($object)
    {
        if ($this->trackResources) {
-            $this->addClassResource(new \ReflectionClass($object));
+            if (is_object($object)) {


if it also accepts class names, the phpdoc must be updated

stof · 2017-02-01T17:23:59Z

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

     */
    public function addClassResource(\ReflectionClass $class)
    {
-        if (!$this->trackResources) {
-            return $this;
+        @trigger_error('The '.__METHOD__.'() mehtod is deprecated since version 3.3 and will be removed in 4.0. Use the addObjectResource() or the getReflectionClass() method instead.', E_USER_DEPRECATED);


stof · 2017-02-01T17:27:51Z

src/Symfony/Component/EventDispatcher/DependencyInjection/RegisterListenersPass.php

-                }
-
-                throw new InvalidArgumentException(sprintf('Service "%s" must implement interface "%s".', $id, $interface));
+            if (!$r = $container->getReflectionClass($class)) {


We actually need to invalidate the cache when the implementation of the class changes here, as it can change the list of events (btw, it means we are missing a ClassResource today)

good catch, fixed

nicolas-grekas · 2017-02-01T18:36:17Z

@stof comments addressed

stof · 2017-02-01T18:59:54Z

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

+        if ($this->trackResources) {
+            if (!$classReflector) {
+                $this->addResource($resource ?: new ClassExistenceResource($class, ClassExistenceResource::EXISTS_KO));
+            } elseif (!$classReflector->isInternal()) {


why is there this check. Should we replace it with a ClassExistenceResource at least for internal classes if there is no way to check for details ?
and if the hash computation does not work for internal classes, what happens for classes extending an internal one ?

We can track details for internal classes, but this makes no sense: if you switch your version/extension of PHP, the container is not invalidated. Tracking here would mean checking the signature every single time.

See #21505 for internal classes

stof · 2017-02-01T19:03:51Z

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

-        foreach ($this->classResources as $r) {
-            $this->container->addClassResource($r);
-        }
-        $this->classResources = array();


do we actually have this duplicate code in older branches, or was it all added in 3.3 only ?

nicolas-grekas · 2017-02-02T13:18:38Z

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

-     *
-     * @final
-     */
-    public function fileExists($path, $trackContents = true)


moved as is above with other resource-related methods

fabpot · 2017-02-02T18:29:04Z

Thank you @nicolas-grekas.

…s-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI][Config] Add & use ReflectionClassResource | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #21079 | License | MIT | Doc PR | - With new changes comming to 3.3, we need a more generic reflection tracking logic than the one already managed by the autowiring subsystem. This PR adds a new ReflectionClassResource in the Config component, and a new ContainerBuilder::getReflectionClass() method in the DI one (for fetching+tracking reflection-related info). ReflectionClassResource tracks changes to any public or protected properties/method. PR updated and ready, best viewed [ignoring whitespaces](https://github.com/symfony/symfony/pull/21419/files?w=1). changelog: * added `ReflectionClassResource` class * added second `$exists` constructor argument to `ClassExistenceResource` - with a special mode that prevents fatal errors from happening when some parent class is broken (logic generalized from AutowiringPass) * made `ClassExistenceResource` also work with interfaces and traits * added `ContainerBuilder::getReflectionClass()` for retrieving and tracking reflection class info * deprecated `ContainerBuilder::getClassResource()`, use `ContainerBuilder::getReflectionClass()` or `ContainerBuilder::addObjectResource()` instead Commits ------- 37e4493 [DI][Config] Add & use ReflectionClassResource

xabbuh · 2017-02-03T06:42:24Z

src/Symfony/Bundle/FrameworkBundle/composer.json

@@ -22,7 +22,7 @@
        "symfony/dependency-injection": "~3.3",
        "symfony/config": "~3.3",
        "symfony/event-dispatcher": "~3.3",
-        "symfony/http-foundation": "~3.1",
+        "symfony/http-foundation": "~3.3",


What is the reason for this change?

Transitivity and composer bug, this doesn't change the result, it only helps composer

thanks, I was just wondering as the code changes didn't indicate any need for this change

This PR was merged into the 1.0-dev branch. Discussion ---------- | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT `ContainerBuilder::addClassResource()` is deprecated. symfony/symfony#21419 Commits ------- de72f26 Fix Symfony 4.0 compatibility

…sn't exist (chalasr) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] Don't try to instantiate reflection class if it doesn't exist | Q | A | ------------- | --- | Branch? | master | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Introduced in #21419 so master only. It breaks on bundles that ~~do not use the convention for naming their `Configuration`~~ do not have configuration, e.g. SecurityBundle's FirewallEntryPointExtension for which tests are actually broken (see travis). Commits ------- f9b917a [DI] Don't instantiate unexisting reflection class

nicolas-grekas added this to the 3.3 milestone Jan 26, 2017

carsonbot added Status: Needs Review DependencyInjection Config Feature Deprecation labels Jan 26, 2017

nicolas-grekas force-pushed the di-reflection-resource branch 2 times, most recently from 79cf689 to 769b2c5 Compare January 26, 2017 15:27

stof reviewed Jan 26, 2017

View reviewed changes

dunglas reviewed Jan 26, 2017

View reviewed changes

nicolas-grekas force-pushed the di-reflection-resource branch 8 times, most recently from dfcd57d to d287508 Compare January 27, 2017 17:38

nicolas-grekas mentioned this pull request Jan 28, 2017

[DI] Add getter injection #20973

Merged

5 tasks

nicolas-grekas force-pushed the di-reflection-resource branch 2 times, most recently from 061a58a to 86836f9 Compare January 28, 2017 10:14

nicolas-grekas force-pushed the di-reflection-resource branch 6 times, most recently from 7a057f6 to 6342a34 Compare January 29, 2017 15:01

nicolas-grekas mentioned this pull request Jan 29, 2017

[DI][Config] Add ContainerBuilder::classExists() for checking and tracking class, interface or trait existence #21452

Closed

nicolas-grekas force-pushed the di-reflection-resource branch 7 times, most recently from b267a7f to e251e19 Compare February 1, 2017 14:37

stof requested changes Feb 1, 2017

View reviewed changes

nicolas-grekas force-pushed the di-reflection-resource branch 2 times, most recently from 3e78969 to 0db525a Compare February 1, 2017 18:36

nicolas-grekas force-pushed the di-reflection-resource branch from 0db525a to 3e78e70 Compare February 1, 2017 18:37

stof reviewed Feb 1, 2017

View reviewed changes

nicolas-grekas force-pushed the di-reflection-resource branch from 3e78e70 to a8ce243 Compare February 1, 2017 21:53

[DI][Config] Add & use ReflectionClassResource

37e4493

nicolas-grekas force-pushed the di-reflection-resource branch from a8ce243 to 37e4493 Compare February 2, 2017 13:17

nicolas-grekas commented Feb 2, 2017

View reviewed changes

fabpot merged commit 37e4493 into symfony:master Feb 2, 2017

xabbuh reviewed Feb 3, 2017

View reviewed changes

nicolas-grekas deleted the di-reflection-resource branch February 3, 2017 12:23

sstok mentioned this pull request Feb 4, 2017

Fix Symfony 4.0 compatibility rollerworks-graveyard/breadcrumbs-bundle#7

Merged

javiereguiluz mentioned this pull request Feb 12, 2017

Added ReflectionClassResource symfony/symfony-docs#7471

Closed

chalasr mentioned this pull request Feb 16, 2017

[DI] Don't try to instantiate reflection class if it doesn't exist #21631

Merged

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

sstok mentioned this pull request Jun 21, 2017

Refactor service bus bundle prooph/service-bus-symfony-bundle#23

Merged

[DI][Config] Add & use ReflectionClassResource #21419

[DI][Config] Add & use ReflectionClassResource #21419

Conversation

nicolas-grekas commented Jan 26, 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

nicolas-grekas Jan 26, 2017 • edited Loading

Choose a reason for hiding this comment

dunglas commented Jan 26, 2017

nicolas-grekas commented Jan 28, 2017 • edited Loading

nicolas-grekas commented Jan 30, 2017

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 Feb 1, 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

nicolas-grekas commented Feb 1, 2017

Choose a reason for hiding this comment

nicolas-grekas Feb 1, 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

fabpot commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Jan 26, 2017 •

edited

Loading

nicolas-grekas Jan 26, 2017 •

edited

Loading

nicolas-grekas commented Jan 28, 2017 •

edited

Loading

nicolas-grekas Feb 1, 2017 •

edited

Loading

nicolas-grekas Feb 1, 2017 •

edited

Loading