[POC] Metadata and Annotation support #209

dantleech · 2014-01-26T17:58:28Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no (well, yes, but should be BC when finished)
Deprecations?	yes
Tests pass?	?
Fixed tickets
License	MIT
Doc PR	not yet

This PR adds initial support for Annotations:

use Symfony\Cmf\Bundle\RoutingBundle\Metadata\Annotations as CmfRouting;

/**
 * @PHPCRODM\Document(referenceable=true)
 *
 * @CmfRouting\Template("TestBundle:Content:index.html.twig")
 * @CmfRouting\Controller("cmf_content.controller:indexAction")
 */
class AnnotatedContent

For this I have introduced the jms/metadata package.

To maintain BC we would need to implement a DIC driver for the metadata factory.

Things to do:

Add an XML driver (and I guess a YAML driver)
Add the DIC driver to maintain BC

dantleech · 2014-01-26T17:59:20Z

DependencyInjection/CmfRoutingExtension.php

-                )
-            );
-        }
-        if (!empty($config['templates_by_class'])) {


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

wouterj · 2014-01-26T18:40:03Z

-1 on removing the configuration etc from the extension. Imo, the DIC driver should load the config from the DIC, not the config files. It'll be very hard to create a fully BC DIC driver otherwise.

dantleech · 2014-01-26T19:02:47Z

@wouterj the DIC driver will indeed load from the DIC -- not sure how yet. I removed the configuration stuff because the validation would now be handled in the metadata layer and before the enhancers were only added when a mapping was present in the DIC config.

dbu · 2014-01-26T19:25:20Z

DependencyInjection/CmfRoutingExtension.php

-             * 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.');


need to keep this sanity check too.

dbu · 2014-01-26T19:29:15Z

very cool!

i wonder if we should care about the xml and yml (and php) driver or just rely on the container configuration for those. in a separate config file per class, this will be really dispersed all over the place. the annotation makes sense, but for the others i think a central configuration makes more sense. wdyt?

wouterj · 2014-01-26T21:29:11Z

Yeah, I tend to agree with @dbu Maybe having the other formats in here doesn't make much sense.

wouterj · 2014-01-26T21:31:43Z

Resources/config/metadata.xml

+
+        <!-- Chain Driver !-->
+        <service
+            id="cmf_routing.metadata.driver.chain"


I think I prefer using cmf_routing.metadata.driver here

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" />

dantleech · 2014-01-26T21:48:25Z

@wouterj @dbu re. the XML, YAML etc, I imagined supporting XML and YAML but its not important I guess - as long as we support the DIC and Annotations then others can be easily added if needed.

dbu · 2014-01-27T07:17:16Z

cool. if somebody really wants xml/yml he just can do the PR and if its clean we merge it.

i have somehow the same question as in the other metadata PR: is this not all handled at container compile time? can we not simply upgrade the service configuration with the additional mappings we detect in the compiler pass? or when is the metadata evaluated?

dantleech · 2014-01-27T07:39:30Z

@dbu the "problem" with evaluating all the mapped classes at compile time is knowing where they are .. you would have to have configuration which points to the mapped classes. When they are loaded on demand we avoid this problem.

Doctrine needs to know where ALL the mapped classes are for creating schemas etc. We simplify things and avoid extra configuration by demanding the metadata from the annotation driver at runtime - I think performance problems would be avoided / largely mitigated by employing a cached annotation driver.

So in short, if collect all the data at compile time we need to infer the location of the mapped objects and do some nasty stuff:

https://github.com/doctrine/common/blob/master/lib/Doctrine/Common/Persistence/Mapping/Driver/AnnotationDriver.php#L167

dbu · 2014-01-29T21:28:01Z

Ok, i see the point. What if you instead just add a new MetadataEnhancer and change nothing on the existing impl? Then one could switch the feature off for max performance, and the code would probably be more readable

dbu · 2014-02-01T11:10:30Z

what aobut the MetadataEnhancer idea?

dantleech · 2014-02-01T14:59:19Z

@dbu the bundle's FieldByClassEnhancer is that MetadataEnhancer. I agree that we should provide a flag or something to switch annotation driver on and off, but I think we should always have the metadata aware enhancer.

The AnnotationDriver would be complemented by a ContainerDriver which would load the config from the DI like we do now. We would just add or remove the annotation driver from the chain driver. wdyt?

dbu · 2014-02-02T10:41:07Z

what would the MetadataEnhancer do if there is no annotation driver?

what would it mean to have a ContainerDriver, when would it be executed? i would really want to keep it in the DI extension to have early notices of mistakes and to have optimal performance with the configuration compiled right into the cache in prod mode. would it work that way?

dantleech · 2014-02-02T14:24:57Z

@dbu the MetadataEnhancer would use the chain driver, initially with both ContainerDriver and AnnotationDriver. I think the ContainerDriver would indeed validate at compile time (as could the AnnotationDriver driver if the document locations are explicitly defined - but this is an optimization).

dantleech · 2014-02-02T16:19:28Z

DependencyInjection/CmfRoutingExtension.php

+                );
+            }
+
+            $configDriver->addMethodCall('registerMapping', array($classFqn, $mapping));


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.

it still means we have to run the validation at runtime rather than compile time, which imho sucks. can't we keep validating here?

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?

dantleech · 2014-02-02T16:26:47Z

Updated. added a ConfiguationDriver and validation. Although as noted, the validation is done eager and early rather than at compile time.

dantleech · 2014-02-06T20:13:56Z

I guess this is not going to be in 1.1

bigfoot90 · 2014-11-19T01:27:21Z

What is the state of this?

dantleech · 2014-11-19T07:31:40Z

tbh I think we have forgotten about this, maybe it wouldn't be too much work to get it in to the next release //cc @dbu

dbu · 2014-11-19T16:32:24Z

yeah, i guess its rather close to being finished. i would want to make it possible to completely disable the feature for performance reasons, but annotations are a nice way for RAD and the xml/yml config can allow a bundle to provide a default configuration how it wants to be handled. with that regard, i think config in the application config should have precedence over annotations or config files by bundles...

@dantleech do you want to rebase this on current master and then we check what is left to do?

dbu · 2015-04-10T16:20:32Z

ping @dantleech

dantleech · 2015-04-10T16:24:46Z

Would be great, but don't know if I will have time in the near future..

dbu · 2016-06-14T07:33:54Z

should we drop this one?

dantleech · 2016-06-14T07:40:55Z

I think it would be a great feature. But we should try and do it for all bundles in the same way at the same time. But its still not something very high on my list, I don't see any harm in leaving it open as a reference / reminder?

dbu · 2016-06-14T10:58:45Z

okay, no harm in keeping it open if its still relevant.

dantleech reviewed Jan 26, 2014
View reviewed changes

First pass for adding metadata + annotations

cbe8661

dbu reviewed Jan 26, 2014
View reviewed changes

wouterj reviewed Jan 26, 2014
View reviewed changes

dantleech added 2 commits February 2, 2014 16:02

Added metadata driver: ConfigurationDriver

3b1ce3e

Added tests for class metadata validation

8f50119

dantleech reviewed Feb 2, 2014
View reviewed changes

dbu removed this from the 1.2 milestone Apr 25, 2014

lsmith77 added the in progress label Aug 15, 2014

dbu added DX feature labels Nov 19, 2014

dantleech self-assigned this Mar 11, 2015

lsmith77 added ready and removed in progress labels May 1, 2015

lsmith77 added the wip/poc label May 11, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[POC] Metadata and Annotation support #209

[POC] Metadata and Annotation support #209

dantleech commented Jan 26, 2014

dantleech Jan 26, 2014

wouterj commented Jan 26, 2014

dantleech commented Jan 26, 2014

dbu Jan 26, 2014

dbu commented Jan 26, 2014

wouterj commented Jan 26, 2014

wouterj Jan 26, 2014

dantleech Feb 23, 2014

dantleech commented Jan 26, 2014

dbu commented Jan 27, 2014

dantleech commented Jan 27, 2014

dbu commented Jan 29, 2014

dbu commented Feb 1, 2014

dantleech commented Feb 1, 2014

dbu commented Feb 2, 2014

dantleech commented Feb 2, 2014

dantleech Feb 2, 2014

dbu Feb 2, 2014

dantleech Feb 23, 2014

dantleech commented Feb 2, 2014

dantleech commented Feb 6, 2014

bigfoot90 commented Nov 19, 2014

dantleech commented Nov 19, 2014

dbu commented Nov 19, 2014

dbu commented Apr 10, 2015

dantleech commented Apr 10, 2015

dbu commented Jun 14, 2016

dantleech commented Jun 14, 2016

dbu commented Jun 14, 2016 via email

[POC] Metadata and Annotation support #209

Are you sure you want to change the base?

[POC] Metadata and Annotation support #209

Conversation

dantleech commented Jan 26, 2014

dantleech Jan 26, 2014

Choose a reason for hiding this comment

wouterj commented Jan 26, 2014

dantleech commented Jan 26, 2014

dbu Jan 26, 2014

Choose a reason for hiding this comment

dbu commented Jan 26, 2014

wouterj commented Jan 26, 2014

wouterj Jan 26, 2014

Choose a reason for hiding this comment

dantleech Feb 23, 2014

Choose a reason for hiding this comment

dantleech commented Jan 26, 2014

dbu commented Jan 27, 2014

dantleech commented Jan 27, 2014

dbu commented Jan 29, 2014

dbu commented Feb 1, 2014

dantleech commented Feb 1, 2014

dbu commented Feb 2, 2014

dantleech commented Feb 2, 2014

dantleech Feb 2, 2014

Choose a reason for hiding this comment

dbu Feb 2, 2014

Choose a reason for hiding this comment

dantleech Feb 23, 2014

Choose a reason for hiding this comment

dantleech commented Feb 2, 2014

dantleech commented Feb 6, 2014

bigfoot90 commented Nov 19, 2014

dantleech commented Nov 19, 2014

dbu commented Nov 19, 2014

dbu commented Apr 10, 2015

dantleech commented Apr 10, 2015

dbu commented Jun 14, 2016

dantleech commented Jun 14, 2016

dbu commented Jun 14, 2016 via email