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

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

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.

lyrixx · 2013-04-08T12:45:19Z

src/Symfony/Bridge/Doctrine/DependencyInjection/CompilerPass/RegisterMappingsPass.php

+     * @param string $enableParameter   service container parameter that must
+     *      be present to enable the mapping. Set to false to not do any check.
+     */
+    public function __construct(


@stof will tell you that we do not wrap constructor argument in symfony code base ;)

@lyrixx this is valid as there are so many arguments that single line would be massively long.

like here: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Http/Firewall/AbstractAuthenticationListener.php#L81

@lyrixx is right, we don't do this in Symfony.

jfsimon · 2013-04-08T16:30:29Z

I cant wait to see this merged!

fabpot · 2013-04-11T06:59:15Z

src/Symfony/Bridge/Doctrine/DependencyInjection/CompilerPass/RegisterMappingsPass.php

+     */
+    protected function enabled(ContainerBuilder $container)
+    {
+        return ! $this->enabledParameter || $container->hasParameter($this->enabledParameter);


there is no whitespace after the ! operator.

dbu · 2013-04-11T13:41:11Z

i fixed the cs issues and did some more cleanup.

@fabpot: may i interpret your feedback as confirmation this is going in the right direction?

to support the annotation and staticphp drivers, we need different buildDriver methods and slightly different parameters. what is the preferred aproach for this? have a BaseRegisterMappingsPass class and 3 extending classes here in the bridge, and then in each of the bundles? or just have the bundles use this as a base class, passing in empty values for mappings and extensions and overwriting the buildDriver method?

fabpot · 2013-04-11T15:42:24Z

@dbu I'm ok with the feature, but I did not had a look at the implementation. @beberlei is probably the one who need to validate the design.

stof · 2013-04-11T21:23:04Z

src/Symfony/Bridge/Doctrine/DependencyInjection/CompilerPass/RegisterMappingsPass.php

+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bridge\Doctrine\DependencyInjection\CompilerPass;


The namespace should be Compiler for consistency

hmm, actually, we already have 1 compiler pass using a different namespace than in bundles :(

this is why i put it in this folder. was also a bit suprised to see the CompilerPass namespace here, but thought to follow the example. i can move it to Compiler and do a PR to move the other compiler pass too and deprecate the one in CompilerPass if we think this should be fixed at some point.

beberlei · 2013-04-13T15:58:06Z

+1 from me

beberlei · 2013-04-13T16:04:09Z

Discussing ways to refactor with @dbu now, regarding the annotations driver

beberlei · 2013-04-13T16:22:06Z

Found a way with @dbu to refactor the code. Idea: The constructor will accept a fully configured Definition instance and the children of this compiler pass will provide static factory methods to create compiler passes for the different mapping drivers such as Xml, Yaml, Annotations, etc...

dbu · 2013-04-15T18:29:52Z

i updated the PR according to the discussion with @beberlei. i think the only open question is whether to keep this in the CompilerPass namespace or move it to Compiler.

dbu · 2013-04-17T21:17:03Z

i just added a PR on mongodb, the implementation for this PR did not need to change so i guess its good.

should i move this class to the Compiler namespace even though we have an existing CompilerPass namespace? that is, shall we keep inconsistency with the rest of the framework or be inconsistent inside this component?

anything else i should do before we can merge?

stof · 2013-04-18T17:28:57Z

src/Symfony/Bridge/Doctrine/DependencyInjection/CompilerPass/RegisterMappingsPass.php

+        }
+
+        $mappingDriverDef = $this->getDriver($container);
+        foreach ($container->getParameter($this->managersParameter) as $name => $manager) {


@dbu IMO, the compiler pass should allow to register the mapping for a specific manager instead of the same mapping for all (which is generally a broken case when all managers are applied for the same entity class)

ok. how would you do that, have an optional manager name in the constructor that defaults to "default" and then only register with the manager service with that name? on the other hand, how could something like the FOSUserBundle know which manager to use? do we need a configuration parameter there, that again defaults to "default"?

FOSUserBundle already have a config setting for it as it needs to know which manager it should retrieve (defaulting to null as $registry->getManager(null) returns the default manager)

i will do a new PR to fix this. sorry!

if i inject the manager name in the constructor, its too early as the compiler pass is created by the bundle class which does not know the configuration. i could make the name of the manager to use yet another option that is read from a container parameter.
or if i could pass around the compiler pass object i could implement a setManagerName method. what would be the clean way to do this?

IMO, using a parameter might be the best way (it would be perfect for FOSUserBundle as we already store it in a parameter anyway).

And the parameter used by default should be the param defining the default manager in Doctrine*Bundle (as it may not be named default)

ok, makes sense. i only use the name in the line below, to replace in the driver name pattern, i don't need the actual manager service name.

regarding the problem of default might not be named "default", i would inject two parameters then? the parameter name for an explicit manager name plus the parameter name for a fallback name to use if the default name does not exist? for the orm that would be doctrine.orm.default_entity_manager holding the name of the manager, not the service name right?

yes, indeed. I first thought you could use doctrine.orm.default_entity_manager as default value for the parameter, but it would indeed break if the user parameter contains null for the default. you need to keep the fallback separate

i created #7755 to hopefully fix the issue

@stof

This PR was merged into the master branch. Discussion ---------- fix register mappings pass to only register mappings with one object manager | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | yes (but feature is totally new) | Deprecations? | no | Tests pass? | yes | Fixed tickets | #7599 | License | MIT | Doc PR | symfony/symfony-docs#2507 This PR fixes the issues @stof discovered in #7599 - we now introduce a way to only register the mappings with one of the object managers. Commits ------- fc12bd1 fix register mappings pass to only register mappings with one object manager

add a base compiler pass class to register doctrine mappings

acf3cf1

This was referenced Apr 8, 2013

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

Merged

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

Merged

lyrixx reviewed Apr 8, 2013
View reviewed changes

jfsimon closed this Apr 8, 2013

jfsimon reopened this Apr 8, 2013

fabpot reviewed Apr 11, 2013
View reviewed changes

cleanup codestyle, more docblocks

0ae61b0

stof reviewed Apr 11, 2013
View reviewed changes

simplify compiler pass by passing in a Definition or Reference

a32e690

This was referenced Apr 15, 2013

[WCM] document the doctrine compiler pass symfony/symfony-docs#2507

Merged

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

Merged

fabpot closed this in 6b27e12 Apr 18, 2013

dbu deleted the doctrine-register-mappings-pass branch April 18, 2013 06:36

stof reviewed Apr 18, 2013
View reviewed changes

jmikola mentioned this pull request Apr 19, 2013

sanity-check DI configuration doctrine/DoctrineMongoDBBundle#184

Closed

dbu mentioned this pull request Apr 21, 2013

fix register mappings pass to only register mappings with one object manager #7755

Merged

Uh oh!

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

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

Uh oh!

Conversation

dbu commented Apr 8, 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!

jfsimon commented Apr 8, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Apr 11, 2013

Uh oh!

fabpot commented Apr 11, 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!

beberlei commented Apr 13, 2013

Uh oh!

beberlei commented Apr 13, 2013

Uh oh!

beberlei commented Apr 13, 2013

Uh oh!

dbu commented Apr 15, 2013

Uh oh!

dbu commented Apr 17, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!