craftcms / commerce

Fully integrated ecommerce for Craft CMS.
https://craftcms.com/commerce
Other
227 stars 170 forks source link

ShippingMethodOption error breaking checkout #1378

Closed keyurshah closed 4 years ago

keyurshah commented 4 years ago

Description

It seems with the latest craft 3.1.1, an error is being caused during shipping calculation.

This error is critical and prevents adding to cart and checking out.

"Setting unknown property: craft\commerce\models\ShippingMethodOption::provider"

This error does not always occur though. It seems to happen when user is logged in.

We are using verbb/postie to handle shipping.

https://github.com/verbb/postie/issues/27

It seems that it is with this commit

https://github.com/craftcms/commerce/commit/4f139cd19c83eff78b36847016c4999672f98993

Steps to reproduce

  1. Add an item to cart
  2. Error is returned from json

Additional info

engram-design commented 4 years ago

As @keyurshah states this is from https://github.com/craftcms/commerce/commit/4f139cd19c83eff78b36847016c4999672f98993 and specifically https://github.com/craftcms/commerce/commit/4f139cd19c83eff78b36847016c4999672f98993#diff-a71a5371ab7c5db4ebcffcab49d15a0fR1573 where its using all the attributes in a shipping method class, and trying to set the respective attribute on the ShippingMethodOption class.

This issue with Postie, and I'm sure any other custom shipping method, is that there will almost certainly additional attributes on classes. Basically, it means we can't have any public attributes on the shipping method class that doesn't exist in the ShippingMethodOption class.

Probably needs to be smarter than $method->attributes - unless I can do something? I can't make the attributes in my shipping method class private/protected because they need public access.

Let me know if I should be doing something on Postie's end?

Refer to https://github.com/verbb/postie/issues/27

lukeholder commented 4 years ago

Yeah looks like a bug. Will fix shortly.

lukeholder commented 4 years ago

This has been fixed for the next release.

Thank you for bringing this to our attention.

A fix has been pushed and will be included in the next release.

To get this early, change your craftcms/commerce requirement in composer.json to:

"require": {
  "craftcms/commerce": "dev-develop#09f66725e201f28bb2b59c65afc374329c1c0952 as 3.1.1",
  "...": "..."
}

Then run composer update

Thanks.

engram-design commented 4 years ago

@lukeholder Tested and working, nice one!

engram-design commented 1 year ago

@lukeholder Something seems to have changed to make this no longer work.

Again, Postie provides its own verbb\postie\models\ShippingMethod model which represents an option for people to pick from a remote provider. These get created as ShippingMethodOption models via Commerce, which essentially guts anything Postie does in its model class, and swaps for a craft\commerce\models\ShippingMethodOption model, which inherits from craft\commerce\base\ShippingMethod (which our Postie classes also do).

This population of a new model is where things fall over. Because these new ShippingMethodOption classes don't use our Postie-provided shipping method classes, a lot of the extra data is lost.

Might I recommend storing the shipping method that's being used to create the shipping method option? I know that one extends the other, but it doesn't factor in any third-party setups.

$option->shippingMethod = $method;

Or, maybe another approach is to add an event for us to hook into which is a before/after populate/create ShippingMethodOption where we can add our own data? It'd still require properties to store that stuff.

Lastly, we could try and figure out a way for Postie to implement it's own ShippingMethodOption (probably needs to be a new base class and interface) that gets used instead of the Commerce ShippingMethodOption class. This would bring things back to the Commerce 3.x method, where Postie can actually control the options used for users to pick shipping options for.