provide a compiler pass for doctrine to register namespace aliases #10853

dbu · 2014-05-04T16:41:25Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	symfony/symfony-docs#4069

I realized that the compiler pass only allows bundles to register folders to load entities / documents but not namespace alias to make things like CmfRoutingBundle:Route work. This is an attempt to provide a separate compiler pass for this. Maybe we should instead provide a new combined compiler pass and have users switch to the new pass if available.

dbu · 2014-05-04T16:48:36Z

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

-     * 'doctrine.orm.%s_metadata_driver'
-     * @var string
-     */
-    protected $driverPattern;


this is a micro BC break as in the base class i call this serviceNamePattern and its only protected. we could either use a wrong name in the base class or keep this field (but then instead of a php error we would get a semantic error if the value is directly read / assigned.)

as none of the doctrine bundles directly accesses the value, i think the BC break would hit only people that did a custom extension of this pass, which i doubt exists.

dbu · 2014-05-04T18:29:32Z

i think the test failure is not related to what i did.

jjsaunier · 2014-05-04T18:58:54Z

Nope, it's not you, take a look #10826, all test failed from 5.5

dbu · 2014-05-11T16:15:02Z

ping @beberlei @stof any inputs on this?

ClementGautier · 2014-07-11T14:15:24Z

looks good to me

fabpot · 2014-07-21T05:05:50Z

I would indeed deprecate the current one and create just one pass to switch to.

dbu · 2014-07-25T11:52:22Z

had a go at cleaning this up and realized that we can keep BC and just add more constructor arguments. this will be a lot more convenient for consumers - they can just pass additional parameters that are ignored on older symfony versions and don't get namespace alias but do get the rest.

dbu · 2014-07-25T11:54:42Z

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

+     * is used, the others skipped.
+     *
+     * The $alias parameter can be used to define bundle namespace shortcuts as the DoctrineBundle
+     * provides automatically for objects in the default Entity/Document folder.


i found no explicit documentation to link to, but alias are mentioned in this section of the book http://symfony.com/doc/current/book/doctrine.html#fetching-objects-from-the-database

dbu · 2014-07-25T12:49:19Z

Once we agree on this, i will update https://github.com/symfony/symfony-docs/blob/master/cookbook/doctrine/mapping_model_classes.rst and update the orm and mongo, couch bundles. phpcr-odm is being updated already: doctrine/DoctrinePHPCRBundle#158

dantleech · 2014-07-25T13:07:09Z

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

+        $enabledParameter = false,
+        $configurationPattern = '',
+        $registerAliasMethodName = '',
+        array $alias = array()


As mentioned in https://github.com/doctrine/DoctrinePHPCRBundle/pull/158/files#r15399221 maybe this should be $aliasMap or $aliases ?

fixed to aliasMap, thanks

dbu · 2014-07-28T06:55:01Z

@fabpot what do you think about this? ok like this? the parameter order in the constructor is not awesome, but is BC compatible, simplifying the live of doctrine bundles that build on top of this a lot.

fabpot · 2014-07-28T09:39:50Z

Looks good to me.

dbu · 2014-07-29T22:11:35Z

did pull requests for the orm and phpcr-odm bundles, and the symfony-docs (links in issue description). how do we proceed? @beberlei can you give some feedback on the doctrine bundle PR?

hacfi · 2014-07-29T23:05:37Z

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

        $chainDriverDefService = $this->getChainDriverServiceName($container);
        $chainDriverDef = $container->getDefinition($chainDriverDefService);
        foreach ($this->namespaces as $namespace) {
+            // instance of Doctrine\Common\Persistence\Mapping\Driver\MappingDriverChain


If you have to commit again you could change that to /** @var \Doctrine\Common\Persistence\Mapping\Driver\MappingDriverChain $chainDriverDef */ and move it up.

of course. must have been tired. thanks for spotting, fixed.

fabpot · 2014-07-30T05:33:18Z

@dbu Let me know whenever @beberlei is ready to merge the changes in DoctrineBundle.

stof · 2014-08-05T15:39:40Z

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

+     * Map of alias to namespace.
+     * @var array
+     */
+    protected $aliasMap;


should we use private properties instead ?

there are a couple of existing properties already, we can't restrict them as it would be a BC break. but can make these private, would have been better.

I know we canot change the existing ones. I'm talking about the new ones here

stof · 2014-08-05T15:45:17Z

This also needs to be rebased to fix conflicts

dbu · 2014-08-05T18:46:44Z

thanks for the feedback stof, agreeing and updated all points you noted.

no idea where that conflict came from. but i rebased on upstream/master and seems good now.

stloyd · 2014-08-05T18:48:27Z

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

+     * @throws ParameterNotFoundException if none of the managerParameters has a
+     *      non-empty value.
+     *
+     * @since 2.6


It's private so I would say that it can be removed.

kimhemsoe · 2014-08-06T14:21:31Z

We are ready to merge doctrine/DoctrineBundle#316 whenever you are.

We have a DoctrineBundle 1.3 release, that we are planning to do this week. Do you want us to post prone until this PR gets merged?

fabpot · 2014-08-06T14:47:39Z

@dbu Do you have time to finish this PR so that it can be merged fast?

…d not only mapping drivers

dbu · 2014-08-06T15:10:55Z

adjusted to the feedback of @stloyd

would be great if this can make it into DoctrineBundle 1.3. @kimhemsoe there is no problem merging the DoctrineBundle PR for older symfony versions, as its just extra parameters to the constructor that php will ignore.

kimhemsoe · 2014-08-08T09:52:10Z

What is the status of this ?

I know it it will not break anything, but we rather wait on this PR to merge before merging DoctrineBundle one.

fabpot · 2014-08-08T09:58:42Z

👍

dbu · 2014-08-08T10:53:20Z

@kimhemsoe here we wait for input from the doctrine orm maintainers :-)

beberlei said that @kimhemsoe is release manager of the doctrine bundle. so i think the comment above is an approval from doctrine and we can merge this, right?

stof · 2014-08-08T11:16:16Z

👍

kimhemsoe · 2014-08-08T11:44:35Z

👍

fabpot · 2014-08-08T13:37:31Z

Thank you @dbu.

…pace aliases (dbu) This PR was merged into the 2.6-dev branch. Discussion ---------- provide a compiler pass for doctrine to register namespace aliases | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | symfony/symfony-docs#4069 I realized that the compiler pass only allows bundles to register folders to load entities / documents but not namespace alias to make things like `CmfRoutingBundle:Route` work. This is an attempt to provide a separate compiler pass for this. Maybe we should instead provide a new combined compiler pass and have users switch to the new pass if available. * [x] [Doctrine ORM](doctrine/DoctrineBundle#316) * [x] [DoctrinePHPCRBundle](doctrine/DoctrinePHPCRBundle#158) * [ ] Doctrine CouchDB * [ ] Doctrine MongoDB Commits ------- 9a62ab3 provide a compiler pass for doctrine to register namespace aliases and not only mapping drivers

This PR was merged into the master branch. Discussion ---------- document the namespace alias feature | Q | A | ------------- | --- | Doc fix? | no | New docs? | [yes](symfony/symfony#10853) | Applies to | 2.6 | Fixed tickets | - Commits ------- d753d0e document the namespace alias feature

dbu mentioned this pull request May 4, 2014

mention templates_by_class in tutorial symfony-cmf/symfony-cmf-docs#474

Merged

dbu reviewed May 4, 2014
View reviewed changes

dbu mentioned this pull request May 8, 2014

add compiler pass for alias mapping doctrine/DoctrinePHPCRBundle#146

Closed

dbu added the Doctrine label Jun 16, 2014

dbu reviewed Jul 25, 2014
View reviewed changes

dbu mentioned this pull request Jul 25, 2014

add support for bundle namespace alias doctrine/DoctrinePHPCRBundle#158

Merged

dantleech reviewed Jul 25, 2014
View reviewed changes

This was referenced Jul 29, 2014

add support for bundle namespace alias doctrine/DoctrineBundle#316

Merged

document the namespace alias feature symfony/symfony-docs#4069

Merged

hacfi reviewed Jul 29, 2014
View reviewed changes

dbu mentioned this pull request Aug 1, 2014

[WIP] Mapped superclass ekino/EkinoWordpressBundle#27

Merged

stof reviewed Aug 5, 2014
View reviewed changes

stloyd reviewed Aug 5, 2014
View reviewed changes

provide a compiler pass for doctrine to register namespace aliases an…

9a62ab3

…d not only mapping drivers

fabpot merged commit 9a62ab3 into symfony:master Aug 8, 2014

dbu deleted the doctrine-bridge-compiler-pass branch August 8, 2014 15:07

This was referenced Aug 8, 2014

add support for bundle namespace alias doctrine/DoctrineCouchDBBundle#39

Merged

add support for bundle namespace alias doctrine/DoctrineMongoDBBundle#264

Merged

support bundle alias notation FriendsOfSymfony/FOSUserBundle#1566

Closed

Uh oh!

provide a compiler pass for doctrine to register namespace aliases #10853

provide a compiler pass for doctrine to register namespace aliases #10853

Uh oh!

Conversation

dbu commented May 4, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented May 4, 2014

Uh oh!

jjsaunier commented May 4, 2014

Uh oh!

dbu commented May 11, 2014

Uh oh!

ClementGautier commented Jul 11, 2014

Uh oh!

fabpot commented Jul 21, 2014

Uh oh!

dbu commented Jul 25, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Jul 25, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Jul 28, 2014

Uh oh!

fabpot commented Jul 28, 2014

Uh oh!

dbu commented Jul 29, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Jul 30, 2014

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!

stof commented Aug 5, 2014

Uh oh!

dbu commented Aug 5, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimhemsoe commented Aug 6, 2014

Uh oh!

fabpot commented Aug 6, 2014

Uh oh!

dbu commented Aug 6, 2014

Uh oh!

kimhemsoe commented Aug 8, 2014

Uh oh!

fabpot commented Aug 8, 2014

Uh oh!

dbu commented Aug 8, 2014

Uh oh!

stof commented Aug 8, 2014

Uh oh!

kimhemsoe commented Aug 8, 2014

Uh oh!

fabpot commented Aug 8, 2014

Uh oh!

Uh oh!