MappedSuperclass with embeddables#132
Closed
Zales0123 wants to merge 2 commits into
Closed
Conversation
Zales0123
added
Bug
Zales0123
force-pushed
the
mappedsuperclass-with-embeddables
branch
2 times, most recently
from
e447893 to
09d17d1
Compare
bartoszpietrzak1994
approved these changes
Apr 30, 2019
GSadee
reviewed
May 2, 2019
| return $this->baseMappingDriverChain->getAllClassNames(); | ||
| } | ||
|
|
||
| public function isTransient($className) |
Zales0123
force-pushed
the
mappedsuperclass-with-embeddables
branch
from
09d17d1 to
658e3eb
Compare
lchrusciel
approved these changes
May 14, 2019
| use Doctrine\ORM\EntityManager; | ||
| use Doctrine\ORM\Event\LoadClassMetadataEventArgs; | ||
|
|
||
| class DriverChain implements MappingDriver |
|
I will try this PR as soon as we have finalized the 1.4.x upgrade to our systems which can possibly be done this week. We will see. :) Thanks for this! ❤️ To be honest, I think this is not resolving any broken things, but rather adding a lot of convenience for developers to be able to extend standard sylius entities with proper means and not overriding all mappings for a plugin. In addition I think the solution to convert mapped-superclasses to entities before embeddables are resolved should be correct. I see no usecase right now where someone would do shenanigans in the same place of execution. :) |
Contributor
|
Took this code and transformed it into Sylius/SyliusResourceBundle#89. |
Contributor
Author
|
Closing, as it should work after Sylius/SyliusResourceBundle#89 in #130 |
pamil
added a commit
that referenced
this pull request
Jun 10, 2019
…bility (kortwotze, pamil, Zales0123) This PR was merged into the 1.0-dev branch. Discussion ---------- Can you also please tag a version of this for Sylius 1.3? Closes #132. Commits ------- 8c9c28e Smaller style and code fixes c081e0f Update the PR and use ResourceBundle ^1.6.0-RC.2 d8da4df Make CreditMemoSequence entity again
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
It was detected on both this and some other plugin. If entities mapping are changed to
mapped-superclass, embeddable relations are not resolved at all (columns are not added to entities' tables). This is, of course, a pity, as embeddables are highly useful for resolving some problems.My first thought was that our
ORMMappedSuperClassSubscriberfrom ResourceBundle is doing something wrong, but I've realized that class metadata passed to itsloadClassMetadatafunction already has embeddable columns set (or not set in our case).Idea
The problem is in this line from doctrine/orm package. Apparently, mapped superclasses are not meant to be used with embeddables for some reason :/ Our subscriber is converting mapped superclasses to entities (with
convertToEntityIfNeededfunction) but it listens toloadClassMetadatawhich is thrown after embeddables resolving 🚀 At the end, the best idea is to useconvertToEntityIfNeededfunction before processing embeddables, but after loading metadata for class.Solution
I've decorated the
MappingDriverChainclass to throw an event just after metadata is loaded for a class (which is still before resolving embeddables) and overwritten theORMMappedSuperClassSubscriberto listen to this event and do the same as forloadClassMetadataevent. It works 🎉 but has some drawbacks:DriverChainnot to break any gedmo extension (which is ridiculous, see https://github.com/Atlantic18/DoctrineExtensions/blob/0b7bdbefd3d166def27928dcd62ab67c11c8f172/lib/Gedmo/Mapping/ExtensionMetadataFactory.php#L136)getDrivers()for example)The separated question is, if this fix is correct, where should it be done? I believe we should fix it in Sylius, but I open a PR here to show that this is working and to not litter the main Sylius repo with some potentially invalid solution 😄
I need your help and opinion very much, to see if it's in any part the way we want to go to fix this frustrating problem 🖖
cc @pamil @lchrusciel @bartoszpietrzak1994 @kortwotze