Exclude abstract methods from pointcut matching #335 #337

TheCelavi · 2017-08-08T23:05:20Z

This is a first, working idea. I guess there is a better approach. There are no tests written yet, however, I have tested internally and this do fixes my problem that I have described.

Solution seams un-natural and hackish. I will explain in code general idea.

@lisachenko I would like from you to review it and propose a better/more elegant solution for this fix implementation.

Thanks.

TheCelavi · 2017-08-08T23:06:01Z

src/Aop/Pointcut/ClassDeclaresPointcut.php

+/**
+ * Pointcut that filters out all class method and properties which are not declared within class
+ */
+class ClassDeclaresPointcut implements Pointcut


Idea is to have a pointcut matcher that will filter out all non-declared members and properties that are matched.

Looks like it is already implemented in MatchInheritedPointcut 😄

Yes, so it seams....

TheCelavi · 2017-08-08T23:07:28Z

src/Aop/Pointcut/ClassDeclaresPointcut.php

+     */
+    public function getKind()
+    {
+        return PointFilter::KIND_METHOD | PointFilter::KIND_PROPERTY;


As I have understood - this will ensure that this pointcut matcher will be applied ONLY if subject of matching is class method and class property, right?

Otherwise, matches function will not be invoked, right?

Yes, you are right, point filter effectively reduce the number of checks for concrete point filter. It's not ideal, but it's very efficient. KIND_METHOD filters only methods, KIND_PROPERTY filters properties in the class.

For future versions of framework I want to put this logic into matches() method. To be in one single place.

TheCelavi · 2017-08-08T23:10:58Z

src/Aop/Pointcut/PointcutParser.php

+     */
+    public function parse(TokenStream $stream)
+    {
+        return new AndPointcut(parent::parse($stream), new ClassDeclaresPointcut());


Ok, so this is why I think that this solution is kinda hackish...

Whatever our parser gets as a pointcut filter, it has to be additionally filtered. So it has to be applied last.

parent::parse($stream) -> will provide a pointcut that will match something, and that something can be a collection of properties and methods, right?

So, whatever methods and properties we got, we have to apply ClassDeclaresPointcut matcher to filter out non-declared properties and methods.

I guess that Grammar should give us this expression, is that even possible?

Oh, crap, I forgot that we can add a node visitor that would modify AST and provide us with same result: https://github.com/jakubledl/dissect/blob/master/docs/ast.md#traversing-the-ast

I can write node visitor to modify AST in order to move this logic from PointcutParser to node visitor and grammar.

However - still need from you the verification of idea so I can proceed (write node visitor, tests, etc...)

Ok, seams wrong -> I should only wrap original Pointcut if getKind() is PROPERTY or METHOD, right?

You shouldn't touch the PointcutParser class at all, it's final :) Instead, all changes should be implemented in grammar and AST transforming, see PointcutGrammar class for examples how AST will be converted to the pointcut.

lisachenko · 2017-08-09T13:21:08Z

Hi, Nikola! Thank you for researching this! But correct me if I'm wrong: you want to exclude all parent methods that aren't declared in current class, right?

If it's what you want to achieve, then there is special syntax for that: && !matchInherited(). You can see this syntax in PointcutParser test case

            // This will match all methods, but not inherited
            ['execution(public **->*(*)) && !matchInherited()'],

TheCelavi · 2017-08-09T13:51:48Z

Yes. Ok, I got confused here for sure - why !matchInherited() is not applied by default?

Why would weaving of method/property occur on a class that does not declares it?

TheCelavi · 2017-08-09T13:58:19Z

Ok, this is the issue of understanding what is the bug and what is not. This proposal is related to #335.

Lack of declaration existence check leads to issue #335

For me - there is no logic to weave method/property that is not declared within the class. If there is - I believe that it should be explicitly requested by pointcut expression, ie. !matchInherited() should be by default.

This was my usecase:

Interface X {
    public function foo();
}

abstract class Y implements X {

}

class Z extends Y {

    public function foo() {
    }
}

Booth Z and Y classes are weaved. It should be only Z. And that should be by default, right?

lisachenko · 2017-08-09T15:50:53Z

Previously developers asked me why there is no matching for parent methods, issue about matching for parent methods was described in #131 and later I was implemented logic to match parent properties and methods, see #272 and #274. New logic was introduced in PR #279.

This was logical for me to match methods from parent classes during weaving. And filtration should be performed via pointcut definition.

Regarding your question about switching matching to declared members only be default - it's discussable. Maybe we can introduce new option for the kernel to allow/disable matching with parent members for classes?

TheCelavi · 2017-08-09T15:58:49Z

Ok, THANK YOU - now we are getting somewhere. In conclusion, we can not change this behaviour, it would result with BC break.

Conclusion is: weaving will occur always on all matching methods/properties, regardless if it is defined in class or some of the parent class (unless ! matchInherited() is specified). This behaviour must be documented! So, this is on me.

Ok, so we have a bug, that is for sure, which is easy to define now:

Pointcut MUST NOT match abstract method of class
Pointcut MUST NOT match method of class that is declared in interface

Simple - what is not implemented in class hierarchy, can not be weaved.

Agree?

(if we can identify bug and agree upon what is a bug - we can solve it)

lisachenko · 2017-08-09T16:08:47Z

Pointcut MUST NOT match abstract method of class

Agree

Pointcut MUST NOT match method of class that is declared in interface

Actually, it's the same as the first check, because method from an interface will be abstract too in reflection. So resulting statement will be: "Pointcut MUST NOT match abstract methods"

TheCelavi · 2017-08-09T18:42:34Z

src/Aop/Support/ModifierMatcherFilter.php

@@ -47,6 +47,7 @@ class ModifierMatcherFilter implements PointFilter
    public function __construct($initialMask = 0)
    {
        $this->andMask = $initialMask;
+        $this->notMask = \ReflectionMethod::IS_ABSTRACT;


@lisachenko Ok, it seams that you are right, modifier filter should have default value of \ReflectionMethod::IS_ABSTRACT for notMask .

Test passes, but what is worries me is that there are no a lot of integration/functional tests where we can test what we actually get (like what I did for console commands).

What do you think about writing some functional tests that would have real-life fixtures for verification of weaving?

I feel myself shame for tests :) I'd vote for unit tests for each class and only several functional tests for checking that whole system is working as expected and still able to transform files and perform weaving.

For this concrete class we can add just simple test case that modifier matcher filter by default doesn't match abstract methods.

One more caution: modifier filter is also used for property matching. Of course, "not abstract" for properties will be always true. But if we will use ModiferFilter for properties with negation, then it can be very unpredictable :)

TheCelavi · 2017-08-10T08:27:40Z

src/Core/AdviceMatcher.php

@@ -166,6 +166,11 @@ private function getAdvicesFromAdvisor(

            $mask = ReflectionMethod::IS_PUBLIC | ReflectionMethod::IS_PROTECTED;
            foreach ($class->getMethods($mask) as $method) {
+                // abstract methods can not be weaved


One more caution: modifier filter is also used for property matching. Of course, "not abstract" for properties will be always true. But if we will use ModiferFilter for properties with negation, then it can be very unpredictable :)

You are soooo right! Ok, how about this? Simply - advice matcher will skip abstract methods for weaving, regardless of pointcut expression used?

This solution is perfect! 👍

lisachenko

Looking good now

…op#335 written.

Nitpick-CI · 2017-08-11T08:15:09Z

tests/Fixtures/project/src/Application/AbstractBar.php

+
+abstract class AbstractBar implements FooInterface
+{
+    public abstract function doSomethingElse();


The abstract declaration must precede the visibility declaration

Nitpick-CI · 2017-08-11T08:15:09Z

tests/Go/Functional/BaseFunctionalTest.php

+
+    protected static function exec($command, $args = '')
+    {
+        $process = new Process(sprintf('php %s %s %s %s',


Opening parenthesis of a multi-line function call must be the last content on the line

Better to follow PSR here and make it one-liner or extract sprintf part of expression into separate variable.

TheCelavi · 2017-08-11T08:18:54Z

tests/Go/Functional/BaseFunctionalTest.php

+use Symfony\Component\Filesystem\Filesystem;
+use Symfony\Component\Process\Process;
+
+abstract class BaseFunctionalTest extends TestCase


@lisachenko When you find a time, please review tests as well.

I have cleaned up code a little bit, setup a base for functional tests, write a required test as well as modified (cleaned up) command tests...

I have one more thing on my mind, however, even with this setup it is going to be quite easy to write a bunch of functional tests that we need.

lisachenko · 2017-08-11T09:59:44Z

tests/Fixtures/project/src/Aspect/DoSomethingAspect.php

+
+use Go\Aop\Aspect;
+use Go\Aop\Intercept\MethodInvocation;
+use Go\Lang\Annotation as Pointcut;


👍 nice import of namespace name

lisachenko · 2017-08-11T10:02:06Z

tests/Go/Functional/BaseFunctionalTest.php

+    protected static $projectDir = __DIR__.'/../../Fixtures/project';
+    protected static $aspectCacheDir = __DIR__.'/../../Fixtures/project/var/cache/aspect';
+    protected static $consolePath = __DIR__.'/../../Fixtures/project/bin/console';
+    protected static $frontControllerPath = __DIR__.'/../../Fixtures/project/web/index.php';


I'd prefer to have these assignments aligned... It will be more readable.

Can we also replace these static properties with simple class properties?

Not recommended at all. In order to be compatible with static calls, such as public static function setUpBeforeClass() they must be static, that is, must be accessible in static context.

Best thing what I can do is to define them as constants, if you prefer that more.

lisachenko · 2017-08-11T10:04:47Z

tests/Go/Functional/BaseFunctionalTest.php

+
+    protected static function exec($command, $args = '')
+    {
+        $process = new Process(sprintf('php %s %s %s %s',


Better to follow PSR here and make it one-liner or extract sprintf part of expression into separate variable.

lisachenko · 2017-08-11T13:21:41Z

Oops, merged without squashing changes. Going to clean manually...

…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 · 2017-08-11T14:08:06Z

Merged into master as well.

Added declare(strict_types = 1); to all new files for master branch.

Proposal for review of fix for goaop#335

6acce0e

TheCelavi requested a review from lisachenko August 8, 2017 23:05

TheCelavi commented Aug 8, 2017

View reviewed changes

TheCelavi added 2 commits August 9, 2017 20:37

Revert to previous, modifying only ModifierMatcherFiletr.

02c75b7

Remove unused use statement

4c7b6c8

TheCelavi commented Aug 9, 2017

View reviewed changes

AdviceMatcher should reject abstract method as fix for goaop#335

7f4fded

TheCelavi commented Aug 10, 2017

View reviewed changes

lisachenko changed the title ~~Proposal for review of fix for #335~~ Exclude abstract methods from pointcut matching #335 Aug 10, 2017

lisachenko approved these changes Aug 10, 2017

View reviewed changes

Cleaned up tests, base for functional tests setup, basic test for goa…

65d27d9

…op#335 written.

Nitpick-CI reviewed Aug 11, 2017

View reviewed changes

Fixed CS and improved test.

41b8386

TheCelavi commented Aug 11, 2017

View reviewed changes

lisachenko reviewed Aug 11, 2017

View reviewed changes

Fixed code style.

95c24ac

lisachenko merged commit e9dace6 into goaop:2.x Aug 11, 2017

lisachenko added Improvement Tests labels Aug 11, 2017

lisachenko mentioned this pull request Aug 13, 2017

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exclude abstract methods from pointcut matching #335 #337

Exclude abstract methods from pointcut matching #335 #337

TheCelavi commented Aug 8, 2017

TheCelavi Aug 8, 2017

lisachenko Aug 9, 2017

TheCelavi Aug 9, 2017

TheCelavi Aug 8, 2017

lisachenko Aug 9, 2017

TheCelavi Aug 8, 2017

TheCelavi Aug 8, 2017

TheCelavi Aug 9, 2017

lisachenko Aug 9, 2017

lisachenko commented Aug 9, 2017

TheCelavi commented Aug 9, 2017

TheCelavi commented Aug 9, 2017 •

edited

Loading

lisachenko commented Aug 9, 2017

TheCelavi commented Aug 9, 2017 •

edited

Loading

lisachenko commented Aug 9, 2017 •

edited

Loading

TheCelavi Aug 9, 2017

lisachenko Aug 9, 2017

TheCelavi Aug 10, 2017

lisachenko Aug 10, 2017

lisachenko left a comment

Nitpick-CI Aug 11, 2017

Nitpick-CI Aug 11, 2017

lisachenko Aug 11, 2017

TheCelavi Aug 11, 2017 •

edited

Loading

lisachenko Aug 11, 2017

lisachenko Aug 11, 2017

TheCelavi Aug 11, 2017

lisachenko Aug 11, 2017

lisachenko commented Aug 11, 2017

lisachenko commented Aug 11, 2017

Exclude abstract methods from pointcut matching #335 #337

Exclude abstract methods from pointcut matching #335 #337

Conversation

TheCelavi commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lisachenko commented Aug 9, 2017

TheCelavi commented Aug 9, 2017

TheCelavi commented Aug 9, 2017 • edited Loading

lisachenko commented Aug 9, 2017

TheCelavi commented Aug 9, 2017 • edited Loading

lisachenko commented Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lisachenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheCelavi Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lisachenko commented Aug 11, 2017

lisachenko commented Aug 11, 2017

TheCelavi commented Aug 9, 2017 •

edited

Loading

TheCelavi commented Aug 9, 2017 •

edited

Loading

lisachenko commented Aug 9, 2017 •

edited

Loading

TheCelavi Aug 11, 2017 •

edited

Loading