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

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

Open
jremmurd opened this issue Dec 20, 2021 · 15 comments
Open

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

jremmurd opened this issue Dec 20, 2021 · 15 comments

Comments

@jremmurd
Copy link
Contributor

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 jremmurd added the Bug label Dec 20, 2021
@jremmurd
Copy link
Contributor Author

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
Copy link

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 #76). The site was originally running on 6.8 but has been on 6.9 for a while now.

@fashxp fashxp reopened this Jan 31, 2022
@stale
Copy link

stale bot commented Mar 14, 2022

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.

@stale stale bot added the PR Welcome label Mar 14, 2022
@teklakct
Copy link

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
Copy link
Contributor

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

@teklakct
Copy link

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
Copy link
Contributor

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
Copy link

teklakct commented Feb 27, 2023

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
Copy link

@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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

"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
Copy link

teklakct commented Feb 28, 2023

@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,
  1. Rebuild db structure for classes bin/console pimcore:deployment:classes-rebuild
  2. 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

@brusch brusch transferred this issue from pimcore/pimcore May 31, 2023
@dvesh3 dvesh3 added Backlog and removed PR Welcome labels May 31, 2023
Copy link

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
Copy link

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

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

8 participants