10000 Massive refactor after comments from Stof to take into account many o… · symfony/symfony@e8598ee · GitHub
[go: up one dir, main page]
Skip to content

Commit e8598ee

Browse files
weaverryanweaverryan
committed
Massive refactor after comments from Stof to take into account many other situations
1 parent 4b521c5 commit e8598ee

File tree

4 files changed

+177
-69
lines changed

4 files changed

+177
-69
lines changed

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

Lines changed: 39 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -60,16 +60,25 @@ public function process(ContainerBuilder $container)
6060
public static function createResourceForClass(\ReflectionClass $reflectionClass)
6161
{
6262
$constructor = $reflectionClass->getConstructor();
63-
$constructorParams = $constructor ? $constructor->getParameters() : array();
64-
$constructorArgumentsForResource = array();
65-
foreach ($constructorParams as $parameter) {
66-
$constructorArgumentsForResource[] = $parameter->getClass();
63+
$constructorMetadata = $constructor ? self::getResourceMetadataForMethod($constructor) : array();
64+
65+
$settersMetadata = array();
66+
// todo - when #17608 is merged, could refactor to private function to remove duplication
67+
// of determining valid "setter" methods
68+
foreach ($reflectionClass->getMethods(\ReflectionMethod::IS_PUBLIC) as $reflectionMethod) {
69+
$name = $reflectionMethod->getName();
70+
if (isset($methodsCalled[$name]) || $reflectionMethod->isStatic() || 1 !== $reflectionMethod->getNumberOfParameters() || 0 !== strpos($name, 'set')) {
71+
continue;
72+
}
73+
74+
$settersMetadata[$name] = self::getResourceMetadataForMethod($reflectionMethod);
6775
}
6876

6977
return new AutowireServiceResource(
7078
$reflectionClass->name,
7179
$reflectionClass->getFileName(),
72-
$constructorArgumentsForResource
80+
$constructorMetadata,
81+
$settersMetadata
7382
);
7483
}
7584

@@ -302,4 +311,29 @@ private function addServiceToAmbiguous 57AE Type($id, $type)
302311
}
303312
$this->ambiguousServiceTypes[$type][] = $id;
304313
}
314+
315+
static private function getResourceMetadataForMethod(\ReflectionMethod $method)
316+
{
317+
$methodArgumentsMetadata = array();
318+
foreach ($method->getParameters() as $parameter) {
319+
try {
320+
$metadata = array(
321+
'class' => $parameter->getClass(),
322+
323+
);
324+
if ($parameter->isDefaultValueAvailable()) {
325+
$metadata['default'] = $parameter->getDeclaringClass();
326+
}
327+
328+
} catch (\ReflectionException $e) {
329+
$metadata = array(
330+
'class' => 'non-existent class'
331+
);
332+
}
333+
334+
$methodArgumentsMetadata[] = $metadata;
335+
}
336+
337+
return $methodArgumentsMetadata;
338+
}
305339
}

‎src/Symfony/Component/DependencyInjection/Config/AutowireServiceResource.php

Lines changed: 17 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -18,23 +18,15 @@ class AutowireServiceResource implements SelfCheckingResourceInterface, \Seriali
1818
{
1919
private $class;
2020
private $filePath;
21-
private $constructorArguments;
22-
private $createResourceCallback;
21+
private $constructorMetadata = array();
22+
private $settersMetadata = array();
2323

24-
public function __construct($class, $path)
24+
public function __construct($class, $path, array $constructorMetadata, array $settersMetadata)
2525
{
2626
$this->class = $class;
2727
$this->filePath = $path;
28-
}
29-
30-
/**
31-
* An array of arguments: 0 => type-hinted class (or null of there is no type-hint).
32-
*
33-
* @param array $constructorArguments
34-
*/
35-
public function setConstructorArguments(array $constructorArguments)
36-
{
37-
$this->constructorArguments = $constructorArguments;
28+
$this->constructorMetadata = $constructorMetadata;
29+
$this->settersMetadata = $settersMetadata;
3830
}
3931

4032
public function isFresh($timestamp)
@@ -48,22 +40,17 @@ public function isFresh($timestamp)
4840
return true;
4941
}
5042

51-
if ($this->createResourceCallback) {
52-
// this override is used for testing
53-
$newResource = call_user_func($this->createResourceCallback, new \ReflectionClass($this->class));
54-
} else {
55-
$newResource = AutowirePass::createResourceForClass(new \ReflectionClass($this->class));
43+
try {
44+
$reflectionClass = new \ReflectionClass($this->class);
45+
} catch (\ReflectionException $e) {
46+
// the class does not exist anymore!
47+
48+
return false;
5649
}
5750

58-
return $newResource == $this;
59-
}
51+
$newResource = AutowirePass::createResourceForClass($reflectionClass);
6052

61-
/**
62-
* @internal
63-
*/
64-
public function setCreateResourceCallback($createResourceCallback)
65-
{
66-
$this->createResourceCallback = $createResourceCallback;
53+
return $newResource == $this;
6754
}
6855

6956
public function __toString()
@@ -76,7 +63,8 @@ public function serialize()
7663
return serialize(array(
7764
$this->class,
7865
$this->filePath,
79-
$this->constructorArguments,
66+
$this->constructorMetadata,
67+
$this->settersMetadata
8068
));
8169
}
8270

@@ -85,7 +73,8 @@ public function unserialize($serialized)
8573
list(
8674
$this->class,
8775
$this->filePath,
88-
$this->constructorArguments
76+
$this->constructorMetadata,
77+
$this->settersMetadata
8978
) = unserialize($serialized);
9079
}
9180
}

‎src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -413,6 +413,43 @@ public function testOptionalScalarArgsNotPassedIfLast()
413413
$definition->getArguments()
414414
);
415415
}
416+
417+
/**
418+
* @dataProvider getCreateResourceTests
419+
*/
420+
public function testCreateResourceForClass($className, $isEqual)
421+
{
422+
$startingResource = AutowirePass::createResourceForClass(
423+
new \ReflectionClass(__NAMESPACE__.'\ClassForResource')
424+
);
425+
$newResource = AutowirePass::createResourceForClass(
426+
new \ReflectionClass(__NAMESPACE__.'\\'.$className)
427+
);
428+
429+
// hack so the objects don't differ by the class name
430+
$startingReflObject = new \ReflectionObject($startingResource);
431+
$reflProp = $startingReflObject->getProperty('class');
432+
$reflProp->setAccessible(true);
433+
$reflProp->setValue($startingResource, __NAMESPACE__.'\\'.$className);
434+
435+
if ($isEqual) {
436+
$this->assertEquals($startingResource, $newResource);
437+
} else {
438+
$this->assertNotEquals($startingResource, $newResource);
439+
}
440+
}
441+
442+
public function getCreateResourceTests()
443+
{
444+
return array(
445+
['IdenticalClassResource', true],
446+
['ClassChangedConstructorArgs', false],
447+
['ClassConstructorArgsChangeDefault', false],
448+
['ClassNewSetter', false],
449+
['ClassSetterDefaultValChanges', false],
450+
['ClassNonExistentTypeHint', false]
451+
);
452+
}
416453
}
417454

418455
class Foo
@@ -562,3 +599,50 @@ public function __construct(A $a, $foo = 'default_val', Lille $lille)
562599
{
563600
}
564601
}
602+
603+
/*
604+
* Classes used for testing createResourceForClass
605+
*/
606+
class ClassForResource
607+
{
608+
public function __construct($foo, Bar $bar = null)
609+
{
610+
}
611+
612+
public function setBar(Bar $bar)
613+
{
614+
}
615+
}
616+
class IdenticalClassResource extends ClassForResource
617+
{
618+
}
619+
class ClassChangedConstructorArgs extends ClassForResource
620+
{
621+
public function __construct($foo, Bar $bar, $baz)
622+
{
623+
}
624+
}
625+
class ClassConstructorArgsChangeDefault extends ClassForResource
626+
{
627+
public function __construct($foo, Bar $bar)
628+
{
629+
}
630+
}
631+
class ClassNewSetter extends ClassForResource
632+
{
633+
public function setNewSetter(Bar $bar)
634+
{
635+
}
636+
}
637+
class ClassSetterDefaultValChanges extends ClassForResource
638+
{
639+
public function setBar(Bar $bar = null)
640+
{
641+
}
642+
}
643+
class ClassNonExistentTypeHint extends ClassForResource
644+
{
645+
public function setFooBar(FooBarFake $bar)
646+
{
647+
}
648+
}

‎src/Symfony/Component/DependencyInjection/Tests/Config/AutowireServiceResourceTest.php

Lines changed: 37 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
namespace Symfony\Component\DependencyInjection\Tests\Config;
1313

14+
use Symfony\Component\DependencyInjection\Compiler\AutowirePass;
1415
use Symfony\Component\DependencyInjection\Config\AutowireServiceResource;
1516

1617
class AutowireServiceResourceTest extends \PHPUnit_Framework_TestCase
@@ -22,24 +23,20 @@ class AutowireServiceResourceTest extends \PHPUnit_Framework_TestCase
2223
private $file;
2324
private $class;
2425
private $time;
25-
private $constructorArgs;
2626

2727
protected function setUp()
2828
{
2929
$this->file = realpath(sys_get_temp_dir()).'/tmp.php';
3030
$this->time = time();
3131
touch($this->file, $this->time);
3232

33-
$this->class = 'Symfony\Component\DependencyInjection\Tests\Config\FooReplicator';
33+
$this->class = __NAMESPACE__.'\Foo';
3434
$this->resource = new AutowireServiceResource(
3535
$this->class,
36-
$this->file
36+
$this->file,
37+
array(),
38+
array('foood')
3739
);
38-
$this->constructorArgs = array(
39-
null, // no type-hint on this argument
40-
'AppBundle\Service\Foo',
41-
);
42-
$this->resource->setConstructorArguments($this->constructorArgs);
4340
}
4441

4542
public function testToString()
@@ -65,45 +62,42 @@ public function testIsFreshForDeletedResources()
6562
{
6663
unlink($this->file);
6764

68-
$this->assertFalse($this->resource->isFresh($this->time), '->isFresh() returns false if the resource does not exist');
65+
$this->assertFalse($this->resource->isFresh($this->getStaleFileTime()), '->isFresh() returns false if the resource does not exist');
6966
}
7067

71-
public function testIsFreshChangedConstructorArgs()
68+
public function testIsNotFreshChangedResource()
7269
{
73-
$newResource = new AutowireServiceResource(
70+
$oldResource = new AutowireServiceResource(
7471
$this->class,
75-
$this->file
72+
$this->file,
73+
array('will_be_different'),
74+
array()
7675
);
77-
// a new 3rd argument was added!
78-
$args = $this->constructorArgs;
79-
$args[] = 'AppBundle\Service\Bar';
80-
$newResource->setConstructorArguments($args);
81-
82-
// "fake" the return value
83-
$callback = function(\ReflectionClass $reflectionClass) use ($newResource) {
84-
return $newResource;
85-
};
86-
$this->resource->setCreateResourceCallback($callback);
87-
88-
$this->assertFalse($this->resource->isFresh($this->time - 10), '->isFresh() returns false if the constructor arguments have changed');
76+
77+
// test with a stale file *and* a resource that *will* be different than the actual
78+
$this->assertFalse($oldResource->isFresh($this->getStaleFileTime()), '->isFresh() returns false if the constructor arguments have changed');
8979
}
9080

9181
public function testIsFreshSameConstructorArgs()
9282
{
93-
$newResource = new AutowireServiceResource(
94-
$this->class,
95-
$this->file
83+
$oldResource = AutowirePass::createResourceForClass(
84+
new \ReflectionClass(__NAMESPACE__.'\Foo')
9685
);
9786

98-
$newResource->setConstructorArguments($this->constructorArgs);
87+
// test with a stale file *but* the resource will not be changed
88+
$this->assertTrue($oldResource->isFresh($this->getStaleFileTime()), '->isFresh() returns false if the constructor arguments have changed');
89+
}
9990

100-
// "fake" the return value
101-
$callback = function(\ReflectionClass $reflectionClass) use ($newResource) {
102-
return $newResource;
103-
};
91+
public function testNotFreshIfClassNotFound()
92+
{
93+
$resource = new AutowireServiceResource(
94+
'Some\Non\Existent\Class',
95+
$this->file,
96+
array(),
97+
array()
98+
);
10499

105-
$this->resource->setCreateResourceCallback($callback);
106-
$this->assertFalse($this->resource->isFresh($this->time - 10), '->isFresh() returns false if the constructor arguments have changed');
100+
$this->assertFalse($resource->isFresh($this->getStaleFileTime()), '->isFresh() returns false if the class no longer exists');
107101
}
108102

109103
protected function tearDown()
@@ -114,9 +108,16 @@ protected function tearDown()
114108

115109
unlink($this->file);
116110
}
111+
112+
private function getStaleFileTime()
113+
{
114+
return $this->time - 10;
115+
}
117116
}
118117

119-
// exists just so that some reflection in the tests doesn't fail
120-
class FooReplicator
118+
class Foo
121119
{
120+
public function __construct($foo)
121+
{
122+
}
122123
}

0 commit comments

Comments
 (0)
0