EntityManagerDecorator base class as an extension point for EntityManager #524

Merged
merged 1 commit into from May 4, 2013

Projects

None yet

8 participants

@lstrojny
Contributor

As discussed on IRC. Open issues to be discussed:

  • Should we better extend ObjectManagerDecorator (see doctrine/common#229) and introduce an EntityManagerInterface?
  • Should the abstract decorator have a constructor?

Hello,

thank you for positing this Pull Request. I have automatically opened an issue on our Jira Bug Tracker for you with the details of this Pull-Request. See the Link:

http://doctrine-project.org/jira/browse/DDC-2169

@stof stof commented on an outdated diff Nov 26, 2012
lib/Doctrine/ORM/EntityManagerDecorator.php
@@ -0,0 +1,355 @@
+<?php
+/*
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ {}
+
+ HOWEVER CAUSED AND ON ANY
stof
stof Nov 26, 2012 Member

Please fix the license header

Owner

As already said on doctrine/common#229 I don't see any value on this support.
Waiting for more comments from other contributors

Member
stof commented Nov 26, 2012

Thus, this implementation will not work properly in all places. When getting a repository and then accessing the entity manager in it (for example to create a query builder), you would not get the decorated manager

@lstrojny lstrojny referenced this pull request in doctrine/common Nov 26, 2012
Merged

Decorator base class for object manager decorators #229

Contributor

@stof is right, we need to add a few setEntityManager methods here and there. @beberlei what do you think?

Owner

@lstrojny thinking of expanding the decoration to EntityRepository. This covers all extension points except events, however these are impossible to replace with the decorator, so we shouldn't try.

Contributor

@beberlei Good idea! Should we add a few methods to Doctrine\ORM\Configuration to register a chain of decorators for Repository, EntityManager, Query and QueryBuilder?

Contributor

Added EntityManagerInterface that extends ObjectManager. Plan would be to add the final modifier for EntityManager in 2.6 (?) and rigorously advertise that one should mock the interface, not the concrete class.

Contributor

QueryInterface and SQLQueryInterface are the necessary base to decorate query objects.

Member

+1 for having a base decorator

Owner

@lstrojny i am not sure about decorators for queries, interfaces only make sense for now. i am wondering if the decorators should be in a subnamespace instead of in ORM directly.

Contributor

@beberlei subnamespace, yep, good idea. Decorators for Queries: needed for cases where you want to implement more fine grained exception handling, custom result functions etc. E.g. doctrine-fun needs it to implement Query::getOptionResult().

Contributor

Hey everybody, that’s what we currently have:

  • New interfaces Doctrine\ORM\QueryInterface and Doctrine\ORM\EntityManagerInterface
  • Related decorator base classes in Doctrine\ORM\Decorator\...

If we agree this is the way to go, this is what I would do next:

  • Add NativeQueryInterface and decorator
  • Add QueryBuilderInterface and decorator
  • Add EntityRepositoryInterface and decorator (with a base class in common)
  • Some configuration support for decorators (haven’t thought this through much though)
Owner

Not sure about this.. 😕

Anyway, if we are going to support this we need other names for the interfaces.
Doctrine does not suffix with Interface.

Member
stof commented Nov 26, 2012

And there is an issue if we add interfaces everywhere: BC. Currently, we can add new methods in the QueryBuilder in minor releases without issue. If we provide an interface, we cannot add new methods in it without breaking BC (which is why the Collection interface in Common does not extend the new Selectable interface for instance)

Contributor

@stof that's a problematic point indeed. We could simply stte, that people should only use those interfaces for mocking, not to implement them "by hand". That’s what decorator base classes are for.

Contributor

@beberlei, @guilhermeblanco what do you guys think?

Contributor

Ping @beberlei, @guilhermeblanco et al. Before spending more work on this issue I would like to know if this has a chance of being merged.

Contributor
Owner

@lstrojny I think decorators for Query and QueryBuilder are over the top, people can add their own decorators for that, the EntityManager Decorator is a good starting point but after that its too much.

As for the interface, i would rather have an AbstractEntityManager that defines the API instead. We don't want an interface for this at the moment.

So that would make:

  • AbstractEntityManager
  • EntityManager extends AbstractEntityManager
  • Tools\EntityManagerDecorator

@guilhermeblanco whats your opinion?

Contributor

@beberlei thank you very much for the response. I'll wait for @guilhermeblanco to comment and will refactor the branch accordingly.

Owner

I agree with @beberlei. Defining interfaces for Query* is overkill.
Regarding names, we do not support Interface at all, following more Java style. We never came to an agreement with Abstract classes, but we do have them in our source code, so we could keep them.
I'm against having an interface for EntityManager too, specially if we strictly follow our rules, it would be called EntityManager and would force implementation to be called as EntityManagerImpl, as JPA suggests.

So for now I'd stick to @beberlei 's idea and only add an abstract class.

Contributor

@guilhermeblanco @beberlei thanks for your feedback. I'm going to change my PR in this direction.

Contributor
lstrojny commented Mar 6, 2013

@beberlei @guilhermeblanco Removed the QueryDecorators. What about the abstract class for EntityManager, what should we put in there?

@beberlei beberlei merged commit c8bcdb4 into doctrine:master May 4, 2013

1 check passed

default The Travis build passed
Details
Contributor
lstrojny commented May 6, 2013

😍

@Majkl578
Contributor

This solution is unusable at this moment in 2.4 since a lot of classes has methods (e.g. in constructors) that typehint EntityManager directly.

@Majkl578 unfortunately, changing these places to EntityManagerInterface breaks BC, something we cannot do on 2.X

Contributor

Yes, I know that, I'm just wondering if it wouldn't be better to not include this feature in 2.4 at all, I think this change doesn't make much sense without also changing signature of those methods and may lead to bad assumptions in userland.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment