[DI] Refacto / cleanup / minor fixes #21561
Conversation
2818954 to
7651262
Compare
| foreach ($methodCalls as $i => $call) { | ||
| list($method, $arguments) = $call; | ||
| $method = $parameterBag->resolveValue($method); |
There was a problem hiding this comment.
this should not happen - we don't resolve method calls in ContainerBuilder/PhpDumper
| @@ -24,6 +24,9 @@ | |||
| */ | |||
| class AutowirePass extends AbstractRecursivePass | |||
| { | |||
| const MODE_REQUIRED = 1; | |||
| const MODE_OPTIONAL = 2; | |||
There was a problem hiding this comment.
shouldn't we mark these constants as internal ?
| @@ -510,22 +511,31 @@ private function addServiceOverriddenGetters($id, Definition $definition) | |||
| } | |||
| if (!$r->isFinal()) { | |||
| if (0 < $r->getNumberOfParameters()) { | |||
| $getters = implode('__construct($container'.$this->salt.', ', explode('(', $this->generateSignature($r), 2)); | |||
| $constructor = implode('($container'.$this->salt.', ', explode('(', InheritanceProxyHelper::getSignature($r, $call), 2)); | |||
There was a problem hiding this comment.
aren't we missing $r->name at the beginning ?
There was a problem hiding this comment.
btw, this tells me that we may be missing a test for this code path, as Travis is green
There was a problem hiding this comment.
getSignature now embeds the name of the method, so no bug (and yes it's tested :) )
| return $r; | ||
| } | ||
|
|
||
| public static function getSignature(\ReflectionFunctionAbstract $r, &$call = null) |
There was a problem hiding this comment.
would be great to document it a bit (for instance explaining what $call will be)
| return ($r->returnsReference() ? '&' : '').($r->isClosure() ? '' : $r->name).'('.implode(', ', $signature).')'.$type; | ||
| } | ||
|
|
||
| public static function getTypeHint($type, \ReflectionFunctionAbstract $r) |
There was a problem hiding this comment.
$type should be documented as string|ReflectionType
|
|
||
| public static function getTypeHint($type, \ReflectionFunctionAbstract $r) | ||
| { | ||
| if (is_string($type)) { |
There was a problem hiding this comment.
this case only happen on PHP 5.x, right ? If yes, we should add a comment about this, to be helpful for the guy doing the cleanup in Symfony 4. If no, we need to support iterable, void and scalar types just below.
7651262 to
ad5b5b5
Compare
|
@stof comments addressed |
|
Thank you @nicolas-grekas. |
…kas) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] Refacto / cleanup / minor fixes | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - An almost neutral refacto touching only new things in master - unlocking some next steps. - renames "autowire method" to "autowire call" - renames `GetterProxyInterface` to `InheritanceProxyInterface` - moves some helpers in a new internal `InheritanceProxyHelper` - some other minor things Commits ------- ad5b5b5 [DI] Refacto / cleanup / minor fixes
An almost neutral refacto touching only new things in master - unlocking some next steps.
GetterProxyInterfacetoInheritanceProxyInterfaceInheritanceProxyHelper