search

pimcore / generic-data-index-bundle

Other
4 stars 4 forks source link

Doctrine ORM #164

Open dpfaffenbauer opened 5 months ago

dpfaffenbauer commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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.

github-actions[bot] commented 4 months ago

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.