-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Detach SyliusCoreBundle from the full-stack app #207
Comments
You already my know my opinion winzou. I totally agree with this idea even if I don't know if dividing the core bundle into two bundles is the best approach. But I don't see any other option atm. So 👍 for me. We have to be able to extend / override entities of the CoreBundle. |
Of course I support this with both my hands and legs. :) In best scenario we could load the mapping dynamically... Anyway I believe we should be cautious here, I agree that splitting the bundles is currently only way... but maybe we can figure out something else. In 2.3 upgrade, there are no entity classes at all (look at PR's), only Models and I load all doctrine mapping via compiler passes. Maybe that's the way to go? |
Uhm you must be right about compiler passes. I don't really know it but we can give it a try. You're welcome to push the 2.3 update and I'll have a look then ;) |
Actually I don't think your idea will be possible. Indeed, as long as the doctrine mapping defined in CoreBundle is an Entity (and not a mappedsuperclass), it can't be used. So in order to add fields you still need to copy/paste CoreBundle mapping into your own bundle, which can't be called "inheritance". |
The syntax of the titles were wrong
Ref.: #196 (comment)
There is a lot of logic in CoreBundle, that doesn't fulfil everyone's needs. Problem is that it's impossible to extend entities, so that the whole bundle is unextendable right now. In order to ease the integration of Sylius in apps that already exist, I think we should split this CoreBundle in two:
AbstractCoreBundle
, which is a copy of the current CoreBundle, but with mappedsuperclasses instead of entities. This bundle must have its own repository so that developers can add it in their app easily;FinalCoreBundle
, which extendsAbstractCoreBundle
and just makes mappedsuperclasses real entities. This bundle will be in Sylius full-stack app, like the current CoreBundle.With this new architecture, developers can just extends
AbstractCoreBundle
and modify what they want (including entities, which is the benefit of this proposal).As far as I know, there is no disadvantage at all.
@pjedrzejewski @marcospassos @stloyd @jjanvier & Cie What do you think?
The text was updated successfully, but these errors were encountered: