Added a way to enable auto_mapping option using multipe entity managers #321

goetas · 2014-08-11T14:04:18Z

Finally, I fond some time to create this PR. This should close #60

I've also added a small test. Feedbacks are welcome!

(if merged, this will require some lines on SF documentation...)

guilhermeblanco · 2014-08-11T14:40:34Z

DependencyInjection/DoctrineExtension.php

+    {
+        $definedBundles = array_keys($container->getParameter('kernel.bundles'));
+
+        foreach ($entityManagerConfigs as $name => &$entityManager) {


I'd completely reverse the order of your foreach here. I'd loop through definedBundles and find an EM that is the candidate for mapping, not an O(n^3) solution. I assume that multi-auto_mapping should be verified somewhere else, not when deciding the bundles mapping.

It would turn your algorithm into something like this:

private function validateORMAutoMapping(array $entityManagerConfigs) { $emAutoMapping = null; foreach ($entityManagerConfigs as $name => $entityManager) { if ( ! (isset($entityManager['auto_mapping']) && $entityManager['auto_mapping'])) { continue; } if ($emAutoMapping && $emAutoMapping !== $name) { throw new \LogicException(sprintf('You cannot enable "auto_mapping" on more than one entity manager at the same time (found in "%s" and %s").', $emAutoMapping, $name)); } $emAutoMapping = $name; } } private function collectAutoMappings(array $entityManagerConfigs, array $bundles) { foreach ($bundles as $bundle) { $emCandidate = null; foreach ($entityManagerConfigs as $name => &$entityManager) { // Specific bundle mapping if (isset($entityManager['mappings'][$bundle])) { $emCandidate = $entityManager; break; } // Check for auto-mapping if ( ! (isset($entityManager['auto_mapping']) && $entityManager['auto_mapping'])) { continue; } $emCandidate = $entityManager; } // If we could resolve at least one EM if ($emCandidate) { $emCandidate['mappings'][$bundle]['mapping'] = true; } } }

Much better!

seems like a lot of complex logic, that imho should better be in the abstract base class, which is what I did. then again this solution is backwards compatible with earlier Symfony versions I guess.

goetas · 2014-08-11T15:54:15Z

I have committed the changes proposed by @guilhermeblanco

lsmith77 · 2014-08-15T11:52:58Z

DependencyInjection/DoctrineExtension.php

@@ -331,6 +331,10 @@ protected function ormLoad(array $config, ContainerBuilder $container)

        $container->setAlias('doctrine.orm.entity_manager', sprintf('doctrine.orm.%s_entity_manager', $config['default_entity_manager']));

+


extra new line

lsmith77 · 2014-08-15T11:54:38Z

the question is if this code should not get pushed to the DoctrineBridge in Symfony, since the ODM bundles will need this code too.

goetas · 2014-08-15T19:18:52Z

Unfortunately I have zero experience with odm, but Monday I will try to take a look!

goetas · 2014-08-18T06:59:31Z

@lsmith77 I can't see a clean solution how to move this code to DoctrineBridge...

My first idea was to move validateORMAutoMapping and collectAutoMappings to DoctrineBridge, marking them as protected. But this solution introduces an extra dependency between DoctrineBridge and DoctrineBundle... and still need a change into DoctrineBundle.

First implementation:
DoctrineBundle: https://github.com/goetas/DoctrineBundle/compare/auto-mapping-alternative
DoctrineBridge: https://github.com/goetas/symfony/compare/auto-mapping
DoctrineMongoDBBundle: ...

Do you have some suggestions?

stof · 2014-08-18T14:09:03Z

@goetas DoctrineBundle already depends on DoctrineBridge currently.

goetas · 2014-08-18T14:56:34Z

@stof
Is acceptable the way used in https://github.com/goetas/DoctrineBundle/compare/auto-mapping-and
https://github.com/goetas/symfony/compare/auto-mapping ?

Can it be merged (in the next future)?

joschi127 · 2014-08-25T14:24:47Z

👍

stof · 2014-08-28T20:09:09Z

DependencyInjection/DoctrineExtension.php

+        $emAutoMapping = null;
+
+        foreach ($entityManagerConfigs as $name => $entityManager) {
+            if ( !isset($entityManager['auto_mapping']) || !$entityManager['auto_mapping']) {


the isset is useless. It will always be set thanks to the way the Configuration class normalizes the config.

goetas · 2014-09-01T08:32:28Z

Hi!
I have modified this PR, now it requires the merging of symfony/symfony#11815, but it adds the ability to add the same functionality to DoctrineMongoDBBundle (see doctrine/DoctrineMongoDBBundle#267).

lsmith77 · 2014-09-10T08:14:52Z

for BC you should do something like what I did in doctrine/DoctrinePHPCRBundle#167

goetas · 2014-09-10T08:18:41Z

why not add ~2.6 as min version in composer.json?

stof · 2014-09-10T08:19:50Z

@goetas I don't want to need supporting 2 versions of the bundle. We need to keep supporting the 2.3 LTS until its EOL

goetas · 2014-09-10T08:26:09Z

Added the BC commit

…ping feature (goetas) This PR was squashed before being merged into the 2.6-dev branch (closes #11815). Discussion ---------- Added some methods to improve the handling of auto_mapping feature | Q | A | ------------- | --- | Bug fix? |no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | doctrine/DoctrineBundle#60 | License | MIT This PR is a part that is required by: * doctrine/DoctrineMongoDBBundle#267 * doctrine/DoctrineBundle#321 The proposed PRs are an alternative to #11650 and doctrine/DoctrineBundle#322 Commits ------- 2e30a43 Added some methods to improve the handling of auto_mapping feature

…ping feature (goetas) This PR was squashed before being merged into the 2.6-dev branch (closes #11815). Discussion ---------- Added some methods to improve the handling of auto_mapping feature | Q | A | ------------- | --- | Bug fix? |no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | doctrine/DoctrineBundle#60 | License | MIT This PR is a part that is required by: * doctrine/DoctrineMongoDBBundle#267 * doctrine/DoctrineBundle#321 The proposed PRs are an alternative to symfony/symfony#11650 and doctrine/DoctrineBundle#322 Commits ------- 2e30a43 Added some methods to improve the handling of auto_mapping feature

lsmith77 · 2014-09-23T06:40:44Z

do your tests require 2.6?

https://travis-ci.org/doctrine/DoctrineBundle/jobs/34889877#L189

goetas · 2014-09-23T06:49:05Z

@lsmith77 I think that i should skip the test if DoctrineExtension does not have the fixManagersAutoMappings method. Might it work?

lsmith77 · 2014-09-23T06:54:47Z

yup

goetas · 2014-09-23T07:19:18Z

Squashed and tested against 2.6

lsmith77 · 2014-09-23T07:22:42Z

ready to merge then ..

lsmith77 · 2014-09-23T08:28:34Z

@stof seems to be like you are the chief merger on this Bundle .. I do have the shiny green button but do not want to get into the way of things.

stof · 2014-10-04T13:33:02Z

Tests/DependencyInjection/DoctrineExtensionTest.php

+
+        if (!method_exists($loader, 'fixManagersAutoMappings')) {
+            $this->markTestSkipped('Auto mapping with multiple managers available with Symfony ~2.6');
+            return;


the return statement should be removed. It is dead code (markTestSkipped is throwing an exception to interrupt the test)

hmm, actually, I will do it myself.

stof · 2014-10-04T13:48:25Z

It took time, but here we go, this is in now. Thank you very much @goetas.

…entity managers (goetas) This PR was merged into the 1.3.x-dev branch. Discussion ---------- Added a way to enable auto_mapping option using multipe entity managers Finally, I fond some time to create this PR. This should close #60 I've also added a small test. Feedbacks are welcome! (if merged, this will require some lines on SF documentation...) Commits ------- 18fc82f Added a way to enable auto_mapping option using multipe entity managers

goetas · 2014-10-04T13:57:40Z

Thanks everybody!

joschi127 · 2014-10-04T14:55:28Z

A huge thank you!

joschi127 · 2014-11-07T21:11:46Z

@stof: do you know if this will make it into 1.3 and will there be a new 1.3 beta soon? Once again, thanks a lot!

phackwer · 2015-08-18T14:17:10Z

People, I can't find how to activate that for Symfony use. Any suggestions? Is there any new docs about how to do it?

guilhermeblanco reviewed Aug 11, 2014
View reviewed changes

goetas mentioned this pull request Aug 11, 2014

Make it possible to auto map with multiple EMs #322

Closed

lsmith77 reviewed Aug 15, 2014
View reviewed changes

fabpot mentioned this pull request Aug 28, 2014

Make it possible to auto map with multiple OMs symfony/symfony#11650

Closed

stof reviewed Aug 28, 2014
View reviewed changes

goetas mentioned this pull request Sep 1, 2014

Added some methods to improve the handling of auto_mapping feature symfony/symfony#11815

Closed

lsmith77 mentioned this pull request Sep 10, 2014

make it possible to use auto_mapping with multiple document managers doctrine/DoctrinePHPCRBundle#167

Merged

Added a way to enable auto_mapping option using multipe entity managers

18fc82f

lsmith77 mentioned this pull request Sep 30, 2014

A way to enable auto_mapping with multipe Entity Manager #60

Closed

stof reviewed Oct 4, 2014
View reviewed changes

stof merged commit 18fc82f into doctrine:master Oct 4, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a way to enable auto_mapping option using multipe entity managers #321

Added a way to enable auto_mapping option using multipe entity managers #321

goetas commented Aug 11, 2014

guilhermeblanco Aug 11, 2014

goetas Aug 11, 2014

lsmith77 Aug 11, 2014

goetas commented Aug 11, 2014

lsmith77 Aug 15, 2014

lsmith77 commented Aug 15, 2014

goetas commented Aug 15, 2014

goetas commented Aug 18, 2014

stof commented Aug 18, 2014

goetas commented Aug 18, 2014

joschi127 commented Aug 25, 2014

stof Aug 28, 2014

goetas commented Sep 1, 2014

lsmith77 commented Sep 10, 2014

goetas commented Sep 10, 2014

stof commented Sep 10, 2014

goetas commented Sep 10, 2014

lsmith77 commented Sep 23, 2014

goetas commented Sep 23, 2014

lsmith77 commented Sep 23, 2014

goetas commented Sep 23, 2014

lsmith77 commented Sep 23, 2014

lsmith77 commented Sep 23, 2014

stof Oct 4, 2014

stof Oct 4, 2014

stof commented Oct 4, 2014

goetas commented Oct 4, 2014

joschi127 commented Oct 4, 2014

joschi127 commented Nov 7, 2014

phackwer commented Aug 18, 2015

		@@ -331,6 +331,10 @@ protected function ormLoad(array $config, ContainerBuilder $container)

		$container->setAlias('doctrine.orm.entity_manager', sprintf('doctrine.orm.%s_entity_manager', $config['default_entity_manager']));

Added a way to enable auto_mapping option using multipe entity managers #321

Added a way to enable auto_mapping option using multipe entity managers #321

Conversation

goetas commented Aug 11, 2014

guilhermeblanco Aug 11, 2014

Choose a reason for hiding this comment

goetas Aug 11, 2014

Choose a reason for hiding this comment

lsmith77 Aug 11, 2014

Choose a reason for hiding this comment

goetas commented Aug 11, 2014

lsmith77 Aug 15, 2014

Choose a reason for hiding this comment

lsmith77 commented Aug 15, 2014

goetas commented Aug 15, 2014

goetas commented Aug 18, 2014

stof commented Aug 18, 2014

goetas commented Aug 18, 2014

joschi127 commented Aug 25, 2014

stof Aug 28, 2014

Choose a reason for hiding this comment

goetas commented Sep 1, 2014

lsmith77 commented Sep 10, 2014

goetas commented Sep 10, 2014

stof commented Sep 10, 2014

goetas commented Sep 10, 2014

lsmith77 commented Sep 23, 2014

goetas commented Sep 23, 2014

lsmith77 commented Sep 23, 2014

goetas commented Sep 23, 2014

lsmith77 commented Sep 23, 2014

lsmith77 commented Sep 23, 2014

stof Oct 4, 2014

Choose a reason for hiding this comment

stof Oct 4, 2014

Choose a reason for hiding this comment

stof commented Oct 4, 2014

goetas commented Oct 4, 2014

joschi127 commented Oct 4, 2014

joschi127 commented Nov 7, 2014

phackwer commented Aug 18, 2015