[WIP] add compiler pass for bundles to register mappings #177

dbu · 2013-04-08T12:21:08Z

use the compiler pass added to the doctrine bridge to provide a nice compiler pass for bundles. See symfony/symfony#7599

i will also write documentation once we agree on the exact format. for now see FriendsOfSymfony/FOSUserBundle#1081 for an example how this is intended to be used.

beberlei · 2013-04-08T12:35:48Z

This needs support for annotation and staticphp driver types.

dbu · 2013-04-15T18:38:01Z

this now follows the new instantiation model with factory methods and adds support for annotation and staticphp drivers.

is there a sane way to do testing that will figure out if the drivers actually can be instantiated when defined by the compiler pass? should i add something to the XmlDoctrineExtensionTest ? or do we have something general that can give me the container builder?

dbu · 2013-04-18T06:37:39Z

the PR on symfony core is now merged. any inputs on testing? other improvements? or do we merge as-is?

stof · 2013-04-18T17:18:29Z

DependencyInjection/Compiler/DoctrineOrmMappingsPass.php

+
+namespace Doctrine\Bundle\DoctrineBundle\DependencyInjection\Compiler;
+
+use Symfony\Bridge\Doctrine\DependencyInjection\CompilerPass\RegisterMappingsPass as BaseMappingPass;


Please don't alias it as it is not needed

stof · 2013-04-18T17:32:31Z

@dbu While reviewing this PR, I aslo found a design flaw in the base class: https://github.com/symfony/symfony/pull/7599/files#r3858891

thsi should be fixed first IMO as it will probably require adding a constructor argument in the parent class

dbu · 2013-04-21T08:52:40Z

thanks for the feedback stof. and sorry for needing so much guidance to do this right :-(

anyway, i created symfony/symfony#7755 and pushed a fix for this to this PR. i also adjusted fos user bundle so that we see how things look.

dbu · 2013-04-21T09:03:58Z

DependencyInjection/Compiler/DoctrineOrmMappingsPass.php

+     */
+    public static function createStaticPhpMappingDriver(array $namespaces, array $directories, array $managerParameters = array(), $enabledParameter = false)
+    {
+        $driver = new Definition('Doctrine\Common\Persistence\Mapping\Driver\StaticPHPDriver', array($directories));


so this and the PHPDriver are actually in doctrine commons. i wonder if we could move the factory methods for those into the bridge? it will be copy-paste, except for the actual compiler pass class instantiated. hm. sounds like cludgy code - but if we keep it here, we will copy-paste to mongo/couch/phpcr

If the same code will end up existing in multiple bundles, that sounds like a prime candidate for the bridge.

problem is its not exactly the same, we need to know what to
instantiate. i could add something like
createStaticPhpMappingDriverDefinition to the bridge and keep the new
in the extending classes. or else pass the class as argument and do new $class if this is not considered bad style.

Oh, I didn't realize how tiny these methods were. Were you considering moving all of these to the bridge, or just createStaticPhpMappingDriver()?

staticphpdriver and phpdriver are the same classes for all doctrines. the rest is specific anyways. but yeah i guess they are so small that its not worth it

I don't think it is worth moving it to the bridge for a single line

right, agree. i will copy it to the compiler passes of mongo/couch/phpcr then.

dbu · 2013-05-04T07:41:32Z

the doc PR already got merged - can you please merge or tell me what is missing? symfony/symfony-docs#2507

[WIP] add compiler pass for bundles to register mappings

dbu · 2013-05-24T14:55:49Z

i noticed that i use the SymfonyFileLocator exclusively. this prevents using mapping files that have the full name including the namespace, for example to map a super class of the class a bundle provides. should i add another parameter to the factory methods to allow to override the locator class or a boolean to tell if it should be default file locator or symfony file locator? or do we just document how to do create a custom definition and call the constructor of the compiler pass directly?

kingcrunch · 2013-06-27T20:20:19Z

Possible, that this (currently?) doesn't work with the console?

dbu · 2013-06-28T06:47:56Z

it should work on console. actually i mainly tested this with the FOSUserBundle creating users on the console.

kingcrunch · 2013-06-28T06:59:29Z

I should gave been a little bit more concrete I guess 😉
Especially I talk about doctrine:schema:update. It seems it still looks for entites in Entities. I havent tried others and maybe I did something wrong (meaning: I forgot a reference to the Entity-namespace somewhere in my code)

dbu · 2013-06-28T07:06:57Z

yeah it will look in Entities, but those are just mapped superclasses that point to the model. it should work regardless of whether you extend the Entities or Model classes.

kingcrunch · 2013-06-28T07:33:23Z

@dbu I did not just extend a model-class, I use them. I moved all my entity-classes (wasn't that much in my experimental app 😉 ) to Model/, because I want to move over to MongoDB. I thought its worth a try to just have a single model with two mappings as XML.

The application itself works fine, but the console command I mentioned not.

dbu · 2013-06-28T12:49:17Z

that is not related to this, right? FriendsOfSymfony/FOSUserBundle#1137

to get both mappings, you also need to register the compiler pass for each database. see FriendsOfSymfony/FOSUserBundle#1081 - FOSUserBundle only ever has one database active at a time, but if you have your own things, there should be no problem having more than one compiler pass actually active.

kingcrunch · 2013-06-29T08:32:08Z

I currently only use one database (because of this issue 😉 ), so this shouldn't be a problem. Also the application itself works fine so far, but not doctrines own command line tool.

I've called doctrine:schema:update I stepped throught the code and it seems, that \Doctrine\Common\Persistence\Mapping\AbstractClassMetadataFactory::getAllMetadata tries to fetch the meta data for classes in Vendor/MyBundle/Entity/* as well as in Vendor/MyBundle/Model/*. The last one works fine, but the first one doesn't exists anymore and fails. Stepping deeper into the code it seems, that a mapping driver (I guess \Doctrine\ORM\Mapping\Driver\SimplifiedXmlDriver) just appends the filename to a given prefix, for example \Vendor\MyBundle\Entity\Foo for mapping file Resources/doctrine/Foo.orm.xml. At the it tries to load the metadata for both \Vendor\MyBundle\Entity\Foo and \Vendor\MyBundle\Model\Foo.

It seems, that it is completey irrelevant, what is in this file. Also I have no idea how to disable this behaviour for single bundles.

dbu · 2013-06-29T09:05:35Z

ah, if you still have an Entity folder you must put your mappings in a
folder that is not Resources/config/doctrine but something else (i used
Resources/config/doctrine-model i think). otherwise some compiler pass
from the doctrine bundle will think you magically want the Entity folder
mapped with the doctrine folder.

This PR was squashed before being merged into the master branch (closes symfony#7599). Discussion ---------- [Doctrine-Bridge] add a base compiler pass class to register doctrine mappings | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | not on code, but defining best practices | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | symfony/symfony-docs#2507 Reusable bundles providing model classes should not rely on the automapping provided by the doctrine bundles. What is more, the same model can often be mapped to Doctrine ORM, MongoODM, CouchODM and PHPCR-ODM. This pull request adds a base class for a compiler pass that the concrete doctrine bundles extend to provide a default compiler pass that makes it trivial for bundles to register their mappings. See doctrine/DoctrineBundle#177 The FOSUserBundle shows how this would look in practice. See FriendsOfSymfony/FOSUserBundle#1081 I will create the documentation pull request as well as pull requests for the mongo, couch and phpcr bundles once we agree how exactly to do this. Commits ------- 099fd9f [Doctrine-Bridge] add a base compiler pass class to register doctrine mappings

add compiler pass for bundles to register mappings

804e159

dbu mentioned this pull request Apr 8, 2013

[Doctrine-Bridge] add a base compiler pass class to register doctrine mappings symfony/symfony#7599

Closed

refactor instantiation to use factory methods

9a9191f

fix typo and cleanup docblock

61561fc

dbu mentioned this pull request Apr 17, 2013

Add compiler pass for bundles to register mappings doctrine/DoctrineMongoDBBundle#185

Merged

stof reviewed Apr 18, 2013
View reviewed changes

dbu added 2 commits April 19, 2013 14:57

fix issues on compiler pass

fb026c2

adjust to symfony/symfony#7755

df8ce8c

use drivers from common, not deprecated orm ones

cf4e0c2

dbu reviewed Apr 21, 2013
View reviewed changes

dbu added 2 commits April 24, 2013 13:45

improve doc on managerParameters

8748d37

ws fixes

06800df

dbu mentioned this pull request May 1, 2013

Ensure all bundles use XML mapping symfony-cmf/symfony-cmf#159

Closed

beberlei added a commit that referenced this pull request May 4, 2013

Merge pull request #177 from dbu/doctrine-register-mappings-pass

b7ae4ce

[WIP] add compiler pass for bundles to register mappings

beberlei merged commit b7ae4ce into doctrine:master May 4, 2013

This was referenced May 7, 2013

add mappings compiler pass for bundles doctrine/DoctrineCouchDBBundle#27

Merged

legacy support for mapping compiler pass? #186

Closed

dbu mentioned this pull request May 23, 2013

add compiler pass for orm mappings symfony-cmf/core-bundle#43

Merged

merk mentioned this pull request Jul 31, 2014

use the new doctrine compiler pass to map the model classes directly FriendsOfSymfony/FOSUserBundle#1081

Merged


		namespace Doctrine\Bundle\DoctrineBundle\DependencyInjection\Compiler;

		use Symfony\Bridge\Doctrine\DependencyInjection\CompilerPass\RegisterMappingsPass as BaseMappingPass;

Uh oh!

[WIP] add compiler pass for bundles to register mappings #177

[WIP] add compiler pass for bundles to register mappings #177

Uh oh!

Conversation

dbu commented Apr 8, 2013

Uh oh!

beberlei commented Apr 8, 2013

Uh oh!

dbu commented Apr 15, 2013

Uh oh!

dbu commented Apr 18, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof commented Apr 18, 2013

Uh oh!

dbu commented Apr 21, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented May 4, 2013

Uh oh!

dbu commented May 24, 2013

Uh oh!

kingcrunch commented Jun 27, 2013

Uh oh!

dbu commented Jun 28, 2013

Uh oh!

kingcrunch commented Jun 28, 2013

Uh oh!

dbu commented Jun 28, 2013

Uh oh!

kingcrunch commented Jun 28, 2013

Uh oh!

dbu commented Jun 28, 2013

Uh oh!

kingcrunch commented Jun 29, 2013

Uh oh!

dbu commented Jun 29, 2013

Uh oh!

Uh oh!