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

[Bug]: Type mismatch in EcommerceBundle installed classes #75

Open jremmurd opened 2 years ago

jremmurd commented 2 years ago

Expected behavior

Installed classes of EcommerceBundle match the provided abstract classes.

See i.e. https://github.com/pimcore/pimcore/blob/10.x/bundles/EcommerceFrameworkBundle/Resources/install/class_sources/class_OnlineShopOrderItem_export.json#L289

vs.

https://github.com/pimcore/pimcore/blob/10.x/bundles/EcommerceFrameworkBundle/Model/AbstractOrderItem.php#L72

Actual behavior

An error occures

Compile Error: Declaration of Pimcore\Model\DataObject\OnlineShopOrderItem::getTotalPrice(): ?float must be compatible with Pimcore\Bundle\EcommerceFrameworkBundle\Model\AbstractOrderItem::getTotalPrice(): ?string

Steps to reproduce

Create OnlineShopOrderItem and extend from AbstractOrderItem of EcommerceBundle.

See https://github.com/pimcore/pimcore/blob/10.x/bundles/EcommerceFrameworkBundle/Resources/install/class_sources/class_OnlineShopOrderItem_export.json#L289

jremmurd commented 2 years ago

The Issue only occurs if the settings for the number field in the OnlineShopOrder Class are changed. If there are max. decimals defined the type changes from float to string and the definitions are compatible.

ThisIsJustARandomGuy commented 2 years ago

Can we reopen this? I didn't change any of the values from the default and they were still empty. So I got the same error after trying to upgrade (in addition to pimcore/ecommerce-framework-bundle#76). The site was originally running on 6.8 but has been on 6.9 for a while now.

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.

teklakct commented 1 year ago

This is "silly". In Upgrade_Notes for v10.0.0 we can read that:

[Ecommerce] Changed price fields totalNetPrice and totalPrice of OnlineShopOrderItem to decimal.

In the generated class definition for DataObject OnlineShopOrderItem typings are correct but in the extended class but in the AbstractOrderItem from Ecommerce framework, you still have wrong typing.

Really annoying that your code is not compatible on its own.

jdreesen commented 1 year ago

Why don't you provide a PR to fix it?

teklakct commented 1 year ago

I'm new to Pimcore and really don't know how it exactly works. I can prepare a MR with the changes of typing in [AbstractOrderItem](https://github.com/pimcore/pimcore/blob/v10.5.17/bundles/EcommerceFrameworkBundle/Model/AbstractOrderItem.php#L82) but I'm not sure if this is enough. @jdreesen if you can be my wingman I can do even in upcoming minutes.

jdreesen commented 1 year ago

Actually, I'm pretty sure the return type of AbstractOrderItem::getTotalNetPrice(): ?string is correct. Are you sure your totalNetPrice field is a number type with decimalSize and decimalPrecision configured? If they're not set, the type will be float, if they are set, it will be string.

grafik

teklakct commented 1 year ago

Thanks. In my setup decimal size and precision are defined

Screenshot 2023-02-28 at 00 10 33

and the generated class methods are typed as following:

public function getTotalNetPrice(): ?float
public function setTotalNetPrice(?float $totalNetPrice)
public function getTotalPrice(): ?float
public function setTotalPrice(?float $totalPrice)

So the generated class works perfectly find, but the my problem is related with the Abstract class

class OnlineShopOrderItem extends \Pimcore\Bundle\EcommerceFrameworkBundle\Model\AbstractOrderItem

where in AbstractOrderItem we have

    /**
     * @return string|null
     */
    abstract public function getTotalPrice(): ?string;

    /**
     * @param string|null $totalPrice
     */
    abstract public function setTotalPrice(?string $totalPrice);

    /**
     * @return string|null
     */
    abstract public function getTotalNetPrice(): ?string;

    /**
     * @param string|null $totalNetPrice
     */
    abstract public function setTotalNetPrice(?string $totalNetPrice);

I was surprised that even in OrderManager there is a TODO about the price type.

I try to find a way how to check if there a string or float is used in OrderManager


BWT Storing $ as a decimal should be illegal ❗

teklakct commented 1 year ago

@jdreesen can you guide me if there could be sth in my field configuration or in code? I am blink right now.

And, I did not write it before. I'm using Pimcore v10.5.17 and use MariaDB 10.5 if this change anything

jdreesen commented 1 year ago

I'm sorry, I don't know what can cause this at the moment, it's very strange.

Hopefully someone of the pimcore team can help here? @dvesh3, etc.

teklakct commented 1 year ago

I found that in my definition_OnlineShopOrderItem.php the Fieldset children list of Price Information the totalNetPrice and totalPrice are defined as Pimcore\Model\DataObject\ClassDefinition\Data\Numeric with 'fieldtype' => 'numeric', I am not sure why this is happening. Will try to trace it and will let you know.

jdreesen commented 1 year ago

"Numeric" should be right. The generated scalar types depend on the config, this is where they come from: https://github.com/pimcore/pimcore/blob/10.5/models/DataObject/ClassDefinition/Data/Numeric.php#L152-L163

teklakct commented 1 year ago

@jdreesen 🎖️ for you.

I found it. Thank you a lot !!!!

tl;dr Data migration to Pimcore X failed.


So what happened?

After migration settings for decimal size and precision of totalNetPrice and totalPrice in OnlineShopOrderItem was missing (null). Because of that phpType was wrong. Also, decimal size and precision were defined for amount, but should be null. Because of that class was generated wrongly.

How to fix that

Maybe it could be useful for someone.

must be confirmed by works for me

Option one -> in code

  1. Open definition_OnlineShopOrderItem.php and find definitions for totalNetPrice and totalPrice
  2. Setup decimal size and precision, or defined it if missing: e.g.
    Pimcore\Model\DataObject\ClassDefinition\Data\Numeric::__set_state(array(
     'name' => 'totalPrice',
     'title' => 'Price',
     // ...
     'decimalSize' => 19,
     'decimalPrecision' => 4,
  3. Rebuild db structure for classes bin/console pimcore:deployment:classes-rebuild
  4. Rebuilds php files for classes bin/console pimcore:build:classes

Option two -> in admin

  1. Open Setting -> Data Objects -> Classes -> E-commerce and find totalNetPrice and totalPrice in PriceInformation.
  2. In "Specific settings" setup decimal size and precision e.g. Screenshot 2023-02-28 at 15 37 56
  3. Save changes
  4. Rebuilds php files for classes bin/console pimcore:build:classes
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.

arrabiata-asanz commented 1 month ago

we had a similar problem (Exception: class must be compatible with a Ecommerce Abstract Class) with getPageLimit() from \Pimcore\Bundle\EcommerceFrameworkBundle\Model\AbstractFilterDefinition in Pimcore v10.0.9. To solve it we had to set in the DataObject\defintion_ file generateTypeDeclarations to true. Maybe it helps others with similar problems