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

[POC] Metadata and Annotation support #209

Open
wants to merge 3 commits into
base: 3.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 16 additions & 47 deletions DependencyInjection/CmfRoutingExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ public function load(array $configs, ContainerBuilder $container)
{
$config = $this->processConfiguration(new Configuration(), $configs);
$loader = new XmlFileLoader($container, new FileLocator(__DIR__.'/../Resources/config'));
$loader->load('metadata.xml');

if ($config['dynamic']['enabled']) {
// load this even if no explicit enabled value but some configuration
Expand Down Expand Up @@ -150,55 +151,23 @@ private function setupDynamicRouter(array $config, ContainerBuilder $container,
)
);
}
if (!empty($config['controllers_by_class'])) {
$dynamic->addMethodCall(
'addRouteEnhancer',
array(
new Reference($this->getAlias() . '.enhancer.controllers_by_class'),
50
)
);
}
if (!empty($config['templates_by_class'])) {
$dynamic->addMethodCall(
'addRouteEnhancer',
array(
new Reference($this->getAlias() . '.enhancer.templates_by_class'),
40
)
);

/*
* The CoreBundle prepends the controller from ContentBundle if the
* ContentBundle is present in the project.
* If you are sure you do not need a generic controller, set the field
* to false to disable this check explicitly. But you would need
* something else like the default_controller to set the controller,
* as no controller will be set here.
*/
if (null === $config['generic_controller']) {
throw new InvalidConfigurationException('If you want to configure templates_by_class, you need to configure the generic_controller option.');
}
$dynamic->addMethodCall(
'addRouteEnhancer',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So here we always add the template and controller enhancers, as the new, metadata aware, FieldByClass enhancer asks the factory directly for metadata. We do not know at this stage if the enhancers will be needed or not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we not let the compiler pass for the metadata add the enhancers as needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need to add the enhancers for the AnnotationDriver to work, as it is lazy. The overhead with the ConfigurationDriver however is just a key check in an associative array and it will be possible to disable annotations.

Would it make sense to have a single MetadataEnhancer class instead of instantiating a FieldByClass enhancer for each case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

single MetadataEnhancer: i would say so, as the metadata thing does not need to be super extensible. it should probably run first of the built in enhancer (it will be more specific rules than global rules).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for registering the enhancers, why not check if either we have configuration or have the annotations active and add if at least one of the two is true? that would be the smallest possible change in behaviour.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we not just register a single MetadataEnhancer in a compiler pass if any metadata driver classes are used? I guess by default we would have a chain driver with the proposed DIConfigDriver in first place, followed by AnnotationsDriver, so correspondingly the metadata enhancer would, by default, be registered and would be the only enhancer registered here.

array(
new Reference($this->getAlias() . '.enhancer.controllers_by_class'),
50
)
);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to re-add this validation in the metadata layer.

$dynamic->addMethodCall(
'addRouteEnhancer',
array(
new Reference($this->getAlias() . '.enhancer.templates_by_class'),
40
)
);

if (is_string($config['generic_controller'])) {
// if the content class defines the template, we also need to make sure we use the generic controller for those routes
$controllerForTemplates = array();
foreach ($config['templates_by_class'] as $key => $value) {
$controllerForTemplates[$key] = $config['generic_controller'];
}

$definition = $container->getDefinition($this->getAlias() . '.enhancer.controller_for_templates_by_class');
$definition->replaceArgument(2, $controllerForTemplates);

$dynamic->addMethodCall(
'addRouteEnhancer',
array(
new Reference($this->getAlias() . '.enhancer.controller_for_templates_by_class'),
30
)
);
}
}
if (!empty($config['generic_controller']) && $config['generic_controller'] !== $defaultController) {
$dynamic->addMethodCall(
'addRouteEnhancer',
Expand Down
82 changes: 82 additions & 0 deletions Enhancer/FieldByClassEnhancer.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
<?php

namespace Symfony\Cmf\Bundle\RoutingBundle\Enhancer;

use Symfony\Component\HttpFoundation\Request;
use Metadata\MetadataFactoryInterface;
use Symfony\Cmf\Component\Routing\Enhancer\RouteEnhancerInterface;

/**
* This enhancer performs the same role as its counterpart
* in the Routing component, but it uses the Metadata factory.
*
* @author Daniel Leech <[email protected]>
*/
class FieldByClassEnhancer implements RouteEnhancerInterface
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class is a new implementation which uses the Metadata factory.

{
/**
* @var string field for the className class
*/
protected $classField;

/**
* @var string field to write hashmap lookup result into
*/
protected $targetField;

/**
* @var string field to write hashmap lookup result into
*/
protected $metaField;

/**
* @var Metadata\MetadataFactoryInterface
*/
protected $metadataFactory;

/**
* @param string $className the field name of the class
* @param string $targetField the field name to set from the map
* @param array $map the map of class names to field values
*/
public function __construct(
$classField,
$targetField,
$metaField,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

targetField = field in $defaults, metaField = field in the Metadata class to get the value from .. does it make sense to specify them individually?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, should the metadata not all be evaluated when the container is built? i would have expected that this metadata stuff is never used at runtime - thats potentially very expensive.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the metadata should be "warmed up" like what happens in Doctrine. So there would be no overhead here, in production, for driver implementations which "know" about all of their mapped documents and are primed with a cache. The annotations would stil be evaluated lazily but would use a cached driver.

MetadataFactoryInterface $metadataFactory
)
{
$this->classField = $classField;
$this->targetField = $targetField;
$this->metaField = $metaField;
$this->metadataFactory = $metadataFactory;
}

/**
* If the class name is mapped
*
* {@inheritDoc}
*/
public function enhance(array $defaults, Request $request)
{
if (isset($defaults[$this->targetField])) {
return $defaults;
}

// return if the field containg the class name has not been set
if (! isset($defaults[$this->classField])) {
return $defaults;
}

$className = get_class($defaults[$this->classField]);

$meta = $this->metadataFactory->getMetadataForClass($className);

if (null !== $meta) {
$meta = $meta->getOutsideClassMetadata();
$defaults[$this->targetField] = $meta->{$this->metaField};
}

return $defaults;
}
}
11 changes: 11 additions & 0 deletions Metadata/Annotations/Controller.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<?php

namespace Symfony\Cmf\Bundle\RoutingBundle\Metadata\Annotations;

/**
* @Annotation
*/
class Controller
{
public $controller;
}
11 changes: 11 additions & 0 deletions Metadata/Annotations/Template.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<?php

namespace Symfony\Cmf\Bundle\RoutingBundle\Metadata\Annotations;

/**
* @Annotation
*/
class Template
{
public $resource;
}
11 changes: 11 additions & 0 deletions Metadata/ClassMetadata.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<?php

namespace Symfony\Cmf\Bundle\RoutingBundle\Metadata;

use Metadata\ClassMetadata as BaseClassMetadata;

class ClassMetadata extends BaseClassMetadata
{
public $template;
public $controller;
}
45 changes: 45 additions & 0 deletions Metadata/Driver/AnnotationDriver.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
<?php

namespace Symfony\Cmf\Bundle\RoutingBundle\Metadata\Driver;

use Metadata\Driver\DriverInterface;
use Symfony\Cmf\Bundle\RoutingBundle\Metadata\ClassMetadata;
use Doctrine\Common\Annotations\Reader;

class AnnotationDriver implements DriverInterface
{
protected $reader;

/**
* @param AnnotationReader $reader
*/
public function __construct(Reader $reader)
{
$this->reader = $reader;
}

public function loadMetadataForClass(\ReflectionClass $class)
{
$templateAnnotation = $this->reader->getClassAnnotation(
$class,
'Symfony\Cmf\Bundle\RoutingBundle\Metadata\Annotations\Template'
);
$controllerAnnotation = $this->reader->getClassAnnotation(
$class,
'Symfony\Cmf\Bundle\RoutingBundle\Metadata\Annotations\Controller'
);


$meta = new ClassMetadata($class->name);

if (null !== $templateAnnotation) {
$meta->template = $templateAnnotation->resource;
}

if (null !== $controllerAnnotation) {
$meta->controller = $controllerAnnotation->controller;
}

return $meta;
}
}
37 changes: 37 additions & 0 deletions Resources/config/metadata.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
<?xml version="1.0" encoding="UTF-8"?>
<container xmlns="http://symfony.com/schema/dic/services"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">

<services>
<!-- Factory -->
<service
id="cmf_routing.metadata.factory"
class="Metadata\MetadataFactory"
>
<argument
type="service"
id="cmf_routing.metadata.driver.chain"
/>
</service>

<!-- Chain Driver !-->
<service
id="cmf_routing.metadata.driver.chain"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I prefer using cmf_routing.metadata.driver here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I think cmf_routing.metadata.driver should be an alias to whatever the driver actually is. So we should still have this definition, e.g. in symfony:

<service id="twig.loader" alias="twig.loader.filesystem" />

class="Metadata\Driver\DriverChain"
>
<argument type="collection">
<argument type="service" id="cmf_routing.metadata.driver.annotation"/>
</argument>
</service>

<!-- Annotations Driver -->
<service
id="cmf_routing.metadata.driver.annotation"
class="Symfony\Cmf\Bundle\RoutingBundle\Metadata\Driver\AnnotationDriver"
>
<argument type="service" id="annotation_reader"/>
<argument type="collection"/>
</service>
</services>
</container>
11 changes: 7 additions & 4 deletions Resources/config/routing-dynamic.xml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
<parameter key="cmf_routing.enhancer.default_controller.class">Symfony\Cmf\Component\Routing\Enhancer\FieldPresenceEnhancer</parameter>
<parameter key="cmf_routing.enhancer.explicit_template.class">Symfony\Cmf\Component\Routing\Enhancer\FieldPresenceEnhancer</parameter>
<parameter key="cmf_routing.enhancer.controllers_by_type.class">Symfony\Cmf\Component\Routing\Enhancer\FieldMapEnhancer</parameter>
<parameter key="cmf_routing.enhancer.field_by_class.class">Symfony\Cmf\Component\Routing\Enhancer\FieldByClassEnhancer</parameter>
<parameter key="cmf_routing.enhancer.field_by_class.class">Symfony\Cmf\Bundle\RoutingBundle\Enhancer\FieldByClassEnhancer</parameter>
<parameter key="cmf_routing.redirect_controller.class">Symfony\Cmf\Bundle\RoutingBundle\Controller\RedirectController</parameter>
</parameters>

Expand Down Expand Up @@ -46,19 +46,22 @@
<service id="cmf_routing.enhancer.controllers_by_class" class="%cmf_routing.enhancer.field_by_class.class%" public="false">
<argument>_content</argument>
<argument>_controller</argument>
<argument>%cmf_routing.controllers_by_class%</argument>
<argument>controller</argument>
<argument type="service" id="cmf_routing.metadata.factory"/>
</service>

<service id="cmf_routing.enhancer.controller_for_templates_by_class" class="%cmf_routing.enhancer.field_by_class.class%" public="false">
<argument>_content</argument>
<argument>_controller</argument>
<argument type="collection" />
<argument>controller</argument>
<argument type="service" id="cmf_routing.metadata.factory"/>
</service>

<service id="cmf_routing.enhancer.templates_by_class" class="%cmf_routing.enhancer.field_by_class.class%" public="false">
<argument>_content</argument>
<argument>_template</argument>
<argument>%cmf_routing.templates_by_class%</argument>
<argument>template</argument>
<argument type="service" id="cmf_routing.metadata.factory"/>
</service>

<service id="cmf_routing.dynamic_router" class="%cmf_routing.dynamic_router.class%">
Expand Down
44 changes: 44 additions & 0 deletions Tests/Functional/Metadata/Driver/AnnotationDriverTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
<?php

/*
* This file is part of the Symfony CMF package.
*
* (c) 2011-2013 Symfony CMF
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/


namespace Symfony\Cmf\Bundle\RoutingBundle\Tests\Functional\Metadata\Driver;

use Symfony\Component\HttpFoundation\RedirectResponse;

use Symfony\Cmf\Bundle\RoutingBundle\Doctrine\Phpcr\Route;
use Symfony\Cmf\Bundle\RoutingBundle\Doctrine\Phpcr\RedirectRoute;
use Symfony\Cmf\Bundle\RoutingBundle\Controller\RedirectController;
use Symfony\Cmf\Bundle\RoutingBundle\Tests\Functional\BaseTestCase;

class AnnotationDriverTest extends BaseTestCase
{
public function setUp()
{
parent::setUp();

$this->driver = $this->getContainer()->get('cmf_routing.metadata.factory');
}

public function testDriver()
{
$meta = $this->driver->getMetadataForClass(
'Symfony\Cmf\Bundle\RoutingBundle\Tests\Resources\Document\AnnotatedContent'
);

$this->assertNotNull($meta);

$meta = $meta->getOutsideClassMetadata();

$this->assertEquals('TestBundle:Content:index.html.twig', $meta->template);
$this->assertEquals('ThisIsAController', $meta->controller);
}
}
3 changes: 2 additions & 1 deletion Tests/Functional/Routing/DynamicRouterTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

use Symfony\Cmf\Bundle\RoutingBundle\Tests\Functional\BaseTestCase;
use Symfony\Cmf\Component\Testing\Document\Content;
use Symfony\Cmf\Bundle\RoutingBundle\Tests\Resources\Document\AnnotatedContent;

/**
* The goal of these tests is to test the interoperation with DI and everything.
Expand Down Expand Up @@ -310,7 +311,7 @@ public function testEnhanceTemplateByClass()
$this->getDm()->flush();
}
NodeHelper::createPath($this->getDm()->getPhpcrSession(), '/test/content');
$document = new Content();
$document = new AnnotatedContent();
$document->setId('/test/content/templatebyclass');
$document->setTitle('the title');
$this->getDm()->persist($document);
Expand Down
Loading