Magento2: Interceptor class methods do not support nullable return types

Created on 24 May 2018 · 4Comments · Source: magento/magento2

Preconditions

Magento 2.3.1-develop (cloned from magento-engcom/php-7.2-support)
PHP 7.2.5.1

Steps to reproduce

Declare a public method with a nullable return type on a class that will have an Interceptor generated for it. In this case, the class extends from the Session Manager:

namespace MyVendor\MyModule\Model;

class MySession extends Magento\Framework\Session\SessionManager
{
    public function getSomeString() : ?string
    {
           return $this->getData('something');
    }
}

Trigger code generation in your environment.

Expected result

The method signature on the Interceptor should be the exact same as the overridden method signature.

Actual result

The method signature on the Interceptor does not include the nullable return type.

namespace MyVendor\MyModule\Model\MySession;

class Interceptor extends \MyVendor\MyModule\Model\Session
{
    public function getSomeString() : string
    {
        $pluginInfo = $this->pluginList->getNext($this->subjectType, 'getSomeString');
        if (!$pluginInfo) {
            return parent::getSomeString();
        } else {
            return $this->___callPlugins('getSomeString', func_get_args(), $pluginInfo);
        }
    }
}

Further Investigation

After digging into this issue, I'm not sure if this is Magento or Zend's fault. It is clear via this declined RFC that ReflectionNamedType probably should have included the ? nullable indicator when the object is represented as a String and allowsNull is true, however this RFC was declined, therefore this change did not occur. To me, this feels like an oversight in PHP, and should be better documented. I digress; onto the investigation.

During the code generation procedure for the Interceptor, when adding methods (in Magento/Framework/Code/Generator/ClassGenerator::addMethods), Magento creates a new instance of Zend's MethodGenerator class and proceeds to "manually" build the object with setters. This approach is an alternative to using Zend's MethodGenerator::fromReflection, which calls for a \ReflectionMethod. Presumably this method is not used because Magento's Code\Generator\Interceptor class _getClassMethods returns an array of method data (strings, objects, Reflections, etc.) and not an array of \ReflectionMethod object, hence the Zend class' named constructor cannot be used (which does account for a nullable ReflectionNamedType if you dig into it).

Jumping back into Magento/Framework/Code/Generator/ClassGenerator::addMethods where Magento is building the object with setters, we find this on line 134:

$methodObject->setReturnType($methodOptions['returnType']);

Where $methodObject is Zend\Code\Generator\MethodGenerator and $methodOptions['returnType'] is a ReflectionNamedType that appropriately returns true when allowsNull is invoked.

Here is where I'm unsure if it is Zend's mishandling of ReflectionNamedType or Magento's improper use of Zend Framework's MethodGenerator.

The setReturnType method typehints a string (so ReflectionNamedType works as it implements __toString()) or null, as so:

    /**
     * @param string|null
     *
     * @return MethodGenerator
     */
    public function setReturnType($returnType = null)
    {
        $this->returnType = null === $returnType
            ? null
            : TypeGenerator::fromTypeString($returnType);

        return $this;
    }

To me, this suggests maybe Magento is misusing Zend's framework by erroneously passing it an instance of ReflectionNamedType, though it is equally possible that Zend has overlooked how ReflectionNamedType functions (i.e., it is castable to a string). As we dig into fromTypeString, we find the following:

    public static function fromTypeString($type)
    {
        list($nullable, $trimmedNullable) = self::trimNullable($type);
        list($wasTrimmed, $trimmedType) = self::trimType($trimmedNullable);

Remember, $type is an instance of ReflectionNamedType. As you see, it is passed over to trimNullable which does this:

    private static function trimNullable($type)
    {
        if (0 === strpos($type, '?')) {
            return [true, substr($type, 1)];
        }

        return [false, $type];
    }

This is where I'm unsure if Zend is not accounting for the ReflectionNamedType being cast to a string or is unaware of PHP's behavior excluding the ? nullable indicator when __toString() is invoked. Perhaps worth noting, ReflectionNamedType__toString() method is marked as deprecated as of PHP 7.1

When a ReflectionNamedType is nullable return type (i.e., allowsNull() returns true), getName() (or casting to a string, as is done here, seemingly intentionally by Magento's team) does NOT include the ? nullable indicator; rather it only includes the scalar type (e.g. "string"). Therefore, the generated method footprint on the Interceptor class does not include the ? nullable indicator, and the described issue occurs.

FrameworCode FrameworInterception FrameworObjectManager Fixed in 2.2.x Fixed in 2.3.x Clear Description Confirmed Format is valid

Source

jsamhall

👍3

Most helpful comment

According to this declined RFC it appears the exclusion of ? when casting to the object to a string was an oversight/mistake due to lackluster documentation. The RFC proposed fixing this (by ensuring ? is included for nullable types) which classified as a BC break. Therefore I think the bug is Magento (via misuse of Zend) or Zend's (via poor handling of ReflectionNamedType) responsibility.

jsamhall on 25 May 2018

👍3