[Known-issue] Doctrine entities could not be woven
TheCelavi opened this issue · 12 comments
Doctrine entities can not be weaved - class metadata gets all messed up.
Engine generates Entity__AopProxied, which is copy of original class, and then generates class Entity that extends Entity__AopProxied and contains weaved methods/properties.
Issue is that Entity__AopProxied contains doctrine metadata, so Doctrine actually generates such metadata that Entity__AopProxied is main class and every relation is bound to that class, not Entity.
Side effects are that, per example, table name gets suffix "__aop_proixed", while relations gets all screwed up, so any join is broken because different aliases are generated with SQL walker. Lazy loading does not work as well.
Possible solution would be (in order to preserve BC compatibility) to provide possibility to specify different method of weaving for certain classes. Per example, in this case, in order to work as it should, Entity__AopProxied should not be generated and inherited, it should be generated only Entity class.
Yes, maybe we will loose nice debugging and breakpoints, however, for now, we can sacrifice that for working code.
Note - I am using annotations to map entities.
Tried to use different mapping method - just for testing purposes, no love. Which is reasonable, Entity__AopProxied is not valid superclass....
Here is the example of wrong SQL:
An exception occurred while executing 'SELECT t1.id AS id_2, t1.feed_item_id AS feed_item_id_3, t1.url AS url_4, t1.platform AS platform_5, t1.views AS views_6, t1.likes AS likes_7, t1.dislikes AS dislikes_8, t1.comments AS comments_9, t1.shares AS shares_10, t1.created_at AS created_at_11, t1.updated_at AS updated_at_12, t1.created_by AS created_by_13, t1.updated_by AS updated_by_14, t1.offer_id AS offer_id_15 FROM report_item t1 WHERE t0.offer_id = ?'
SQLSTATE[42S22]: Column not found: 1054 Unknown column 't0.offer_id' in 'where clause'").
Note how generated alias for report_item is t1, but for where statement is t0 ? This is simple, lazy loaded relation.
Ok, this does not allow me to sleep at all. By playing around, going trough code, I think that there is a workaround in for this situation, however, Doctrine mapping must be modified (tried with hardcoding just to test theory, works).
When Entity__AopProxied class is generated, its mapping data must exists and must be appropriately set. It should contain all Entity mapping data, with exception, it must be set as MappedSuperclass (see http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/inheritance-mapping.html#mapped-superclasses). In that matter Entity__AopProxied would become a mapping superclass, while generated Entity would have all required metadata as concrete class with mapping info from superclass.
It is not just simple "replace annotation" process, Doctrine supports mapping via XML, YML, annotations and PHP...
So, in order to do so successfully, we have to find out a certain point of time, or a method, that we can modify, intercept, whatever, so we can do this modification.
Maybe @Ocramius, @beberlei (or someone else) could give us some pointers or advices how to do this properly...?
@Ocramius not that I have that choice, either way, it seams as easy to solve/add support for doctrine entities.
Only thing required is to modify ClassMetadata for __AopProxied entity. For what I have seen, it should be an easy pick with Doctrine's loadclassmetadata event - just to set isMappedSupperclass to true.
I will try that approach today, I think that it will work.
@lisachenko Ok, got it, here is a bastebin of listener that could handle this mapping issue with Doctrine entities which are weaved: https://pastebin.com/LwdJ0hXn
It gives a basis support, it works with my use case scenario, but in general, it is quite possible, Doctrine seams very flexible when it comes to metadata. I would create a PR for this project, an optional listener for Doctrine that can be used, if required.
For symfony bundle, I would add a compiler pass which would determine if there is a doctrine ORM included, and if that is true, it would auto register listener.
Feedback?
@TheCelavi this issue is very cumbersome, yes. There is no place for double magic (goaop + doctrine), so yes, unicorns are expected )
Just to check one more idea, could we somehow use Proxies\__CG__ namespace to make doctrine trust our generated entities? /cc @Ocramius
Closed via 6e2a0fe
If we are dealing with complex object graph with bidirectional relations and several inheritance, metadata hierarchy gets really, really complex and things tends to go bad...
It seams that Doctrine entities could not be supported by manipulating metadata... I will have to investigate further more in regards to this topic....
This issue ought to be open.
Hi,
FYI, there are still some issues with the workaround. I was experiencing the same t0/t1 issue in the requests, I added the workaround, it worked fine with most of the cases, but I still got an error with some entity relationships.
EntityA is representing sensors and EntityB is representing past sensor values. EntityB has a ManyToOne relation to EntityA, which works fine. But if I add a OneToMany relationship from EntityA to EntityB I get this error :
[Doctrine\ORM\Mapping\MappingException]
It is illegal to put an inverse side one-to-many or many-to-many association on mapped superclass 'Appli\Metier\Bundle\Entity\EntityB__AopProxied#pastValues'.
I'm using Go AOP to add execution traces around all the methods in the Appli namespace, using this annotation in my aspect:
@Around("execution(public|protected Appli\**->*(*))")
If I disable this annotation, everything works fine with the OneToMany relationship between EntityA and EntityB.
Hi, @MatthieuSarter, if you don't need AOP logic for entities, then you could easily exclude all of them from weaving process by adding logical restriction into the pointcut:
@Around("execution(public|protected Appli\**->*(*)) && !within(Appli\**\*Entity*)")
Idea is to exclude all class entities by their namespace or Entity suffix in the name.
Thanks @lisachenko !
It works fine this way :) And I can live with the fact of not having execution traces on the entities methods :)