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

Validate return type using reflection #78

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

adrian-martinez-onestic
Copy link

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
Copy link
Author

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
Copy link
Contributor

fredden commented Apr 2, 2024

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
Copy link
Author

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.

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

Successfully merging this pull request may close these issues.

2 participants