Advices are called multiple times in inheritance hierarchies

[neos-bot]

Jira issue originally created by user mhelmich:

I'm not sure if this behaviour is intentional; please feel free to yell if this is NOT a bug. I also noticed that it is very similar to FLOW-155 reported by [~vertexvaar] and can possibly be fixed with the same proposed solution.

I have a class hierarchy, and an advice around a method in the parent class. When calling the method (which calls it's parent method) on an instance of the subclass, the advice is called twice.

Simple example:

abstract class AbstractFoo {
    public function hello() {
        echo "Hello world.";
    }
    public function bye() {
        echo "Goodbye world.";
    }
}

class ConcreteFoo extends AbstractFoo {
    public function hello() {
        parent::hello();
        echo "Hallo Welt!";
    }
}

/****
 * @Flow\Aspect
 */
class HelloByeAspect {
    /****
     ** @Flow\Around("method(**.->hello())")
     */
    public function myHelloAspect(JoinPointInterface $joinPoint) {
        echo "called hello()";
    }
    /****
     ** @Flow\Around("method(**.->bye())")
     */
    public function myByeAspect(JoinPointInterface $joinPoint) {
        echo "called bye()";
    }
}

When calling ConcreteFoo->hello, I'd expect the advice to be invoked once; but it is actually invoked twice.
The same goes for ConcreteFoo->bye, which isn't even overwritten in ConcreteFoo, but the advice is called twice nevertheless.

My best guess is that, the Flow*Aop_Proxy*methodIsInAdviceMode property in the proxy classes is actually supposed to prevent this. However, this property is private and thus not visible in super(!)classes. Changing the visibility of this property to protected helps.

I tested with Flow 2.3, but probably occurs in earlier releases and in master, too. Will push a patch into Gerrit shortly.

Jira-URL: https://jira.neos.io/browse/FLOW-158

[neos-bot]

Comment created by vertexvaar:

Hi [~mhelmich],

i think you've found a Bug, too, since AbstractFoo::bar() is never called.

The way i understood AOP and inheritance, the Advice should only get called for both methods if the method of the superclass is called within the overwritten method like:

class ConcreteFoo extends AbstractFoo {
    public function bar() {
        echo "Hallo Welt!";
        parent::bar();
    }
}

IMHO setting the visibility of Flow*Aop_Proxy*methodIsInAdviceMode to protected absolutely makes sense, because it reflects class inheritance or method overwriting better. (But i could be just wrong, too )

Nevertheless, i've tested your suggestion and it works for my use case. I'll write a comment into FLOW-155 repeating your solution.

[neos-bot]

Comment created by mhelmich:

bq. i think you've found a Bug, too, since AbstractFoo::bar() is never called.

Whoops; that's just a bug in my code example. My real scenario is a bit more complex, and I wanted to strip it down a bit. Worked great, apparently. Anyway, thanks for noticing, I'll fix the issue description accordingly.

[neos-bot]

Comment created by mhelmich:

Ugh. Turns out there is a functional test that exactly asserts the (supposedly) faulty behaviour. So I suspect that is actually IS intentional, although I don't really see the reason for it. The only test case that fails is the AopProxyTest::advicesAreExecutedAgainIfAnOverriddenMethodCallsItsParentMethod, all other test cases pass.

[neos-bot]

Comment created by vertexvaar:

Closed because this is not a Bug and covered by a functional test

[neos-bot]

Comment created by @bwaidelich:

I'm not sure that this issue can be closed already, or is this considered a duplicate of FLOW-155? [~mhelmich] what do you think?
Btw: The test has been added with https://review.typo3.org/1407 maybe that helps? (I didn't dig into this one yet)

[neos-bot]

Comment created by mhelmich:

[~bwaidelich], thanks for the reply and for the Gerrit link. Actually, this bug seems to go beyond advices being called for parent calls. I have just extended my code examples a little bit; as far as I could see, the described behavior also occurs for methods that are not overridden in subclasses. I'll try to write a test case to reproduce this behaviour.

Not really sure if this is a duplicate of FLOW-155. One could argue whether this issue and FLOW-155 are different symptoms of the same problem (then duplicate) or different problems with the same solution (maybe no duplicate).

[neos-bot]

Comment created by mhelmich:

Reopened, because I'd still like to provide a small test case and clear whether this behavior is intentional.

[neos-bot]

Comment created by @bwaidelich:

Thanks for the feedback. I'll try to look into this one before holidays, some test scenarios would help a lot!

[neos-bot]

Comment created by mhelmich:

I've created two functional test cases which reproduce the issue at http://review.typo3.org/35455. I also came up with another solution that does not break any further functional tests, but I am not confident that it's fully backwards compatible. Any feedback is welcome.

[neos-bot]

Comment created by @bwaidelich:

Sorry, I lost track of this one.. Assigned it to myself now to avoid that from happening again.

[neos-bot]

Comment created by @bwaidelich:

FYI: I can reproduce the issue with Flow 3.0b6 but don't dare to merge the fix last-minute now for the final release.. But hopefully we can add it for the next bugfix releases!

[neos-bot]

Comment created by akii:

Does this fix the issue? https://review.typo3.org/#/c/42542/

[neos-bot]

Comment created by @bwaidelich:

Sorry, I won't be able to look into this one in the near future. But can anyone check whether https://review.typo3.org/42542 would solve this?

[neos-bot]

Comment created by @albe:

In the PR of the linked issue I also discovered that this behaviour is actually currently expected and tested for, but feels wrong.

See neos#210 (comment)