Utilize Doctrine common for mappings #124

Merged
merged 5 commits into from Jul 20, 2012

Conversation

Projects
None yet
2 participants
Owner

jmikola commented Jul 19, 2012

This replaces #116 and #122.

@stof: Please take a look at my revisions to the test class.

The only question I have is that I had to flip the key/value pairs from the previous tests in order to work with the new locator service. Does this pose a problem for the extension class, or does the bridge take care of the change for us?

Owner

jmikola commented Jul 19, 2012

I cleaned up the tests a bit and we have one failure remaining:

1) Symfony\Bundle\DoctrineMongoDBBundle\Tests\ContainerTest::testContainer
call_user_func_array() expects parameter 1 to be a valid callback, class 'Symfony\Bundle\DoctrineMongoDBBundle\Mapping\Driver\YamlDriver' does not have a method 'setNamespacePrefixes'

vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php:764
vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php:338
vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php:804
vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php:801
vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php:764
vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php:338
vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php:804
vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php:801
vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php:764
vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php:338
vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php:804
vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php:801
vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php:722
vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php:338
Tests/ContainerTest.php:53

I traced the outstanding calls to setNamespacePrefixes() to some bridge and DoctrineAbstractBundle classes:

vendor/symfony/symfony/src/Symfony/Bridge/Doctrine/Mapping/Driver/XmlDriver.php:
   37      }
   38  
   39:     public function setNamespacePrefixes($prefixes)
   40      {
   41          $this->_prefixes = $prefixes;

vendor/symfony/symfony/src/Symfony/Bridge/Doctrine/Mapping/Driver/YamlDriver.php:
   37      }
   38  
   39:     public function setNamespacePrefixes($prefixes)
   40      {
   41          $this->_prefixes = $prefixes;

vendor/symfony/symfony/src/Symfony/Bundle/DoctrineAbstractBundle/DependencyInjection/AbstractDoctrineExtension.php:
  209              $mappingDriverDef->setPublic(false);
  210              if (false !== strpos($mappingDriverDef->getClass(), 'yml') || false !== strpos($mappingDriverDef->getClass(), 'xml')) {
  211:                 $mappingDriverDef->addMethodCall('setNamespacePrefixes', array(array_flip($driverPaths)));
  212                  $mappingDriverDef->addMethodCall('setGlobalBasename', array('mapping'));
  213              }

vendor/symfony/symfony/tests/Symfony/Tests/Bridge/Doctrine/Mapping/Driver/XmlDriverTest.php:
   24      {
   25          $driver = new XmlDriver(array_values($paths));
   26:         $driver->setNamespacePrefixes(array_flip($paths));
   27  
   28          return $driver;

vendor/symfony/symfony/tests/Symfony/Tests/Bridge/Doctrine/Mapping/Driver/YamlDriverTest.php:
   24      {
   25          $driver = new YamlDriver(array_values($paths));
   26:         $driver->setNamespacePrefixes(array_flip($paths));
   27  
   28          return $driver;

I take it you never added common mapping support to ORM's DoctrineBundle for 2.0, correct?

We don't use the bridge's XmlDriver and YamlDriver classes, and we can probably get away with extending AbstractDoctrineExtension::registerMappingDrivers() if necessary.

@ghost ghost assigned jmikola Jul 19, 2012

Member

stof commented Jul 19, 2012

indeed, 2.0 does not support the ORM 2.3 (we have other issues too as the mapping drivers are in DoctrineBundle itself in 2.0 whereas they are in the ORM since 2.2)

Member

stof commented Jul 19, 2012

@jmikola the need to flip is weird. The ORM does not need to flip the array passed by the DI extensions of the bridge.

Owner

jmikola commented Jul 20, 2012

@stof: I looked at the difference between AbstractDoctrineExtension::registerMappingDrivers() in Symfony 2.0 and master and it comes down to a single commit, which looks familiar: symfony/symfony@3ca1ccb#commitcomment-671454

Member

stof commented Jul 20, 2012

ah yeah, indeed. So you may need to re-add the method in the 2.0 branch (forwarding the data to the file locator).

Owner

jmikola commented Jul 20, 2012

I came up with an alternate solution, which I'll push in a minute.

jmikola added some commits Jul 19, 2012

Account for AbstractDoctrineExtension's assumptions about ORM mapping…
… drivers

Since the ODM is using Doctrine Common's mapping drivers for Symfony 2.0, we need to remove the setNamespacePrefixes() method call and instead pass its argument to the driver constructor.

See: symfony/symfony@3ca1ccb for some historical context.
Member

stof commented Jul 20, 2012

seems good to me. If all works now, please merge it so that people stop complaining.

And don't forget to remove the extra code when merging into master to avoid useless code :)

Owner

jmikola commented Jul 20, 2012

@stof: I didn't think reimplementing setNamespacePrefixes() in the driver was as viable, since the $fileExtension parameter to the constructor would not be available when recreating and resetting the locator property with our method. Although admittedly, the file extension parameter is never used in the bundle from what I can see...

jmikola added a commit that referenced this pull request Jul 20, 2012

Merge pull request #124 from doctrine/2.0-common-mapping
Utilize Doctrine common for mappings

@jmikola jmikola merged commit 84fc74c into 2.0 Jul 20, 2012

Owner

jmikola commented Jul 20, 2012

Ok, merged into master and also reverted the 2.0-specific fix with e0f3570

@jmikola jmikola referenced this pull request Jul 30, 2012

Closed

RFC: Release tags #138

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment