Resolve target entity also in discriminator map (allows interfaces and custom names in discriminator map) #1257
Conversation
|
Hello, thank you for creating this pull request. I have automatically opened an issue http://www.doctrine-project.org/jira/browse/DDC-3503 We use Jira to track the state of pull requests and the versions they got |
| @@ -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))) { | |||
There was a problem hiding this comment.
This check was actually moved into populateDiscriminatorValue
There was a problem hiding this comment.
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.
…based on modified class metadata)
…value into a private method
…g classes first): throwing exceptions if the class is not found in the discriminator map
…bers were missing
…ener` and related test
… discriminator values when needed
Ocramius
force-pushed
the
hotfix/resolve-target-entity-also-in-discriminator-map
branch
from
01fa5d6
to
550694a
Compare
|
Decided to hold back this one until next week: requires some more evaluation due to the complexity of the newly introduced code. |
|
@Ocramius Great! :D Thanks! |
|
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:
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. |
|
Oh dis is nice |
|
👍 |
|
This was merged into |
Ocramius
deleted the
hotfix/resolve-target-entity-also-in-discriminator-map
branch
|
@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 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. |
…ange in discriminator map declarations
… in the upgrade notes/blogpost
|
@guilhermeblanco I documented this breaking change at 3f360d7, making it clear that the previous behavior was a mistake/bug. |
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"}) */