[DDC-3300] Added resolve entities support in discrim. map #1130
Conversation
|
Hello, thank you for creating this pull request. I have automatically opened an issue http://www.doctrine-project.org/jira/browse/DDC-3300 We use Jira to track the state of pull requests and the versions they got |
| * | ||
| * @return void | ||
| */ | ||
| public function addResolveTargetEntity($originalEntity, $newEntity, array $mapping) |
There was a problem hiding this comment.
Please move these to a constructor instead
There was a problem hiding this comment.
Sorry, what do you mean? This method can be called many times. This class can be properly refactored, but... what can a constructor be useful for?
There was a problem hiding this comment.
@mmoreram avoiding mutability at all costs: reduces bugs and makes code simpler :-)
There was a problem hiding this comment.
@Ocramius you're right, thanks :) How about now?
| */ | ||
| public function __construct($originalEntity, $newEntity) | ||
| { | ||
| $this->resolveTargetEntities[ltrim($originalEntity, "\\")] = ltrim($newEntity, "\\"); |
There was a problem hiding this comment.
This looks good now, though an array is useless for $resolveTargetEntities.
There was a problem hiding this comment.
@Ocramius hmm, resolveTargetEntities is an array like this:
array(
'My\Namespace\PersonInterface' => 'My\Namespace\Person',
'My\Namespace\CartInterface' => 'My\Namespace\Cart',
'My\Namespace\OrderInterface' => 'My\Namespace\Order'
);so there are two ways of doing this.
- With something like a setter (old implementation)
- Passing full associative array through the constructor as an argument
/**
* @var array
*/
private $resolveTargetEntities = array();
/**
* Construct
*
* @param array $resolveTargetEntities
*/
public function __construct(array $resolveTargetEntities)
{
$this->resolveTargetEntities = $resolveTargetEntities;
}This second one maybe is better to avoid mutability. Am I right?
There was a problem hiding this comment.
Yes, that would be better: just consider that you need to iterate over given $resolveTargetEntities.
|
@Ocramius Has sense to keep working in that PR? |
| { | ||
| $classMetadata = $args->getClassMetadata(); | ||
|
|
||
| if (!empty($classMetadata->discriminatorMap)) { |
There was a problem hiding this comment.
What happens if the map is still empty after remap?
There was a problem hiding this comment.
@mmoreram can you clarify on this point? Is it covered by tests?
|
@mmoreram it would make sense to have a functional test showing how this new API is supposed to be used, then I can defer the decision. |
|
@Ocramius Great! I'll ping you when it's ready ! |
|
@Ocramius Having some problems to test it. Actually working on that :) http://github.com/elcodi/elcodi is actually working with this little change, and its the way I can uncouple completely from the implementation. I'll try to push these tests this weekend. Any tip will be appreciated :) |
The newly introduced class needs a unit test, but a functional test would just be something like the tests in https://github.com/doctrine/doctrine2/tree/f5ecabbc2198973f5e54cb76e99709c59fe62ffa/tests/Doctrine/Tests/ORM/Functional/Ticket
Yeah, but that doesn't count for us :P |
Of course not, just why I want to finish the PR :) As I see, the name of the class is the name of the ticket, 3300, Am I right? Thanks! |
|
Yep, though we may rename the test later (for explicitness). Also, the new feature needs documentation as well. |
|
@Ocramius Just added unit and functional tests. I'll wait for your review before starting with documentation and DoctrineBundle Pull Request :) |
| @@ -2725,7 +2725,7 @@ public function addDiscriminatorMapClass($name, $className) | |||
| if ($this->name == $className) { | |||
| $this->discriminatorValue = $name; | |||
| } else { | |||
| if ( ! class_exists($className)) { | |||
| if ( ! class_exists($className) && ! interface_exists($className)) { | |||
There was a problem hiding this comment.
This should also be covered in ClassMetadataTest
|
Ok, I have a 3 thoughts on this issue:
|
Ocramius
changed the title
|
This may actually have been partially fixed by #1181, as the |
…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
…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
|
Moved to #1257 |
…s first): throwing exceptions if the class is not found in the discriminator map
This PR is WIP. I just wanted to know if what I am doing is wrong or I can go on with tests and documentation.
This is what happens.
I have nicely the ability to define a relation using just the interfaces, and then, resolve these relations overriding the interfaces with the implementations. This resolves a big problem: Multiple implementations of one interface.
But, when you define a discriminatorMap, you Must define it using specific implementations, so all this flexibility gained in the relations is lost at this point.
Using a new listener, its easy to override this interfaces.
What do you think?