search

pimcore / ecommerce-framework-bundle

Ecommerce Framework community bundle provides e-commerce functionality such as product listing and filtering, pricing, carts and checkouts for Pimcore.
https://pimcore.com/docs/platform/Ecommerce_Framework/
Other
8 stars 30 forks source link

[Ecommerce Framework] Migration which enables generateTypeDeclarations #76

Open BlackbitDevs opened 2 years ago

BlackbitDevs commented 2 years ago

In pimcore/pimcore#8040 the interfaces for the data model classes of the Ecommerce Framework bundle were extended so that the getter methods require return type declarations, e.g.: https://github.com/pimcore/pimcore/blob/5661134f4bb8841dba8f6aa458277df89e50622c/bundles/EcommerceFrameworkBundle/Model/AbstractFilterDefinition.php#L33

Sadly there never was a migration for this. Consequently Pimcore crashes when you had the Ecommerce framework classes installed in Pimcore 6 (where generateTypeDeclarations did not exist yet) and update to Pimcore X. It also does not matter if you are actually using the Ecommerce Framework data object classes because they are loaded anyway once they got created and so the system crashes on every request: Declaration of Pimcore\Model\DataObject\FilterDefinition::getPageLimit() must be compatible with Pimcore\Bundle\EcommerceFrameworkBundle\Model\AbstractFilterDefinition::getPageLimit(): ?float This means that without generateTypeDeclarations being enabled the ecommerce framework classes will not work.

Also if you accidentically uncheck the generateTypeDeclarations checkbox in the class definition of one of the Ecommerce framework classes the same will happen.

In both cases you will not be able to access the Pimcore backend anymore and instead have to manually edit the class definition files and set generateTypeDeclarations to true.

A migration which enables generateTypeDeclarations for the corresponding classes would solve at least the problem when updating from earlier Pimcore versions to Pimcore X. I do not know if we can solve the problem of unchecking the generateTypeDeclarations checkbox manually.

dvesh3 commented 2 years ago

@BlackbitNeueMedien In the upgrade notes, it is mentioned that you to activate the generateTypeDeclarations check for all Ecommerce classes for migrations. https://pimcore.com/docs/pimcore/current/Development_Documentation/Installation_and_Upgrade/Upgrade_Notes/index.html#page_Changes

And there are migrations already exists which regenerates the class definitions on upgrade to Pimcore X(https://github.com/NiklasBr/pimcore/blob/10.1/bundles/CoreBundle/Migrations/Version20210702102100.php), it is not enough?

BlackbitDevs commented 2 years ago

@dvesh3 - it is always nice when things are documented, but isn't it even nicer when it just works? What speaks against a migration which enables generateTypeDeclarations for the Ecommerce classes? I would even create a PR, only wanted to check first if this had a chance to get accepted.

brusch commented 2 years ago

@BlackbitNeueMedien hmm, I'm just wondering ... can we really create a migration that adds type declarations without breaking existing code? So if some fixes the issue already manually I mean 🤔

BlackbitDevs commented 2 years ago

@brusch I cannot follow you. How can someone have fixed this problem already manually if not by enabling generateTypeDeclarations? The only alternative way how this could have een done is to keep generateTypeDeclarations disabled and then override the data object classes and add the return types to the getter methods there. But this then would also still work if we set up a migration which enables generateTypeDeclarations.

dvesh3 commented 2 years ago

@BlackbitNeueMedien we are ok with a migration that enables generateTypeDeclarations property for Ecommerce classes but remember it needs to re-generate these classes afterwards (and there are already migrations for Pimcore X that is doing the job).

stale[bot] commented 2 years ago

Thanks a lot for reporting the issue. The issue was not considered by us as "Priority" or "Backlog", so we're not gonna work on that anytime soon. In case this is a bug report, please create a pull request fixing the issue, we'll then review it as soon 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.

github-actions[bot] commented 7 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.