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

Add Assert::implementsInterface #15

Merged
merged 4 commits into from
Oct 1, 2019

Conversation

boesing
Copy link
Contributor

@boesing boesing commented Sep 30, 2019

No description provided.

@boesing boesing requested a review from ondrejmirtes October 1, 2019 11:10
@ondrejmirtes ondrejmirtes merged commit f57366a into phpstan:master Oct 1, 2019
@ondrejmirtes
Copy link
Member

Thank you! Will tag it promptly.

@boesing boesing deleted the feature/implements-interface branch October 1, 2019 13:49
@mamazu
Copy link

mamazu commented Oct 3, 2019

Nice pull request however this doesn't work in the case where both arguments are strings which the assert function allows.

@ondrejmirtes
Copy link
Member

Yes, AFAIK PHPStan does not understand $object instanceof $class so it should be fixed there.

@mamazu
Copy link

mamazu commented Oct 3, 2019

Yeah I would also go with the class_implements approach like this:

return new \PhpParser\Node\Expr\FuncCall(
    new \PhpParser\Node\Name('in_array'),
    [
        $class,
        new \PhpParser\Node\Expr\FuncCall(new \PhpParser\Node\Name('class_implements'), [$expr]),
    ]
);

@ondrejmirtes
Copy link
Member

I’m not sure there’s class_implements dynamic return type extension in PHPStan and that this would work...

@mamazu
Copy link

mamazu commented Oct 3, 2019

True.

@mamazu
Copy link

mamazu commented Oct 4, 2019

But the only thing that phpstan could optimize for are two cases:

  1. Both values are "constant" strings then the assertion doesn't make sense (either it always throws or never throws)
  2. If the object implements the interface by type hint. For all other cases there shouldn't be any optimization.

But instanceof doesn't work if both arguments are strings.

@melkamar
Copy link

melkamar commented Oct 30, 2019

Hi, PHPStan is reporting errors where it should not when upgrading to this version. The following code is, imo, correct:

class FilterDefinition
{
    /* @var string */
    private $filterClass;

    /**
     * @param string $filterClass FQCN of a filter class. The string must be a PHP class which implements the {@see FilterInterface}
     */
    public function __construct(string $filterClass)
    {
        Assert::implementsInterface($filterClass, FilterInterface::class);

        $this->filterClass = $filterClass;
    }
}

Instances of FilterDefinition are used in some factory methods and I need to ensure that the provided class (not instances of the class!) implements the FilterInterface. So the Assert::implementsInterface() makes sense here, yet PHPStan yells at me with

Call to static method Webmozart\Assert\Assert::implementsInterface() with string and 'RH\\CoreBundle…' will always evaluate to false. 

Any suggested workaround, other than fixing the version to 0.11.2?

@mamazu
Copy link

mamazu commented Oct 30, 2019

Yeah that's the same issue I also tried to fix in my merge request. But the quick fix would be to create an object of that class, which is probably not really a possible solution.

@melkamar
Copy link

I see. No, instantiating the class is not an option here, so I'll have to downgrade.

Just wanted to add the comment here for visibility, that this issue affects someone else as well :)

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.

4 participants