Skip to content

Made discriminatorMap annotation unnecessary #481

Closed
wants to merge 9 commits into from

3 participants

@DenisGorbachev

Use case:

  • XParentDocument in bundle X.
  • XChildDocument extends XParentDocument in bundle X.
  • YChildDocument extends XParentDocument in bundle Y.

If bundle Y is homemade and bundle X is third-party, a developer can't redefine discriminatorMap on XParentDocument, and thus can't include YChildDocument in it.

This PR solves the issue by storing FQCNs in discriminatorFields instead of arbitrary keys. Also, this PR remaps discriminatorField to an array of parent FQCNs (which, in absence of discriminatorMap, allows to find all parent + children class documents in one query).

Q A
Bug fix? yes (?)
New feature? yes
BC breaks? yes
Deprecations? no
Tests pass? yes
License MIT
Doc PR none (will create if needed)

One test fails, but it does so even on current mongodb-odm master.

@DenisGorbachev

Also, passing DocumentManager to ClassMetadata::getDiscriminatorValuesHierarchy looks superfluous, but moving this method into DocumentManager itself is not a good solution either.

@DenisGorbachev

Any comments?

@jmikola jmikola commented on the diff Feb 11, 2013
lib/Doctrine/ODM/MongoDB/DocumentManager.php
@@ -663,9 +663,10 @@ public function getConfiguration()
public function getClassNameFromDiscriminatorValue(array $mapping, $value)
{
- $discriminatorField = isset($mapping['discriminatorField']) ? $mapping['discriminatorField'] : '_doctrine_class_name';
+ $discriminatorField = isset($mapping['discriminatorField']) ? $mapping['discriminatorField'] : '_doctrine_class_hierarchy';
@jmikola
Doctrine member
jmikola added a note Feb 11, 2013

Changing the field names seems to be a rather significant BC break.

Well, you're right. I've changed "BC breaks?" to "yes".

The purpose was to better reflect the changed field meaning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmikola
Doctrine member
jmikola commented Feb 11, 2013

No worries about the old build failure, as that was due to the PHP driver. I realize existing tests were updated to pass, but do you have new tests to really demonstrate the problem this is solving?

I shared this with @jwage and he mentioned the loadClassMetadata event possibly helping. Have you experimented with creating a listener on that event? I expect it may allow you to modify the parent class' discriminator map.

@DenisGorbachev
@DenisGorbachev

I don't know whether you've been notified about an added commit with a test case, so if you haven't: I did it :)

@jwage
Doctrine member
jwage commented Mar 28, 2015

At this time I don't think we want to accept this PR. I would rather keep the discriminator maps and keep things explicit. This is also a rather large BC break that I don't think I would want to introduce.

@jwage jwage closed this Mar 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.