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

[BUG] Method matching positive on abstract classes which does not implements pointcuted method from interface #335

Closed
TheCelavi opened this issue Jul 31, 2017 · 5 comments

Comments

@TheCelavi
Copy link
Contributor

Consider usecase (precondition):

  • Class/Interface structure: Interface I -> AbstractClass A implements I -> ConcreteClass C extends A
  • Interface defines method foo()
  • Abstract class DOES NOT implements foo()
  • ConcreteClass implements foo()

Consider that ALL stated Interface/Classes are within same namespace.

If matcher is execution(public *->foo()), abstract class gets weaved -> it is interpreted as if there is a method foo, but there is not.

Possible fix: check PHP Tokenizer and class metadata for better introspection of implemented methods.

@lisachenko
Copy link
Member

Hi! I can see for the abstract class, that it doesn't contain an abstract method after proxy generation. because ClassProxy's method overrideMethod has special temporary check at line 382:

        // temporary disable override of final methods
        if (!$method->isFinal() && !$method->isAbstract()) {
            $this->override($method->name, $this->getJoinpointInvocationBody($method));
        }

This check prevents interception of final or abstract methods (including methods from interfaces).

But I can agree that pointcut currently matches abstract and final methods too. Not sure if it's good or bad, just possible to match abstract method. It can be used for example for introduction pointcuts to introduce new properties, methods or body for interfaces.

@TheCelavi
Copy link
Contributor Author

Not sure if it's good or bad, just possible to match abstract method.

I think it is bad, it should not match what can not be weaved.

It can be used for example for introduction pointcuts to introduce new properties, methods or body for interfaces.

Isn't that different story here? Introductions would introduce new Interfaces to a class, and via traits interfaces are implemented, right?

@lisachenko
Copy link
Member

Isn't that different story here? Introductions would introduce new Interfaces to a class, and via traits interfaces are implemented, right?

Yes, for example, TYPO3 just adds properties and methods into specific class. But I've decided to switch to traits and interfaces to have an ability to put breakpoints in traits. And it also gives me control over line numbering in the classes which was important.

I think it is bad, it should not match what can not be weaved.

Ok, let's fix this 👍

@lisachenko
Copy link
Member

lisachenko commented Aug 7, 2017

I haven't checked, but I think it will be possible to do this on grammar level by giving to the ModifierMatcherFilter negative filter for abstract modifier like that:

$modifierMatcherFilter = new ModifierMatcherFilter();
$modifierMatcherFilter->notMatch(\ReflectionMethod::IS_ABSTRACT);

TheCelavi added a commit to RunOpenCode/framework that referenced this issue Aug 8, 2017
TheCelavi added a commit to RunOpenCode/framework that referenced this issue Aug 10, 2017
TheCelavi added a commit to RunOpenCode/framework that referenced this issue Aug 11, 2017
lisachenko added a commit that referenced this issue Aug 11, 2017
…g-class-non-declared-methods-and-properties

Exclude abstract methods from pointcut matching #335
lisachenko pushed a commit that referenced this issue Aug 11, 2017
Cleaned up tests, base for functional tests setup, basic test for #335 written.
lisachenko added a commit that referenced this issue Aug 11, 2017
…g-class-non-declared-methods-and-properties

AdviceMatcher should reject abstract method as fix for #335
Cleaned up tests, base for functional tests setup, basic test for #335 written.
lisachenko added a commit that referenced this issue Aug 11, 2017
* origin/2.x:
  AdviceMatcher should reject abstract method as fix for #335
@lisachenko
Copy link
Member

Fixed in #337

@lisachenko lisachenko added this to the 2.x milestone Aug 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants