symfony-cmf
[POC] Metadata and Annotation support
| 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
-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.
@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.
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?
Yeah, I tend to agree with @dbu Maybe having the other formats in here doesn't make much sense.
@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.
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?
@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
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 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?
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?
@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).
Updated. added a ConfiguationDriver and validation. Although as noted, the validation is done eager and early rather than at compile time.
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
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?
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?