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

Doctrine ORM #164

Closed
dpfaffenbauer opened this issue Jun 11, 2024 · 7 comments
Closed

Doctrine ORM #164

dpfaffenbauer opened this issue Jun 11, 2024 · 7 comments

Comments

@dpfaffenbauer
Copy link
Contributor

Why are you defining a new Entity Manager here?

https://github.com/pimcore/generic-data-index-bundle/blob/1.x/config/doctrine.yaml#L4

Another Question: Why are you not using the Entity Manager to create the Table on Installation here?

https://github.com/pimcore/generic-data-index-bundle/blob/1.x/src/Installer.php#L93

You can use the SchemaTool with the MetaData of the Table to create it like we do here:

https://github.com/coreshop/CoreShop/blob/next/src/CoreShop/Bundle/ResourceBundle/Command/CreateDatabaseTablesCommand.php#L86

@markus-moser
Copy link
Contributor

Why are you defining a new Entity Manager here?

Because we currently do not have a entity manager/ORM in the core. We do it the same way for all (private and public) bundles. Which approach would you expect instead?

Another Question: Why are you not using the Entity Manager to create the Table on Installation here?

Just because we did not know it better. Thanks for you input, we can try to change it like suggested 👍

@dpfaffenbauer
Copy link
Contributor Author

There is always a default entity manager.

I saw that this configuration breaks CoreShop cause we use the default one and when installing the generic index bundle this one will be the default one. I mean there are workarounds for that, I just noticed it and I don't see a reason for a separate entity manager.

@markus-moser
Copy link
Contributor

Isn't your example a good reason why a separate entity manager could help? If we do not want that other bundles break the generic data index a explicitly defined entity manager would make it safer. Also as mentioned above all other Pimcore bundles which use ORM use the same approach (so something like 10 bundles). Changing all of them is not easily possible.

@dpfaffenbauer
Copy link
Contributor Author

Symfony notes that:

Using multiple entity managers is not complicated to configure, but more advanced and not usually required. Be sure you actually need multiple entity managers before adding in this layer of complexity.

https://symfony.com/doc/current/doctrine/multiple_entity_managers.html

There is a point to make to use separate ones. But is it worth it for one table? Not sure about it. Other big frameworks that use ORM also only use the default entity manager.

Copy link

github-actions bot commented Jul 3, 2024

Thanks a lot for reporting the issue. We did not consider the issue as "Pimcore:Priority", "Pimcore:ToDo" or "Pimcore:Backlog", so we're not going to work on that anytime soon. Please create a pull request to fix the issue if this is a bug report. We'll then review it as quickly as possible. If you're interested in contributing a feature, please contact us first here before creating a pull request. We'll then decide whether we'd accept it or not. Thanks for your understanding.

@markus-moser
Copy link
Contributor

The reason why the generic data index bundle defines it's own entity manager is because all other pimcore bundles which use ORM do it the same way with their own entity manager, therefore we did the same approach here.

The other bundles are mostly enterprise bundles (like copilot, backend power tools, translation provider interfaces, ...) but also in the execution engine of the core we use this approach:

https://github.com/pimcore/pimcore/blob/e3d6e19e304bd62421b9efb3cb03df570a25127f/bundles/GenericExecutionEngineBundle/config/pimcore/doctrine.yaml

We should not mix the approaches and therefore we would need to adopt it in all bundles. At the moment this is too much effort for only a little effect so we decided to stay with this approach for now. Also to be honest I'm still not 100% sure if it is the better approach to use the default entity manager in a bundle. In my opinion it's ok to define a separate one and not a "bug". Other bundles should be built in a way that they do not get destroyed when one bundle defines it's own entity manager.

The consequence is that you need to enable the default entity manager like discribed here when it's needed:
#209 (comment)

If you see other drawbacks of using a separate entity manager please let me know.

@dpfaffenbauer
Copy link
Contributor Author

@markus-moser Ok. Fine. If I see side-effects or something, I let you know.

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

No branches or pull requests

4 participants