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

Fix #922, ProxyGenerator missing assert #923

Merged

Conversation

GameplayJDK
Copy link
Contributor

@GameplayJDK GameplayJDK commented Jan 26, 2021

Upon suggestion I've copied this description from the Ticket.

Hello everyone,

while working on #917 I noticed a missing assert() in the method getParameterType() of the ProxyGenerator.

The ReflectionParameter::getDeclaringFunction() method returns a ReflectionFunctionAbstract, the type hint of ProxyGenerator::formatType(...) is ReflectionMethod, one of two possible implementations, the other is ReflectionFunction.

    /**
     * @return string|null
     */
    private function getParameterType(ReflectionParameter $parameter)
    {
        if (! $parameter->hasType()) {
            return null;
        }

        return $this->formatType($parameter->getType(), $parameter->getDeclaringFunction(), $parameter);
    }

I'd propose a change from the above code to the code below:

    /**
     * @return string|null
     */
    private function getParameterType(ReflectionParameter $parameter)
    {
        if (! $parameter->hasType()) {
            return null;
        }

        $declaringFunction = $parameter->getDeclaringFunction();

        assert($declaringFunction instanceof ReflectionMethod);

        return $this->formatType($parameter->getType(), $declaringFunction, $parameter);
    }

3v4l.org snippet

Of course the assertion will always be true, but this theoretical type mismatch was pointed out by my IDE, which confused me at first. In my opinion this would make the code more clean and prevent any future confusion about what type is expected.

@greg0ire
Copy link
Member

Please amend your commit message according to the contributing guide, currently it's hard to guess why the assert is missing solely based on that message.

An assertion is missing from `ProxyGenerator::getParameterType()` after retrieving an `ReflectionFunctionAbstract` instance (per `ReflectionParameter::getDeclaringFunction()`). It is argument to `ProxyGenerator::formatType()`, which expects an `ReflectionMethod` object. Since that is one of the two possible concrete implementations of said abstract class, an `assert()` call should by added beforehand to eliminate any possible confusion and to avoid IDE complaints.
@GameplayJDK GameplayJDK force-pushed the 3.1.x_fix-proxygenerator-missing-assert branch from 344bfd5 to 034bb3e Compare January 27, 2021 21:28
@malarzm malarzm added this to the 3.1.2 milestone Feb 6, 2021
@malarzm
Copy link
Member

malarzm commented Feb 6, 2021

@greg0ire are you ok with current commit message?

Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

Yes, it looks great!

@greg0ire greg0ire merged commit 565ce4a into doctrine:3.1.x Feb 6, 2021
@greg0ire
Copy link
Member

greg0ire commented Feb 6, 2021

Thanks @GameplayJDK !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants