Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve target entity also in discriminator map (allows interfaces and custom names in discriminator map) #1257

Conversation

Ocramius
Copy link
Member

This PR supersedes #1130 (DDC-3300).

The final aim of the PR is to be able to define following in discriminator maps:

/** @DiscriminatorMap({"foo" = "FooInterface", "bar" = "BarConcreteImpl"}) */

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3503

We use Jira to track the state of pull requests and the versions they got
included in.

@@ -266,9 +280,9 @@ protected function validateRuntimeMetadata($class, $parent)
if ( ! $class->discriminatorColumn) {
throw MappingException::missingDiscriminatorColumn($class->name);
}
} else if ($parent && !$class->reflClass->isAbstract() && !in_array($class->name, array_values($class->discriminatorMap))) {
} else if (! $class->reflClass->isAbstract() && !in_array($class->name, array_values($class->discriminatorMap))) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check was actually moved into populateDiscriminatorValue

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the check for $parent also causes a behavioral change: the root of the inheritance (if non-abstract) must be part of the inheritance.

While this has not been enforced previously, it would have led to runtime problems before.

Now it is enforced/validated when metadata is loaded, therefore the discriminator map must be completely filled, if provided.

@Ocramius Ocramius force-pushed the hotfix/resolve-target-entity-also-in-discriminator-map branch from 01fa5d6 to 550694a Compare January 16, 2015 15:46
@Ocramius Ocramius self-assigned this Jan 16, 2015
@Ocramius
Copy link
Member Author

Decided to hold back this one until next week: requires some more evaluation due to the complexity of the newly introduced code.

@mmoreram
Copy link

@Ocramius Great! :D Thanks!

@Ocramius
Copy link
Member Author

I think this is actually going in the right direction (as for metadata resolution logic).

It is related to DDC-3512, as the current metadata structure is super-messy.

In pseudo-logic, what I'd like to achieve with DDC-3512 is:

  • base metadata is loaded from the mapping driver
  • onLoadMetadata event is fired for each loaded metadata instance
  • metadata is completed by the ClassMetadataFactory logic
  • onCompleteMetadata event is fired for each loaded metadata instance

This would make metadata manipulation from events a bit messier (user needs to know which value to change during which event), but would allow using better constrained metadata structures in future, and that would disallow mistakes during event listeners execution as well (internal validation).

Integrated these notes also in the original DDC-3512 issue.

@macnibblet
Copy link

Oh dis is nice

@TomasVotruba
Copy link
Contributor

👍

@Ocramius
Copy link
Member Author

This was merged into master at dfa4bbd

@Ocramius Ocramius deleted the hotfix/resolve-target-entity-also-in-discriminator-map branch January 22, 2015 08:53
@Ocramius
Copy link
Member Author

@guilhermeblanco you mentioned yesterday that this breaks inheritance mapping when the root of the inheritance is not in the discriminator map.

I actually inspected that further and found that the new behavior is actually the correct one: if you need to NOT persist the root of a STI/JTI, then just mark it abstract (as you should), or otherwise it's a non-transient class like anything else.

Our previous way of doing this was to ignore the root of the inheritance if it wasn't in the identity map, and that's actually a metadata mapping fuckup.

I will add it to the upgrade notes as a minor BC break.

Ocramius added a commit that referenced this pull request Jan 24, 2015
Ocramius added a commit that referenced this pull request Jan 24, 2015
@Ocramius
Copy link
Member Author

@guilhermeblanco I documented this breaking change at 3f360d7, making it clear that the previous behavior was a mistake/bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants