Expr::references should go through discriminator map #1333
Conversation
| } | ||
| } | ||
| if ( ! isset($mapping) || $mapping === null) { | ||
| throw $e; |
There was a problem hiding this comment.
This would throw an exception with the message "No mapping found for field '$fieldName' in class '$className'." - should we change that to mention that we've tried child classes as well to let people know about that?
There was a problem hiding this comment.
"No mapping found for field '$fieldName' in class '$className' nor its descendants." ?
There was a problem hiding this comment.
Oook, now I'll need to add more tests :P
|
Looks good! 👍 |
|
Exception added and rebased atop new |
| $childClass = $this->dm->getClassMetadata($child); | ||
| if ($childClass->hasAssociation($this->currentField)) { | ||
| $mapping = $childClass->getFieldMapping($this->currentField); | ||
| break; |
There was a problem hiding this comment.
Rather than give up after finding the first mapping, I think we should check all discriminator possibilities and throw if we find two conflicting mappings. It should be sufficient to compare the mapping arrays with ===.
This would warn users about an edge case of two child classes defining conflicting mappings and remove the ambiguity of selecting the first one that happened to show up during our discriminator map scan.
|
I just recalled that |
|
👍 on the changes to catch conflicting field mappings as well! |
|
LGTM! |
Expr::references should go through discriminator map
Before it was throwing an exception that field was not found. Test case is written like this only for the sake of testing, normally we use same type of references everywhere, only fields are named differently in child classes for the sake of making sense :)