[DDC-3300] Added resolve entities support in discrim. map#1130
[DDC-3300] Added resolve entities support in discrim. map#1130mmoreram wants to merge 5 commits intodoctrine:masterfrom mmoreram:feature/resolve-target-entity-in-discriminator-map
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 |
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.
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? |
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 :) |
|
Ok, I have a 3 thoughts on this issue:
|
|
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?