Merge pull request #337 from RunOpenCode/bugfix/335-fix-false-matchin…

…g-class-non-declared-methods-and-properties

Exclude abstract methods from pointcut matching #335

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
+                if ($method->isAbstract()) {
+                    continue;
+                }
+
                 if ($filter->matches($method, $class)) {
                     $prefix = $method->isStatic() ? AspectContainer::STATIC_METHOD_PREFIX : AspectContainer::METHOD_PREFIX;
                     $classAdvices[$prefix][$method->name][$advisorId] = $advisor->getAdvice();

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

@@ -0,0 +1,13 @@
+<?php
+
+namespace Go\Tests\TestProject\Application;
+
+abstract class AbstractBar implements FooInterface
+{
+    abstract public function doSomethingElse();
+
+    public function doSomeThirdThing()
+    {
+        echo 'I did some third thing';
+    }
+}

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

@@ -0,0 +1,8 @@
+<?php
+
+namespace Go\Tests\TestProject\Application;
+
+interface FooInterface
+{
+    public function doSomething();
+}

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

@@ -4,13 +4,18 @@
 
 use Go\Tests\TestProject\Annotation as Aop;
 
-class Main
+class Main extends AbstractBar
 {
     /**
      * @Aop\Loggable()
      */
     public function doSomething()
     {
+        echo 'I did something';
+    }
 
+    public function doSomethingElse()
+    {
+        echo 'I did something else';
     }
 }

tests/Fixtures/project/src/ApplicationAspectKernel.php

@@ -4,6 +4,7 @@
 
 use Go\Core\AspectContainer;
 use Go\Core\AspectKernel;
+use Go\Tests\TestProject\Aspect\DoSomethingAspect;
 use Go\Tests\TestProject\Aspect\LoggingAspect;
 use Psr\Log\NullLogger;
 
@@ -19,5 +20,6 @@ class ApplicationAspectKernel extends AspectKernel
     protected function configureAop(AspectContainer $container)
     {
         $container->registerAspect(new LoggingAspect(new NullLogger()));
+        $container->registerAspect(new DoSomethingAspect());
     }
 }

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

@@ -0,0 +1,22 @@
+<?php
+
+namespace Go\Tests\TestProject\Aspect;
+
+use Go\Aop\Aspect;
+use Go\Aop\Intercept\MethodInvocation;
+use Go\Lang\Annotation as Pointcut;
+
+class DoSomethingAspect implements Aspect
+{
+    /**
+     * Intercepts doSomething()
+     *
+     * @param MethodInvocation $invocation
+     *
+     * @Pointcut\After("execution(public Go\Tests\TestProject\Application\*->doSomething(*)) || execution(public Go\Tests\TestProject\Application\*->doSomethingElse(*))")
+     */
+    public function afterDoSomething()
+    {
+        echo 'It does something else after something is done.';
+    }
+}

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

@@ -4,7 +4,7 @@
 
 use Go\Aop\Aspect;
 use Go\Aop\Intercept\MethodInvocation;
-use Go\Lang\Annotation\Before;
+use Go\Lang\Annotation as Pointcut;
 use Psr\Log\LoggerInterface;
 
 /**
@@ -26,7 +26,8 @@ public function __construct(LoggerInterface $logger)
      * Writes a log info before method execution
      *
      * @param MethodInvocation $invocation
-     * @Before("@execution(Go\Tests\TestProject\Annotation\Loggable)")
+     *
+     * @Pointcut\Before("@execution(Go\Tests\TestProject\Annotation\Loggable)")
      */
     public function beforeMethod(MethodInvocation $invocation)
     {

tests/Go/Console/Command/CacheWarmupCommandTest.php

@@ -2,37 +2,22 @@
 
 namespace Go\Console\Command;
 
-use PHPUnit_Framework_TestCase as TestCase;
-use Symfony\Component\Filesystem\Filesystem;
-use Symfony\Component\Process\Process;
+use Go\Functional\BaseFunctionalTest;
 
-class CacheWarmupCommandTest extends TestCase
+class CacheWarmupCommandTest extends BaseFunctionalTest
 {
-    protected $aspectCacheDir = __DIR__.'/../../../Fixtures/project/var/cache/aspect';
-
     public function setUp()
     {
-        $filesystem = new Filesystem();
-
-        if ($filesystem->exists($this->aspectCacheDir)) {
-            $filesystem->remove($this->aspectCacheDir);
-        }
+        self::clearCache();
     }
 
     public function testItWarmsUpCache()
     {
-        $this->assertFalse(file_exists($this->aspectCacheDir));
-
-        $process = new Process(sprintf('php %s cache:warmup:aop %s',
-            realpath(__DIR__.'/../../../Fixtures/project/bin/console'),
-            realpath(__DIR__.'/../../../Fixtures/project/web/index.php')
-        ));
-
-        $process->run();
+        $this->assertFalse(file_exists(self::$aspectCacheDir));
 
-        $this->assertTrue($process->isSuccessful(), 'Unable to execute "cache:warmup:aop" command.');
+        self::warmUp();
 
-        $this->assertTrue(file_exists($this->aspectCacheDir.'/_proxies/src/Application/Main.php'));
-        $this->assertTrue(file_exists($this->aspectCacheDir.'/src/Application/Main.php'));
+        $this->assertTrue(file_exists(self::$aspectCacheDir.'/_proxies/src/Application/Main.php'));
+        $this->assertTrue(file_exists(self::$aspectCacheDir.'/src/Application/Main.php'));
     }
 }

tests/Go/Console/Command/DebugAdvisorCommandTest.php

@@ -2,35 +2,19 @@
 
 namespace Go\Console\Command;
 
-use PHPUnit_Framework_TestCase as TestCase;
-use Symfony\Component\Process\Process;
+use Go\Functional\BaseFunctionalTest;
+use Go\Instrument\FileSystem\Enumerator;
 
-class DebugAdvisorCommandTest extends TestCase
+class DebugAdvisorCommandTest extends BaseFunctionalTest
 {
     public function setUp()
     {
-        $process = new Process(sprintf('php %s cache:warmup:aop %s',
-            realpath(__DIR__.'/../../../Fixtures/project/bin/console'),
-            realpath(__DIR__.'/../../../Fixtures/project/web/index.php')
-        ));
-
-        $process->run();
-
-        $this->assertTrue($process->isSuccessful(), 'Unable to execute "cache:warmup:aop" command.');
+        self::warmUp();
     }
 
     public function testItDisplaysAdvisorsDebugInfo()
     {
-        $process = $process = new Process(sprintf('php %s debug:advisor %s',
-            realpath(__DIR__.'/../../../Fixtures/project/bin/console'),
-            realpath(__DIR__.'/../../../Fixtures/project/web/index.php')
-        ));
-
-        $process->run();
-
-        $this->assertTrue($process->isSuccessful(), 'Unable to execute "debug:advisor" command.');
-
-        $output = $process->getOutput();
+        $output = self::exec('debug:advisor');
 
         $expected = [
             'List of registered advisors in the container',
@@ -45,19 +29,13 @@ public function testItDisplaysAdvisorsDebugInfo()
 
     public function testItDisplaysStatedAdvisorDebugInfo()
     {
-        $process = $process = new Process(sprintf('php %s debug:advisor %s --advisor="Go\Tests\TestProject\Aspect\LoggingAspect->beforeMethod"',
-            realpath(__DIR__.'/../../../Fixtures/project/bin/console'),
-            realpath(__DIR__.'/../../../Fixtures/project/web/index.php')
-        ));
-
-        $process->run();
-
-        $this->assertTrue($process->isSuccessful(), 'Unable to execute "debug:advisor" command.');
+        $output = self::exec('debug:advisor', '--advisor="Go\Tests\TestProject\Aspect\LoggingAspect->beforeMethod"');
 
-        $output = $process->getOutput();
+        $enumerator = new Enumerator(realpath(self::$projectDir.'/src'));
+        $srcFilesCount = iterator_count($enumerator->enumerate());
 
         $expected = [
-            'Total 4 files to analyze.',
+            sprintf('Total %s files to analyze.', $srcFilesCount),
             '-> matching method Go\Tests\TestProject\Application\Main->doSomething',
         ];
 

tests/Go/Console/Command/DebugAspectCommandTest.php

@@ -2,35 +2,18 @@
 
 namespace Go\Console\Command;
 
-use PHPUnit_Framework_TestCase as TestCase;
-use Symfony\Component\Process\Process;
+use Go\Functional\BaseFunctionalTest;
 
-class DebugAspectCommandTest extends TestCase
+class DebugAspectCommandTest extends BaseFunctionalTest
 {
     public function setUp()
     {
-        $process = new Process(sprintf('php %s cache:warmup:aop %s',
-            realpath(__DIR__.'/../../../Fixtures/project/bin/console'),
-            realpath(__DIR__.'/../../../Fixtures/project/web/index.php')
-        ));
-
-        $process->run();
-
-        $this->assertTrue($process->isSuccessful(), 'Unable to execute "cache:warmup:aop" command.');
+        self::warmUp();
     }
 
     public function testItDisplaysAspectsDebugInfo()
     {
-        $process = $process = new Process(sprintf('php %s debug:aspect %s',
-            realpath(__DIR__.'/../../../Fixtures/project/bin/console'),
-            realpath(__DIR__.'/../../../Fixtures/project/web/index.php')
-        ));
-
-        $process->run();
-
-        $this->assertTrue($process->isSuccessful(), 'Unable to execute "debug:aspect" command.');
-
-        $output = $process->getOutput();
+        $output = self::exec('debug:aspect');
 
         $expected = [
             'Go\Tests\TestProject\ApplicationAspectKernel has following enabled aspects',

tests/Go/Functional/BaseFunctionalTest.php

@@ -0,0 +1,47 @@
+<?php
+
+namespace Go\Functional;
+
+use PHPUnit_Framework_TestCase as TestCase;
+use Symfony\Component\Filesystem\Filesystem;
+use Symfony\Component\Process\Process;
+
+abstract class BaseFunctionalTest extends TestCase
+{
+    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';
+
+    protected static function clearCache()
+    {
+        $filesystem = new Filesystem();
+
+        if ($filesystem->exists(self::$aspectCacheDir)) {
+            $filesystem->remove(self::$aspectCacheDir);
+        }
+    }
+
+    protected static function warmUp()
+    {
+        return self::exec('cache:warmup:aop');
+    }
+
+    protected static function exec($command, $args = '')
+    {
+        $commandStatement = sprintf('php %s %s %s %s',
+            self::$consolePath,
+            $command,
+            self::$frontControllerPath,
+            $args
+        );
+
+        $process = new Process($commandStatement);
+
+        $process->run();
+
+        self::assertTrue($process->isSuccessful(), sprintf('Unable to execute "%s" command.', $command));
+
+        return $process->getOutput();
+    }
+}

tests/Go/Functional/MethodWeavingTest.php

@@ -0,0 +1,25 @@
+<?php
+
+namespace Go\Functional;
+
+class MethodWeavingTest extends BaseFunctionalTest
+{
+    public function setUp()
+    {
+        self::warmUp();
+    }
+
+    /**
+     * test for https://github.com/goaop/framework/issues/335
+     */
+    public function testItDoesNotWeaveAbstractMethods()
+    {
+        // it weaves Main class
+        $this->assertTrue(file_exists(self::$aspectCacheDir.'/_proxies/src/Application/Main.php'));
+        $this->assertTrue(file_exists(self::$aspectCacheDir.'/src/Application/Main.php'));
+
+        // it does not weaves AbstractBar class
+        $this->assertFalse(file_exists(self::$aspectCacheDir.'/_proxies/src/Application/AbstractBar.php'));
+        $this->assertFalse(file_exists(self::$aspectCacheDir.'/src/Application/AbstractBar.php'));
+    }
+}