Skip to content

Save transaction data before state change#22

Open
BushraAsif wants to merge 5 commits intomainfrom
save-transaction-data-before-state-change
Open

Save transaction data before state change#22
BushraAsif wants to merge 5 commits intomainfrom
save-transaction-data-before-state-change

Conversation

@BushraAsif
Copy link
Copy Markdown
Contributor

No description provided.

Comment on lines +294 to +302
$customFields = array_merge(
$customFields,
[
self::ALTAPAY_TRANSACTION_ID_CUSTOM_FIELD => $altaPayTransactionId,
self::ALTAPAY_TRANSACTION_PAYMENT_SCHEME_NAME_CUSTOM_FIELD => $altaPayPaymentSchemeName,
self::ALTAPAY_TRANSACTION_PAYMENT_NATURE_CUSTOM_FIELD => $altaPayPaymentNature,
self::ALTAPAY_PAYMENT_ID_CUSTOM_FIELD => $altaPayPaymentId,
]
);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really necessary?

The way I see it, you just need to insert the custom fields you want to add using the repository:

https://developer.shopware.com/docs/guides/plugins/plugins/framework/custom-field/add-custom-field.html#filling-data-into-custom-fields

Rather than loading all the custom fields, merging them, and then inserting them again.

You risk that if the custom fields is changed while this operation is processing, then you will insert custom fields with an older value (race condition)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emicha ,
Order custom_fields are stored as JSON, but there is no clear guarantee that writing only part of that JSON will always keep the other existing fields.
There is also a Shopware core issue showing that existing custom_fields can be overwritten when a new array is written instead of merging first:
shopware/shopware#9613

We also need this to behave predictably across different Shopware versions used by merchants.
The safest long-term solution would be to use a separate table/entity instead of relying on the order custom_fields JSON. I have created a separate ticket for that:

https://developer.shopware.com/docs/guides/plugins/plugins/framework/data-handling/add-custom-compl…

So for this change, I think the explicit merge is the safer short-term fix, and the separate entity is the better long-term solution.
Ticket: PL-1598

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BushraAsif The issue you shared seems to be pointing at updating the full user, and not just a field using:

$this->swagExampleRepository->upsert([[
    'id' => '<your ID here>',
    'customFields' => ['swag_example_size' => 15]
]], $context);

I am not sure what you shared is applicable here. Please discuss with @mateusz and @ovidiu in my absence.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emicha @ovidiuba the information I shared is not related to the user field, it belongs to the order field. I'm attaching the order details screen for reference. As shown in the screenshot, the custom_fields data is saved as JSON, which is why I merge the existing data before saving it again.

That's why we read custom fields before writing to it, so we don't mess with other plugins saving data to the custom field.

image

To properly handle the race condition you mentioned, we need to create a custom table according to the Shopware documentation:
https://developer.shopware.com/docs/guides/plugins/plugins/framework/data-handling/add-custom-complex-data.html

Ticket: PL-1598

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants