Custom cart fields cannot be updated

jimkrutor commented 7 months ago

Hi Tobias, thank you for this awesome plugin, I love it!

I am not sure if this is bug or a feature, but I found an inconsistence between default and custom cart fields behavior. Please see below.

Steps to reproduce

Setup

Set custom cart field shipping in option ww.merx.cart.fields

Have a product page:

Id: test
----
Title: Test product
----
Price: 50
----
Shipping: 20

Add (or update) item with custom values for price and shipping

$cart->add([
'id' => 'test',
'price' => 99,
'shipping' => 33
]);

Expected behavior

price should be 99
shipping should be 33

Current behavior

price is 99
shipping is 20

The difference

Default fields have this check so they are fetched only once when item is added to cart and you did not provide any custom value. https://github.com/wagnerwagner/merx/blob/ec51526d1fba7233ac632b2c953397abee864416/src/ProductList.php#L102-L104

Custom fields are missing this feature and they are fetched from product page on every cart initialization, cart update or item update, making basically impossible to have any custom value in custom fields from different source than product page model. https://github.com/wagnerwagner/merx/blob/ec51526d1fba7233ac632b2c953397abee864416/src/ProductList.php#L125-L135

I would suggest to make this behavior consistent by adding the same check to custom fields too. The behavior will slightly change. I can imagine someone having setup based on page model methods and being dependent on this feature, so I am not sure if it should be done. But again, it is inconsistent with the default fields.

foreach (option('ww.merx.cart.fields', []) as $fieldName) {

    if (!isset($value[$fieldName])) { // <- add check if the field is present in updated data

        $field = $page->{$fieldName}();
        if (is_a($field, '\Kirby\Cms\Field') && $field->isNotEmpty()) {
            $value[$fieldName] = $field->toString();
        } elseif (
            $field === null ||
            is_scalar($field) ||
            is_string($field) ||
            (is_object($field) && method_exists($field, '__toString'))
        ) {
            $value[$fieldName] = (string)$field;
        }

    }

}

What do you think?

Thank you, all the best, Jakub

tobiasfabian commented 6 months ago

Thank you for your detailed description.

I updated the code and added a check for !array_key_exists($fieldName, $value) instead of isset(). This allows you to set a null value via $cart->add([…, 'shipping' => null]). (see 843238e1bf686b97d4af20534a6718b0e97b7a1a)

I think is_a($field, '\Kirby\Cms\Field') && $field->isNotEmpty() is not necessary because every Kirby\Cms\Field has a __toString method.

Let me know if this works for you.

This change is published in v1.8.1

jimkrutor commented 2 months ago

Hello Tobias, sorry for late reply, I just got to actually test this – this is perfect and it works as expected. I really appreciate you thought about about the null values, i am sure it will come in handy in the future.

Thank you very much, Jakub

tobiasfabian commented 2 months ago

Hey Jakub, thanks for your test and your feedback.

wagnerwagner / merx