NotificationsYou must be signed in to change notification settings
Fork9.6k
Star30.4k

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:masterfromnicolas-grekas:di-reflection-resource

Feb 2, 2017

Merged

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

fabpot merged 1 commit intosymfony:masterfromnicolas-grekas:di-reflection-resource

Feb 2, 2017

Conversation

Copy link

Member

nicolas-grekas commentedJan 26, 2017•
edited
Loading

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 viewedignoring whitespaces.

changelog:

addedReflectionClassResource class
added second$exists constructor argument toClassExistenceResource - with a special mode that prevents fatal errors from happening when some parent class is broken (logic generalized from AutowiringPass)
madeClassExistenceResource also work with interfaces and traits
addedContainerBuilder::getReflectionClass() for retrieving and tracking reflection class info
deprecatedContainerBuilder::getClassResource(), useContainerBuilder::getReflectionClass() orContainerBuilder::addObjectResource() instead

nicolas-grekas added this to the3.3 milestone

Jan 26, 2017

carsonbot added Status: Needs Review DependencyInjection Config Feature Deprecation labels

Jan 26, 2017

nicolas-grekas force-pushed thedi-reflection-resource branch 2 times, most recently from79cf689 to769b2c5Compare

January 26, 2017 15:27

stof reviewed

Jan 26, 2017

View reviewed changes

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


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

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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.

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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)

Copy link

MemberAuthor

nicolas-grekasJan 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

src/Symfony/Component/DependencyInjection/ContainerBuilder.php Outdated

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

Copy link

Member

stofJan 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

nicolas-grekasJan 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

stofJan 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

Member

fabpotJan 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@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.

Copy link

MemberAuthor

nicolas-grekasJan 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

done

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",

Copy link

Member

stofJan 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

nicolas-grekasJan 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

good idea, added

dunglas reviewed

Jan 26, 2017

View reviewed changes

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);

Copy link

Member

dunglasJan 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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).

Copy link

MemberAuthor

nicolas-grekasJan 26, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.)

Copy link

Member

dunglas commentedJan 26, 2017

👍

nicolas-grekas force-pushed thedi-reflection-resource branch 8 times, most recently fromdfcd57d tod287508Compare

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 thedi-reflection-resource branch 2 times, most recently from061a58a to86836f9Compare

January 28, 2017 10:14

Copy link

MemberAuthor

nicolas-grekas commentedJan 28, 2017•
edited
Loading

addObjectResource andReflectionClassResource 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 force-pushed thedi-reflection-resource branch 6 times, most recently from7a057f6 to6342a34Compare

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

Copy link

MemberAuthor

nicolas-grekas commentedJan 30, 2017

Rebased
ping@stof :)

nicolas-grekas force-pushed thedi-reflection-resource branch 7 times, most recently fromb267a7f toe251e19Compare

February 1, 2017 14:37

stof requested changes

Feb 1, 2017

View reviewed changes

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

		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)) {

Copy link

Member

stofFeb 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

justif here, as the previous case is throwing

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

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

Copy link

Member

stofFeb 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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

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

Copy link

Member

stofFeb 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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

		yield $class->getDocComment().$class->getModifiers();

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

Copy link

Member

stofFeb 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

shouldn't this be always included ?

Copy link

MemberAuthor

nicolas-grekasFeb 1, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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


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

Copy link

Member

stofFeb 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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

		@@ -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;

Copy link

Member

stofFeb 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

why changing this ?

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

		// 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);

Copy link

Member

stofFeb 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

nicolas-grekasFeb 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

true, removed

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)) {

Copy link

Member

stofFeb 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

src/Symfony/Component/DependencyInjection/ContainerBuilder.php Outdated

		*/
		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);

Copy link

Member

stofFeb 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

typo

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

		}

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

Copy link

Member

stofFeb 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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)

Copy link

MemberAuthor

nicolas-grekasFeb 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

good catch, fixed

nicolas-grekas force-pushed thedi-reflection-resource branch 2 times, most recently from3e78969 to0db525aCompare

February 1, 2017 18:36

Copy link

MemberAuthor

nicolas-grekas commentedFeb 1, 2017

@stof comments addressed

nicolas-grekas force-pushed thedi-reflection-resource branch from0db525a to3e78e70Compare

February 1, 2017 18:37

stof reviewed

Feb 1, 2017

View reviewed changes

src/Symfony/Component/DependencyInjection/ContainerBuilder.php Outdated

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

Copy link

Member

stofFeb 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 ?

Copy link

MemberAuthor

nicolas-grekasFeb 1, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

MemberAuthor

nicolas-grekasFeb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

See#21505 for internal classes

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

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

Copy link

Member

stofFeb 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

nicolas-grekasFeb 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

3.3 only

nicolas-grekas force-pushed thedi-reflection-resource branch from3e78e70 toa8ce243Compare

February 1, 2017 21:53

[DI][Config] Add & use ReflectionClassResource

37e4493

nicolas-grekas force-pushed thedi-reflection-resource branch froma8ce243 to37e4493Compare

February 2, 2017 13:17

nicolas-grekas commented

Feb 2, 2017

View reviewed changes

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

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

Copy link

MemberAuthor

nicolas-grekasFeb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

moved as is above with other resource-related methods

Copy link

Member

fabpot commentedFeb 2, 2017

Thank you@nicolas-grekas.

fabpot merged commit37e4493 intosymfony:master

Feb 2, 2017

fabpot added a commit that referenced this pull request

Feb 2, 2017

feature#21419[DI][Config] Add & use ReflectionClassResource (nicola…

03b7cf7

…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()` insteadCommits-------37e4493 [DI][Config] Add & use ReflectionClassResource

xabbuh reviewed

Feb 3, 2017

View reviewed changes

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",

Copy link

Member

xabbuhFeb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

What is the reason for this change?

Copy link

MemberAuthor

nicolas-grekasFeb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

xabbuhFeb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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 compatibilityrollerworks-graveyard/breadcrumbs-bundle#7

Merged

sstok added a commit to rollerworks-graveyard/breadcrumbs-bundle that referenced this pull request

Feb 4, 2017

bug#7Fix Symfony 4.0 compatibility (sstok)

f773e1f

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#21419Commits-------de72f26 Fix Symfony 4.0 compatibility

javiereguiluz mentioned this pull request

Feb 12, 2017

Added ReflectionClassResourcesymfony/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 added a commit that referenced this pull request

Feb 16, 2017

minor#21631[DI] Don't try to instantiate reflection class if it doe…

c5bdfc5

…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/aIntroduced 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

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 bundleprooph/service-bus-symfony-bundle#23

Merged

Labels

Config DependencyInjection Deprecation Feature Status: Needs Review

6 participants

Movatterモバイル変換

Uh oh!

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

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

Uh oh!

Conversation

nicolas-grekas commentedJan 26, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekasJan 26, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas commentedJan 26, 2017

Uh oh!

nicolas-grekas commentedJan 28, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commentedJan 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekasFeb 1, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commentedFeb 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekasFeb 1, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commentedJan 26, 2017•
edited
Loading

nicolas-grekasJan 26, 2017•
edited
Loading

nicolas-grekas commentedJan 28, 2017•
edited
Loading

nicolas-grekasFeb 1, 2017•
edited
Loading

nicolas-grekasFeb 1, 2017•
edited
Loading