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 all commits
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
115 changes: 65 additions & 50 deletions DependencyInjection/CmfRoutingExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,12 @@ 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
$this->setupDynamicRouter($config['dynamic'], $container, $loader);
$this->setupMetadataDrivers($config['dynamic'], $container);
}

/* set up the chain router */
Expand All @@ -53,6 +55,52 @@ public function load(array $configs, ContainerBuilder $container)
$this->setupFormTypes($config, $container, $loader);
}

public function setupMetadataDrivers($config, ContainerBuilder $container)
{
$configDriver = $container->getDefinition($this->getAlias() . '.metadata.driver.configuration');
$annotationDriver = $container->getDefinition($this->getAlias() . '.metadata.driver.annotation');

$mapping = array();

$controller = $container->getParameter($this->getAlias() . '.generic_controller');;

// configure annotation driver
$annotationDriver->addMethodCall('setGenericController', array($controller));

$mappings = array();

// configure configuration driver
if (!empty($config['controllers_by_class'])) {
foreach ($config['controllers_by_class'] as $classFqn => $controller) {
if (!isset($mappings[$classFqn])) {
$mappings[$classFqn] = array();
}

$mappings[$classFqn]['controller'] = $controller;
}
}

if (!empty($config['templates_by_class'])) {
foreach ($config['templates_by_class'] as $classFqn => $template) {
if (!isset($mappings[$classFqn])) {
$mappings[$classFqn] = array();
}

$mappings[$classFqn]['template'] = $template;
}
}

foreach ($mappings as $classFqn => &$mapping) {
if (!isset($mapping['controller'])) {
$mapping['controller'] = $container->getParameter(
$this->getAlias() . '.generic_controller'
);
}

$configDriver->addMethodCall('registerMapping', array($classFqn, $mapping));
Copy link
Member Author

Choose a reason for hiding this comment

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

The the validation for this mapping will be triggered the first time the configuration driver is instantiated. So it is not as early as you might want, but it the validation is eager and not lazy. See the ConfigurationDriver.

Copy link
Member

Choose a reason for hiding this comment

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

it still means we have to run the validation at runtime rather than compile time, which imho sucks. can't we keep validating 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.

it would, I think, mean doing the same validation in two places. I guess one way around this problem would be to listen to a cache warming event or similar, that way drivers which are capable of validating their mapped objects (e.g. DI driver, XML, YAML and annotations when possible) can do so eagerly?

}
}

public function setupFormTypes(array $config, ContainerBuilder $container, LoaderInterface $loader)
{
$loader->load('form-type.xml');
Expand All @@ -76,7 +124,7 @@ public function setupFormTypes(array $config, ContainerBuilder $container, Loade
private function setupDynamicRouter(array $config, ContainerBuilder $container, LoaderInterface $loader)
{
// strip whitespace (XML support)
foreach (array('controllers_by_type', 'controllers_by_class', 'templates_by_class', 'route_filters_by_id') as $option) {
foreach (array('controllers_by_type', 'route_filters_by_id') as $option) {
$config[$option] = array_map(function ($value) {
return trim($value);
}, $config[$option]);
Expand All @@ -86,11 +134,10 @@ private function setupDynamicRouter(array $config, ContainerBuilder $container,
if (null === $defaultController) {
$defaultController = $config['generic_controller'];
}

$container->setParameter($this->getAlias() . '.default_controller', $defaultController);
$container->setParameter($this->getAlias() . '.generic_controller', $config['generic_controller']);
$container->setParameter($this->getAlias() . '.controllers_by_type', $config['controllers_by_type']);
$container->setParameter($this->getAlias() . '.controllers_by_class', $config['controllers_by_class']);
$container->setParameter($this->getAlias() . '.templates_by_class', $config['templates_by_class']);
$container->setParameter($this->getAlias() . '.uri_filter_regexp', $config['uri_filter_regexp']);
$container->setParameter($this->getAlias() . '.route_collection_limit', $config['route_collection_limit']);

Expand Down Expand Up @@ -150,55 +197,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
)
);

$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
84 changes: 84 additions & 0 deletions Enhancer/FieldByClassEnhancer.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
<?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();
if (null !== $meta->{$this->metaField}) {
$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;
}
23 changes: 23 additions & 0 deletions Metadata/Driver/AbstractDriver.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?php

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

use Metadata\Driver\DriverInterface;
use Symfony\Cmf\Bundle\RoutingBundle\Metadata\ClassMetadata;

abstract class AbstractDriver implements DriverInterface
{
public function validateMetadata(ClassMetadata $meta)
{
if (
null !== $meta->template &&
null == $meta->controller
) {
throw new \InvalidArgumentException(sprintf(
'Template specified for class "%s" but no controller specified.' .PHP_EOL.
'You must either explicitly map a controller to the class or define'.PHP_EOL.
'a "generic_controller" in the main configuration.'
, $meta->name));
}
}
}
56 changes: 56 additions & 0 deletions Metadata/Driver/AnnotationDriver.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
<?php

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

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

class AnnotationDriver extends AbstractDriver
{
protected $reader;
protected $genericController;

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

public function setGenericController($genericController)
{
$this->genericController = $genericController;
}

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;
}

if (null === $meta->controller) {
$meta->controller = $this->genericController;
}

$this->validateMetadata($meta);
Copy link
Member Author

Choose a reason for hiding this comment

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

AnnotationDriver validates lazily


return $meta;
}
}
Loading