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

[DDC-3300] Added resolve entities support in discrim. map #1130

Closed
wants to merge 5 commits into from
Closed

[DDC-3300] Added resolve entities support in discrim. map #1130

wants to merge 5 commits into from

Conversation

mmoreram
Copy link

@mmoreram mmoreram commented Sep 9, 2014

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?

@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-3300

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

*
* @return void
*/
public function addResolveTargetEntity($originalEntity, $newEntity, array $mapping)
Copy link
Member

Choose a reason for hiding this comment

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

Please move these to a constructor instead

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

@mmoreram avoiding mutability at all costs: reduces bugs and makes code simpler :-)

Copy link
Author

Choose a reason for hiding this comment

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

@Ocramius you're right, thanks :) How about now?

*/
public function __construct($originalEntity, $newEntity)
{
$this->resolveTargetEntities[ltrim($originalEntity, "\\")] = ltrim($newEntity, "\\");
Copy link
Member

Choose a reason for hiding this comment

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

This looks good now, though an array is useless for $resolveTargetEntities.

Copy link
Author

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that would be better: just consider that you need to iterate over given $resolveTargetEntities.

@mmoreram
Copy link
Author

@Ocramius Has sense to keep working in that PR?

{
$classMetadata = $args->getClassMetadata();

if (!empty($classMetadata->discriminatorMap)) {
Copy link
Member

Choose a reason for hiding this comment

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

What happens if the map is still empty after remap?

Copy link
Member

Choose a reason for hiding this comment

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

@mmoreram can you clarify on this point? Is it covered by tests?

@Ocramius
Copy link
Member

@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.

@mmoreram
Copy link
Author

@Ocramius Great! I'll ping you when it's ready !

@mmoreram
Copy link
Author

@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 :)

@Ocramius
Copy link
Member

Having some problems to test it

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

http://github.com/elcodi/elcodi is actually working with this little change, and its the way I can uncouple completely from the implementation.

Yeah, but that doesn't count for us :P

@mmoreram
Copy link
Author

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!

@Ocramius
Copy link
Member

Yep, though we may rename the test later (for explicitness).

Also, the new feature needs documentation as well.

@mmoreram
Copy link
Author

@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)) {
Copy link
Member

Choose a reason for hiding this comment

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

This should also be covered in ClassMetadataTest

@Ocramius
Copy link
Member

Ok, I have a 3 thoughts on this issue:

  • First, the fix in ClassMetadataInfo is valid, needs a test and is 100% going to be merged.
  • I'm not sure if I like the rest of the feature though, as it seems to be something that should be part of the ResolveTargetEntityListener, though then we'd have mixed responsibilities there.
  • Additionally, assuming that the user has interfaces in inheritance map definitions, how do we know that those mappings are invalid, if the listener was not attached to the event manager? I think this needs patching in the SchemaValidator

@Ocramius Ocramius changed the title [WIP] Added resolve entities support in discrim. map [DDC-3300] Added resolve entities support in discrim. map Jan 15, 2015
@Ocramius
Copy link
Member

This may actually have been partially fixed by #1181, as the ClassMetadataFactory is now able to fetch by interface name.

Ocramius added a commit to Ocramius/doctrine2 that referenced this pull request Jan 15, 2015
Ocramius added a commit to Ocramius/doctrine2 that referenced this pull request Jan 15, 2015
Ocramius added a commit to Ocramius/doctrine2 that referenced this pull request Jan 15, 2015
Ocramius added a commit to Ocramius/doctrine2 that referenced this pull request Jan 15, 2015
…g classes first): throwing exceptions if the class is not found in the discriminator map
Ocramius added a commit to Ocramius/doctrine2 that referenced this pull request Jan 15, 2015
Ocramius added a commit to Ocramius/doctrine2 that referenced this pull request Jan 15, 2015
Ocramius added a commit to Ocramius/doctrine2 that referenced this pull request Jan 15, 2015
@Ocramius
Copy link
Member

@mmoreram I provided #1257 as a drop-in replacement for this PR: consider reviewing it.

@Ocramius Ocramius self-assigned this Jan 15, 2015
Ocramius added a commit to Ocramius/doctrine2 that referenced this pull request Jan 16, 2015
Ocramius added a commit to Ocramius/doctrine2 that referenced this pull request Jan 16, 2015
Ocramius added a commit to Ocramius/doctrine2 that referenced this pull request Jan 16, 2015
Ocramius added a commit to Ocramius/doctrine2 that referenced this pull request Jan 16, 2015
…g classes first): throwing exceptions if the class is not found in the discriminator map
Ocramius added a commit to Ocramius/doctrine2 that referenced this pull request Jan 16, 2015
Ocramius added a commit to Ocramius/doctrine2 that referenced this pull request Jan 16, 2015
Ocramius added a commit to Ocramius/doctrine2 that referenced this pull request Jan 16, 2015
Ocramius added a commit to Ocramius/doctrine2 that referenced this pull request Jan 16, 2015
Ocramius added a commit to Ocramius/doctrine2 that referenced this pull request Jan 16, 2015
Ocramius added a commit to Ocramius/doctrine2 that referenced this pull request Jan 16, 2015
@Ocramius
Copy link
Member

Moved to #1257

@Ocramius Ocramius closed this Jan 16, 2015
Ocramius added a commit that referenced this pull request Jan 22, 2015
Ocramius added a commit that referenced this pull request Jan 22, 2015
Ocramius added a commit that referenced this pull request Jan 22, 2015
…s first): throwing exceptions if the class is not found in the discriminator map
Ocramius added a commit that referenced this pull request Jan 22, 2015
Ocramius added a commit that referenced this pull request Jan 22, 2015
Ocramius added a commit that referenced this pull request Jan 22, 2015
Ocramius added a commit that referenced this pull request Jan 22, 2015
Ocramius added a commit that referenced this pull request Jan 22, 2015
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.

3 participants