Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUGFIX: Correctly reflect type of aliased classes in method typehints #1891

Open
wants to merge 8 commits into
base: 5.3
from

Conversation

@albe
Copy link
Member

albe commented Dec 21, 2019

The namespace change introduced with doctrine/persistence 1.3.x makes use of class_alias to create a b/c layer over the old class names (see https://github.com/greg0ire/type_deprecation_experiment). However, this broke for a couple of our depending packages that still used the old Doctrine\Common\Persistence\ObjectMananger class as a method typehint.
The reason is, that PHP Reflection returns different values for aliased method parameter types when using ReflectionParameter::getClass()->getName() vs. ReflectionParameter::getType()->getName().
The former returns the name of the actual class, instead of the alias, while the latter returns the actual name that was used in the typehint, regardless if it is an alias only.

In turn, this led our dependency injection to try to inject classes that it was not aware of and led to issues like yeebase/Yeebase.TwoFactorAuthentication#3 or neos/Neos.EventSourcing#249
It was not an issue for injections that were not typehinted via PHP, e.g. @Flow\Inject instead of setter/constructor injection.

This change fixes that retrospectively, by always reflecting the actual class name that is specified in the typehint and hence allowing usage of class_alias.

Thanks a lot @alcaeus for helping me figure this out!

Apply fixes from StyleCI
@albe

This comment has been minimized.

Copy link
Member Author

albe commented Dec 21, 2019

Note: The first commit only adds the test that shows the minimal breaking behaviour for verification. Commit with the actual fix coming.

image

@@ -56,13 +56,10 @@ public function getClass()
*/
public function getBuiltinType()
{
if (!is_callable([$this, 'getType'])) {

This comment has been minimized.

Copy link
@albe

albe Dec 21, 2019

Author Member

Was only necessary for PHP 5.x compatibility back then. No longer needed.

$type = $this->getType();
if ($type === null || !$type->isBuiltin()) {
return null;
}
return (string)$type;

This comment has been minimized.

Copy link
@albe

albe Dec 21, 2019

Author Member

__toString() is deprecated on ReflectionType as of PHP 7.1

@@ -1785,7 +1785,7 @@ protected function convertParameterDataToArray(array $parametersInformation)
* @param MethodReflection $method The parameter's method
* @return array Parameter information array
*/
protected function convertParameterReflectionToArray(ParameterReflection $parameter, MethodReflection $method = null)
protected function convertParameterReflectionToArray(ParameterReflection $parameter, MethodReflection $method)

This comment has been minimized.

Copy link
@albe

albe Dec 21, 2019

Author Member

We never ever call this protected method without a MethodReflection so...

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Jan 6, 2020

Member

Since ReflectionService is not final and this method not private, this could still break things – it's very unlikely though

This comment has been minimized.

Copy link
@albe

albe Jan 6, 2020

Author Member

True. I mean we can postpone this change to master too, not picky here. But I also don't see this super internal method problematic either.

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Jan 8, 2020

Member

Yes, I agree and don't think it's necessary to postpone this

if (isset($paramAnnotations[$parameter->getPosition()])) {
$explodedParameters = explode(' ', $paramAnnotations[$parameter->getPosition()]);
if (count($explodedParameters) >= 2) {
$parameterType = $this->expandType($method->getDeclaringClass(), $explodedParameters[0]);

This comment has been minimized.

Copy link
@albe

albe Dec 21, 2019

Author Member

This is still needed to override the PHP typehint in order to allow generics/collection element typehints.

@albe albe changed the title BUGFIX: Add test to cover reflection of aliased classes BUGFIX: Correctly reflect type of aliased classes in method typehints Dec 21, 2019
@albe

This comment has been minimized.

Copy link
Member Author

albe commented Dec 21, 2019

Hm, seems PHP 7.4 does somehow not work out.

Edit: We're not setting settings on ReflectionService in the tests, so this tried to access an index on null. PHP 7.4 is more strict on that.

@albe

This comment has been minimized.

Copy link
Member Author

albe commented Jan 5, 2020

Copy link
Member

bwaidelich left a comment

Great!
Just a tiny remark regarding the signature change in ReflectionService but I think it can be neglected.

Thanks for adding tests first so the failing case could be tested without hassle!

@@ -1785,7 +1785,7 @@ protected function convertParameterDataToArray(array $parametersInformation)
* @param MethodReflection $method The parameter's method
* @return array Parameter information array
*/
protected function convertParameterReflectionToArray(ParameterReflection $parameter, MethodReflection $method = null)
protected function convertParameterReflectionToArray(ParameterReflection $parameter, MethodReflection $method)

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Jan 6, 2020

Member

Since ReflectionService is not final and this method not private, this could still break things – it's very unlikely though

@albe

This comment has been minimized.

Copy link
Member Author

albe commented Jan 15, 2020

I'd like to merge this soonish @kdambekalns @kitsunet - so if you could take a quick check this is okay as is, that would be nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.