Skip to content

Conversation

VincentLanglet
Copy link
Contributor

No description provided.

*/
private function isSupported(Expr $expr, Scope $scope): bool
{
if (!($expr instanceof MethodCall) || !($expr->name instanceof Identifier) || $expr->name->name !== self::TRAIT_METHOD_NAME) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the logic. You're interested in the method name and you're interested where we're calling it from.

But you're not interested on what we're calling the method. This extension might be influencing a total coincidence that's nothing to do with BrowserKitAssertionsTrait::getClient().

We're in a class, the class has getClient method. And the method is declared in BrowserKitAssertionsTrait trait. But the method we're still calling might be unrelated.

My hunch is telling me you're no checking $scope->getMethodReflection($scope->getType($expr->var), $expr->name->name) but you should be.

Also, method names are case-insensitive, so toLowerString() needs to happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied the logic from the extension you gave me as example

private function isSupported(Expr $expr, Scope $scope): bool
{
if (!($expr instanceof MethodCall) || !($expr->name instanceof Identifier) || $expr->name->name !== self::TRAIT_METHOD_NAME) {
return false;
}
if (!$scope->isInClass()) {
return false;
}
$reflectionClass = $scope->getClassReflection()->getNativeReflection();
if (!$reflectionClass->hasMethod(self::TRAIT_METHOD_NAME)) {
return false;
}
$methodReflection = $reflectionClass->getMethod(self::TRAIT_METHOD_NAME);
$declaringClassReflection = $methodReflection->getBetterReflection()->getDeclaringClass();
return $declaringClassReflection->getName() === self::TRAIT_NAME;
}

So I assume it should be improve on the other extension too.

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