hyva-themes / magento2-hyva-admin

This module aims to make creating grids and forms in the Magento 2 adminhtml area joyful and fast.
https://hyva-themes.github.io/magento2-hyva-admin/
BSD 3-Clause "New" or "Revised" License
168 stars 39 forks source link

Validate return type using reflection #78

Open adrian-martinez-onestic opened 4 months ago

adrian-martinez-onestic commented 4 months ago

This change aims to use declared method return type value, if defined, over doc block @return tag annotation, for \Hyva\Admin\Model\GridSourceType\RepositorySourceType\RepositorySourceFactory::validateReturnType validation.

It has been tested against the following PHP versions: 7.3 - 7.4 - 8.0 - 8.1 - 8.2 - 8.3

Given the following, simplified, class:

<?php

declare(strict_types=1);

namespace Some\Namespace;

use Magento\Framework\Api\SearchCriteriaInterface;
use Magento\Framework\Exception\NotFoundException;
use Magento\Framework\Api\SearchResultsInterface;

class Details
{
    /**
     * @param SearchCriteriaInterface $searchCriteria
     *
     * @return SearchResultsInterface
     *
     * @throws NotFoundException
     */
    public function getHistory(SearchCriteriaInterface $searchCriteria): SearchResultsInterface
    {
        (...)
    }
}

Current code execution relies in \Magento\Framework\Reflection\MethodsMap::getMethodReturnType method, which in turn uses only the @return tag, to retrieve the return type. In the case above, SearchResultsInterface is found as the @return type declared in the doc block, but it fails to return its FQDN, so the \is_subclass_of check fails.

Simplifying and importing classes instead of using its FQDN every time is a common practice (PhpStorm, php-cs-fixer...), and cannot be applied in a standard way if this @return tag needs to be fully qualified in order to work.

This change proposes to use reflection to get method declared return type, which is enforced to return, and is more reliable than a doc block annotation for validation. Approach can be discussed, as the suggested changes have been implemented quickly for testing purposes (although they work) and may be improved regarding caching, performance, or another criteria.

I hope this PR proposal is interesting to be added to source code, write me for any aclaration if needed, thank you!

adrian-martinez-onestic commented 4 months ago

Current suggested code may actually fail in PHP 7.3 due to coalesce assignment operator used at $returnType ??= $this->reflectionMethodsMap->getMethodReturnType($class, $method);, but can be changed to something compatible with PHP 7.3 if needed.

fredden commented 4 months ago

This looks like something which might be good to offer for the \Magento\Framework\Reflection\MethodsMap::getMethodReturnType() method itself, rather than here. However, getting code into the core of Magento2 is a long process.

adrian-martinez-onestic commented 4 months ago

However, getting code into the core of Magento2 is a long process.

I know :( , that's why I propose it here, changing this code in Magento may have some side effects, as it is used in current functionality. Changing this piece of code here is simpler, since it is used as a defensive programming resource, its impact is limited and more controlled.