[DI] Only rebuild autowiring cache when actually needed #18144
Next
Next commit
Smart caching system for autowiring to only clear container cache whe…
…n it's *actually* needed
- Loading branch information
commit d0b600aee2c9e43545233d797d0cc7d4d6f95a07
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |
|
|
||
| namespace Symfony\Component\DependencyInjection\Compiler; | ||
|
|
||
| use Symfony\Component\DependencyInjection\Config\AutowireServiceResource; | ||
| use Symfony\Component\DependencyInjection\ContainerBuilder; | ||
| use Symfony\Component\DependencyInjection\Definition; | ||
| use Symfony\Component\DependencyInjection\Exception\RuntimeException; | ||
|
|
@@ -49,6 +50,39 @@ public function process(ContainerBuilder $container) | |
| $this->ambiguousServiceTypes = array(); | ||
| } | ||
|
|
||
| /** | ||
| * Creates a resource to help know if this service has changed. | ||
| * | ||
| * @param \ReflectionClass $reflectionClass | ||
| * | ||
| * @return AutowireServiceResource | ||
| */ | ||
| public static function createResourceForClass(\ReflectionClass $reflectionClass) | ||
| { | ||
| $metadata = array(); | ||
|
|
||
| if ($constructor = $reflectionClass->getConstructor()) { | ||
| $metadata['__construct'] = self::getResourceMetadataForMethod($constructor); | ||
| } | ||
|
|
||
| // todo - when #17608 is merged, could refactor to private function to remove duplication | ||
| // of determining valid "setter" methods | ||
| foreach ($reflectionClass->getMethods(\ReflectionMethod::IS_PUBLIC) as $reflectionMethod) { | ||
| $name = $reflectionMethod->getName(); | ||
| if (isset($methodsCalled[$name]) || $reflectionMethod->isStatic() || 1 !== $reflectionMethod->getNumberOfParameters() || 0 !== strpos($name, 'set')) { | ||
|
There was a problem hiding this comment.
|
||
| continue; | ||
| } | ||
|
|
||
| $metadata[$name] = self::getResourceMetadataForMethod($reflectionMethod); | ||
| } | ||
|
|
||
| return new AutowireServiceResource( | ||
| $reflectionClass->name, | ||
| $reflectionClass->getFileName(), | ||
| $metadata | ||
| ); | ||
|
|
||
| } | ||
|
|
||
| /** | ||
| * Wires the given definition. | ||
| * | ||
|
|
@@ -63,7 +97,7 @@ private function completeDefinition($id, Definition $definition) | |
| return; | ||
| } | ||
|
|
||
| $this->container->addResource(static::createResourceForClass($reflectionClass)); | ||
|
There was a problem hiding this comment. please wrap this call in a |
||
|
|
||
| if (!$constructor = $reflectionClass->getConstructor()) { | ||
| return; | ||
|
|
@@ -278,4 +312,14 @@ private function addServiceToAmbiguousType($id, $type) | |
| } | ||
| $this->ambiguousServiceTypes[$type][] = $id; | ||
| } | ||
|
|
||
| static private function getResourceMetadataForMethod(\ReflectionMethod $method) | ||
|
|
||
| { | ||
|
There was a problem hiding this comment. |
||
| $methodArgumentsMetadata = array(); | ||
| foreach ($method->getParameters() as $parameter) { | ||
| $methodArgumentsMetadata[] = (string) $parameter; | ||
|
There was a problem hiding this comment. IIRC, the argument name doesn't matter (it can be changed safely), only the type hint and the order is important. There was a problem hiding this comment. And casting to a string probably give us a lot of noise like the default value, ... Why not just explicitly use what make sense for us here? There was a problem hiding this comment. @fabpot a change in the default value also affects autowiring when the default value must be used There was a problem hiding this comment. and casting to string makes it much easier to deal with typehints on non-existent classes on PHP 5.x (as PHP 5 does not have There was a problem hiding this comment. I changed to not cast to a string, but I agree with Stof (last committed can be easily reverted) |
||
| } | ||
|
|
||
| return $methodArgumentsMetadata; | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| <?php | ||
|
|
||
| /* | ||
| * This file is part of the Symfony package. | ||
| * | ||
| * (c) Fabien Potencier <fabien@symfony.com> | ||
| * | ||
| * For the full copyright and license information, please view the LICENSE | ||
| * file that was distributed with this source code. | ||
| */ | ||
|
|
||
| namespace Symfony\Component\DependencyInjection\Config; | ||
|
|
||
| use Symfony\Component\Config\Resource\SelfCheckingResourceInterface; | ||
| use Symfony\Component\DependencyInjection\Compiler\AutowirePass; | ||
|
|
||
| class AutowireServiceResource implements SelfCheckingResourceInterface, \Serializable | ||
| { | ||
| private $class; | ||
| private $filePath; | ||
| private $autowiringMetadata = array(); | ||
|
|
||
| public function __construct($class, $path, array $autowiringMetadata) | ||
| { | ||
| $this->class = $class; | ||
| $this->filePath = $path; | ||
| $this->autowiringMetadata = $autowiringMetadata; | ||
| } | ||
|
|
||
| public function isFresh($timestamp) | ||
| { | ||
| if (!file_exists($this->filePath)) { | ||
| return false; | ||
| } | ||
|
|
||
| // has the file *not* been modified? Definitely fresh | ||
| if (@filemtime($this->filePath) <= $timestamp) { | ||
| return true; | ||
| } | ||
|
|
||
| try { | ||
| $reflectionClass = new \ReflectionClass($this->class); | ||
| } catch (\ReflectionException $e) { | ||
| // the class does not exist anymore! | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| $newResource = AutowirePass::createResourceForClass($reflectionClass); | ||
|
There was a problem hiding this comment. |
||
|
|
||
| return $newResource == $this; | ||
|
There was a problem hiding this comment. the intermediate variable can be removed |
||
| } | ||
|
|
||
| public function __toString() | ||
| { | ||
| return 'service.autowire.'.$this->class; | ||
| } | ||
|
|
||
| public function serialize() | ||
| { | ||
| return serialize(array( | ||
| $this->class, | ||
| $this->filePath, | ||
| $this->autowiringMetadata, | ||
| )); | ||
|
|
||
| } | ||
|
|
||
| public function unserialize($serialized) | ||
| { | ||
| list( | ||
| $this->class, | ||
| $this->filePath, | ||
| $this->autowiringMetadata | ||
| ) = unserialize($serialized); | ||
|
|
||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -413,6 +413,39 @@ public function testOptionalScalarArgsNotPassedIfLast() | |
| $definition->getArguments() | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * @dataProvider getCreateResourceTests | ||
| */ | ||
| public function testCreateResourceForClass($className, $isEqual) | ||
| { | ||
| $startingResource = AutowirePass::createResourceForClass( | ||
|
There was a problem hiding this comment. |
||
| new \ReflectionClass(__NAMESPACE__.'\ClassForResource') | ||
| ); | ||
| $newResource = AutowirePass::createResourceForClass( | ||
|
There was a problem hiding this comment. |
||
| new \ReflectionClass(__NAMESPACE__.'\\'.$className) | ||
| ); | ||
|
|
||
| // hack so the objects don't differ by the class name | ||
| $startingReflObject = new \ReflectionObject($startingResource); | ||
| $reflProp = $startingReflObject->getProperty('class'); | ||
| $reflProp->setAccessible(true); | ||
| $reflProp->setValue($startingResource, __NAMESPACE__.'\\'.$className); | ||
|
|
||
| if ($isEqual) { | ||
| $this->assertEquals($startingResource, $newResource); | ||
| } else { | ||
|
There was a problem hiding this comment. |
||
| $this->assertNotEquals($startingResource, $newResource); | ||
| } | ||
| } | ||
|
|
||
| public function getCreateResourceTests() | ||
| { | ||
| return array( | ||
| ['IdenticalClassResource', true], | ||
| ['ClassChangedConstructorArgs', false], | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| class Foo | ||
|
|
@@ -562,3 +595,26 @@ public function __construct(A $a, $foo = 'default_val', Lille $lille) | |
| { | ||
| } | ||
| } | ||
|
|
||
| /* | ||
| * Classes used for testing createResourceForClass | ||
| */ | ||
| class ClassForResource | ||
| { | ||
| public function __construct($foo, Bar $bar = null) | ||
|
There was a problem hiding this comment.
|
||
| { | ||
| } | ||
|
|
||
| public function setBar(Bar $bar) | ||
|
There was a problem hiding this comment.
|
||
| { | ||
| } | ||
| } | ||
| class IdenticalClassResource extends ClassForResource | ||
| { | ||
| } | ||
| class ClassChangedConstructorArgs extends ClassForResource | ||
| { | ||
| public function __construct($foo, Bar $bar, $baz) | ||
| { | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| <?php | ||
|
|
||
| /* | ||
| * This file is part of the Symfony package. | ||
| * | ||
| * (c) Fabien Potencier <fabien@symfony.com> | ||
| * | ||
| * For the full copyright and license information, please view the LICENSE | ||
| * file that was distributed with this source code. | ||
| */ | ||
|
|
||
| namespace Symfony\Component\DependencyInjection\Tests\Config; | ||
|
|
||
| use Symfony\Component\DependencyInjection\Compiler\AutowirePass; | ||
| use Symfony\Component\DependencyInjection\Config\AutowireServiceResource; | ||
|
|
||
| class AutowireServiceResourceTest extends \PHPUnit_Framework_TestCase | ||
| { | ||
| /** | ||
| * @var AutowireServiceResource | ||
| */ | ||
| private $resource; | ||
| private $file; | ||
| private $class; | ||
| private $time; | ||
|
|
||
| protected function setUp() | ||
| { | ||
| $this->file = realpath(sys_get_temp_dir()).'/tmp.php'; | ||
| $this->time = time(); | ||
| touch($this->file, $this->time); | ||
|
|
||
| $this->class = __NAMESPACE__.'\Foo'; | ||
| $this->resource = new AutowireServiceResource( | ||
| $this->class, | ||
| $this->file, | ||
| array() | ||
| ); | ||
| } | ||
|
|
||
| public function testToString() | ||
| { | ||
| $this->assertSame('service.autowire.'.$this->class, (string) $this->resource); | ||
| } | ||
|
|
||
| public function testSerializeUnserialize() | ||
| { | ||
| $unserialized = unserialize(serialize($this->resource)); | ||
|
|
||
| $this->assertEquals($this->resource, $unserialized); | ||
| } | ||
|
|
||
| public function testIsFresh() | ||
| { | ||
| $this->assertTrue($this->resource->isFresh($this->time), '->isFresh() returns true if the resource has not changed in same second'); | ||
| $this->assertTrue($this->resource->isFresh($this->time + 10), '->isFresh() returns true if the resource has not changed'); | ||
| $this->assertFalse($this->resource->isFresh($this->time - 86400), '->isFresh() returns false if the resource has been updated'); | ||
|
There was a problem hiding this comment. Ok it this line, the object is returned instead of false. |
||
| } | ||
|
|
||
| public function testIsFreshForDeletedResources() | ||
| { | ||
| unlink($this->file); | ||
|
|
||
| $this->assertFalse($this->resource->isFresh($this->getStaleFileTime()), '->isFresh() returns false if the resource does not exist'); | ||
| } | ||
|
|
||
| public function testIsNotFreshChangedResource() | ||
| { | ||
| $oldResource = new AutowireServiceResource( | ||
| $this->class, | ||
| $this->file, | ||
| array('will_be_different') | ||
| ); | ||
|
|
||
| // test with a stale file *and* a resource that *will* be different than the actual | ||
| $this->assertFalse($oldResource->isFresh($this->getStaleFileTime()), '->isFresh() returns false if the constructor arguments have changed'); | ||
| } | ||
|
|
||
| public function testIsFreshSameConstructorArgs() | ||
| { | ||
| $oldResource = AutowirePass::createResourceForClass( | ||
|
There was a problem hiding this comment. |
||
| new \ReflectionClass(__NAMESPACE__.'\Foo') | ||
| ); | ||
|
|
||
| // test with a stale file *but* the resource will not be changed | ||
| $this->assertTrue($oldResource->isFresh($this->getStaleFileTime()), '->isFresh() returns false if the constructor arguments have changed'); | ||
| } | ||
|
|
||
| public function testNotFreshIfClassNotFound() | ||
| { | ||
| $resource = new AutowireServiceResource( | ||
| 'Some\Non\Existent\Class', | ||
| $this->file, | ||
| array() | ||
| ); | ||
|
|
||
| $this->assertFalse($resource->isFresh($this->getStaleFileTime()), '->isFresh() returns false if the class no longer exists'); | ||
| } | ||
|
|
||
| protected function tearDown() | ||
| { | ||
| if (!file_exists($this->file)) { | ||
| return; | ||
| } | ||
|
|
||
| unlink($this->file); | ||
| } | ||
|
|
||
| private function getStaleFileTime() | ||
| { | ||
| return $this->time - 10; | ||
| } | ||
| } | ||
|
|
||
| class Foo | ||
| { | ||
| public function __construct($foo) | ||
|
There was a problem hiding this comment.
|
||
| { | ||
| } | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't those static methods be extracted to a Factory class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just seen #18144 (comment)
Perfectly makes sense.