Skip to content

Commit

Permalink
[Routing] fixed annotation loaders for abstract classes, added more u…
Browse files Browse the repository at this point in the history 
…nit tests
  • Loading branch information
@fabpot
fabpot committed Aug 30, 2011
1 parent 6a81f07 commit 946ccb6
Show file tree
Hide file tree
Showing 8 changed files with 125 additions and 29 deletions.
4 changes: 4 additions & 0 deletions src/Symfony/Component/Routing/Loader/AnnotationClassLoader.php
View file
Expand Up @@ -105,6 +105,10 @@ public function load($class, $type = null)
);

$class = new \ReflectionClass($class);
if ($class->isAbstract()) {

This comment has been minimized.

Sign in to view

Copy link
@schmittjoh

schmittjoh Sep 12, 2011

Contributor

This actually breaks some code, why was this necessary?

This comment has been minimized.

Sign in to view

Copy link
@schmittjoh

schmittjoh Sep 12, 2011

Contributor

I think we should add a @controller annotation to mark the classes from which routes should be parsed.

If you agree, I can make a PR for it.

This comment has been minimized.

Sign in to view

Copy link
@fabpot

fabpot Sep 13, 2011

Author Member

What does it break? Problem was that Doctrine caches the abstract annotations and reused them for all classes extending the abstract class. Adding a @Controller annotation is not an option for the 2.0 branch.

This comment has been minimized.

Sign in to view

Copy link
@schmittjoh

schmittjoh Sep 13, 2011

Contributor

If your controller resolver can handle abstract classes (because it knows how to implement the remaining methods), then these controllers don't work anymore. For an example, you can see the JMSDiExtraBundle which implements some lookup methods automatically.

This comment has been minimized.

Sign in to view

Copy link
@schmittjoh

schmittjoh Sep 18, 2011

Contributor

Any news on this? Should we revert this commit in the 2.0 branch?

This comment has been minimized.

Sign in to view

Copy link
@fabpot

fabpot Sep 19, 2011

Author Member

An abstract class can never be a controller; you must at least extends it to make it usable by PHP. So, I still don't see the problem.

This comment has been minimized.

Sign in to view

Copy link
@schmittjoh

schmittjoh Sep 19, 2011

Contributor

Here's the workflow:

  • Route annotations are read from abstract controller class
  • Request matches abstract controller -> ControllerResolver is invoked
  • ControllerResolver loads metadata for abstract controller
  • ControllerResolver sees that it needs to implement some methods -> proxy class is generated
  • ControllerResolver instantiates proxy class which enhances the abstract controller

An example is this:

<?php

use JMS\DiExtraBundle\Annotation as DI;
use Sensio\Bundle\FrameworkExtraBundle\Configuration\Route;

/** This is the code that is written by hand */
abstract class MyController
{
    /** @Route("/signup") */
    public function myAction()
    {
        // create form etc.
        if ($registrationSuccessful) {
            $this->getMailer()->send($something);
        }
        // render view, redirect, etc.
    }

    /** @DI\LookupMethod("mailer") */
    abstract protected function getMailer();
}

/** This code is automatically generated */
class AutoGeneratedProxyClass extends MyController
{
    private $container;

    public function setContainer(ContainerInterface $container)
    {
        $this->container = $container;
    }

    protected function getMailer()
    {
        return $this->container->get('mailer');
    }
}

I understand that this is probably a very advanced case, but as it is now this is not working anymore.

This comment has been minimized.

Sign in to view

Copy link
@fabpot

fabpot Sep 22, 2011

Author Member

Now I understand why it is a problem for your specific case, but the patch actually fixes a much more common use case where you have an abstract controller used by several concrete controller classes:

Controller/
    AbstractController.php
    BlogController.php (extends AbstractController)
    ForumController.php (extends AbstractController)

In this case, and if you configure the routing to read all controllers from the Controller\ subnamespace, the routes won't be the right ones.

This comment has been minimized.

Sign in to view

Copy link
@schmittjoh

schmittjoh Sep 22, 2011

Contributor

Yes, I see your case as well, that's why I proposed to be explicit and let the user decided which way he wants it to work (see #2160).

Since this is a behavior change, and might break code that worked before, I think we should revert it in 2.0, and only apply it to 2.1 along with the explicit configuration. If you take a look at my PR, you'll see that this @Controller/@nocontroller annotation is only used on abstract classes, not on regular classes, so the upgrade path should be relative smooth.

This comment has been minimized.

Sign in to view

Copy link
@fabpot

fabpot Sep 22, 2011

Author Member

This is a bug fix for 2.0 as the common use case does not work.
throw new \InvalidArgumentException(sprintf('Annotations from class "%s" cannot be read as it is abstract.', $class));
}

if ($annot = $this->reader->getClassAnnotation($class, $this->routeAnnotationClass)) {
if (null !== $annot->getPattern()) {
$globals['pattern'] = $annot->getPattern();
Expand Down
5 changes: 5 additions & 0 deletions src/Symfony/Component/Routing/Loader/AnnotationDirectoryLoader.php
View file
Expand Up @@ -44,6 +44,11 @@ public function load($path, $type = null)
}

if ($class = $this->findClass($file)) {
$refl = new \ReflectionClass($class);
if ($refl->isAbstract()) {
continue;
}

$collection->addCollection($this->loader->load($class, $type));
}
}
Expand Down
7 changes: 7 additions & 0 deletions tests/Symfony/Tests/Component/Routing/Fixtures/AnnotatedClasses/AbstractClass.php
View file
@@ -0,0 +1,7 @@
<?php

namespace Symfony\Tests\Component\Routing\Fixtures\AnnotatedClasses;

abstract class AbstractClass
{
}
7 changes: 7 additions & 0 deletions tests/Symfony/Tests/Component/Routing/Fixtures/AnnotatedClasses/FooClass.php
View file
@@ -0,0 +1,7 @@
<?php

namespace Symfony\Tests\Component\Routing\Fixtures\AnnotatedClasses;

class FooClass
{
}
36 changes: 36 additions & 0 deletions tests/Symfony/Tests/Component/Routing/Loader/AbstractAnnotationLoaderTest.php
View file
@@ -0,0 +1,36 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Tests\Component\Routing\Loader;

use Symfony\Component\Config\Loader\LoaderResolver;
use Symfony\Component\Routing\Loader\AnnotationFileLoader;
use Symfony\Component\Routing\Route;
use Symfony\Component\Routing\RouteCollection;

abstract class AbstractAnnotationLoaderTest extends \PHPUnit_Framework_TestCase
{
public function getReader()
{
return $this->getMockBuilder('Doctrine\Common\Annotations\Reader')
->disableOriginalConstructor()
->getMock()
;
}

public function getClassLoader($reader)
{
return $this->getMockBuilder('Symfony\Component\Routing\Loader\AnnotationClassLoader')
->setConstructorArgs(array($reader))
->getMockForAbstractClass()
;
}
}
26 changes: 19 additions & 7 deletions tests/Symfony/Tests/Component/Routing/Loader/AnnotationClassLoaderTest.php
View file
Expand Up @@ -16,20 +16,32 @@
use Symfony\Component\Routing\Route;
use Symfony\Component\Routing\RouteCollection;

class AnnotationClassLoaderTest extends \PHPUnit_Framework_TestCase
require_once __DIR__.'/../Fixtures/AnnotatedClasses/AbstractClass.php';
require_once __DIR__.'/AbstractAnnotationLoaderTest.php';

class AnnotationClassLoaderTest extends AbstractAnnotationLoaderTest
{
protected $loader;

protected function setUp()
public function setUp()
{
$this->loader = $this->getClassLoader($this->getReader());
}

/**
* @expectedException \InvalidArgumentException
*/
public function testLoadMissingClass()
{
$this->loader = $this->getMockBuilder('Symfony\Component\Routing\Loader\AnnotationClassLoader')
->disableOriginalConstructor()
->getMockForAbstractClass();
$this->loader->load('MissingClass');
}

protected function tearDown()
/**
* @expectedException \InvalidArgumentException
*/
public function testLoadAbstractClass()
{
$this->loader = null;
$this->loader->load('Symfony\Tests\Component\Routing\Fixtures\AnnotatedClasses\AbstractClass');
}

/**
Expand Down
34 changes: 23 additions & 11 deletions tests/Symfony/Tests/Component/Routing/Loader/AnnotationDirectoryLoaderTest.php
View file
Expand Up @@ -17,25 +17,37 @@
use Symfony\Component\Config\Loader\LoaderResolver;
use Symfony\Component\Config\FileLocator;

class AnnotationDirectoryLoaderTest extends \PHPUnit_Framework_TestCase
require_once __DIR__.'/AbstractAnnotationLoaderTest.php';

class AnnotationDirectoryLoaderTest extends AbstractAnnotationLoaderTest
{
protected $loader;
protected $reader;

public function setUp()
{
$this->reader = $this->getReader();
$this->loader = new AnnotationDirectoryLoader(new FileLocator(), $this->getClassLoader($this->reader));
}

public function testLoad()
{
$this->reader->expects($this->once())->method('getClassAnnotation');

$this->loader->load(__DIR__.'/../Fixtures/AnnotatedClasses');
}

/**
* @covers Symfony\Component\Routing\Loader\AnnotationDirectoryLoader::supports
*/
public function testSupports()
{
$annotationClassLoader = $this->getMockBuilder('Symfony\Component\Routing\Loader\AnnotationClassLoader')
->disableOriginalConstructor()
->getMockForAbstractClass();

$loader = new AnnotationDirectoryLoader(new FileLocator(), $annotationClassLoader);

$fixturesDir = __DIR__.'/../Fixtures';

$this->assertTrue($loader->supports($fixturesDir), '->supports() returns true if the resource is loadable');
$this->assertFalse($loader->supports('foo.foo'), '->supports() returns true if the resource is loadable');
$this->assertTrue($this->loader->supports($fixturesDir), '->supports() returns true if the resource is loadable');
$this->assertFalse($this->loader->supports('foo.foo'), '->supports() returns true if the resource is loadable');

$this->assertTrue($loader->supports($fixturesDir, 'annotation'), '->supports() checks the resource type if specified');
$this->assertFalse($loader->supports($fixturesDir, 'foo'), '->supports() checks the resource type if specified');
$this->assertTrue($this->loader->supports($fixturesDir, 'annotation'), '->supports() checks the resource type if specified');
$this->assertFalse($this->loader->supports($fixturesDir, 'foo'), '->supports() checks the resource type if specified');
}
}
35 changes: 24 additions & 11 deletions tests/Symfony/Tests/Component/Routing/Loader/AnnotationFileLoaderTest.php
View file
Expand Up @@ -15,26 +15,39 @@
use Symfony\Component\Routing\Loader\AnnotationFileLoader;
use Symfony\Component\Routing\Route;
use Symfony\Component\Routing\RouteCollection;
use Symfony\Component\Config\FileLocator;

class AnnotationFileLoaderTest extends \PHPUnit_Framework_TestCase
require_once __DIR__.'/AbstractAnnotationLoaderTest.php';

class AnnotationFileLoaderTest extends AbstractAnnotationLoaderTest
{
protected $loader;
protected $reader;

public function setUp()
{
$this->reader = $this->getReader();
$this->loader = new AnnotationFileLoader(new FileLocator(), $this->getClassLoader($this->reader));
}

public function testLoad()
{
$this->reader->expects($this->once())->method('getClassAnnotation');

$this->loader->load(__DIR__.'/../Fixtures/AnnotatedClasses/FooClass.php');
}

/**
* @covers Symfony\Component\Routing\Loader\AnnotationFileLoader::supports
*/
public function testSupports()
{
$annotationClassLoader = $this->getMockBuilder('Symfony\Component\Routing\Loader\AnnotationClassLoader')
->disableOriginalConstructor()
->getMockForAbstractClass();

$loader = new AnnotationFileLoader($this->getMock('Symfony\Component\Config\FileLocator'), $annotationClassLoader);

$fixture = __DIR__.'/../Fixtures/annotated.php';

$this->assertTrue($loader->supports($fixture), '->supports() returns true if the resource is loadable');
$this->assertFalse($loader->supports('foo.foo'), '->supports() returns true if the resource is loadable');
$this->assertTrue($this->loader->supports($fixture), '->supports() returns true if the resource is loadable');
$this->assertFalse($this->loader->supports('foo.foo'), '->supports() returns true if the resource is loadable');

$this->assertTrue($loader->supports($fixture, 'annotation'), '->supports() checks the resource type if specified');
$this->assertFalse($loader->supports($fixture, 'foo'), '->supports() checks the resource type if specified');
$this->assertTrue($this->loader->supports($fixture, 'annotation'), '->supports() checks the resource type if specified');
$this->assertFalse($this->loader->supports($fixture, 'foo'), '->supports() checks the resource type if specified');
}
}

0 comments on commit 946ccb6

Please sign in to comment.