AutoShipment observer process overrides earlier saved order instance

[vinodsowdagar]

The DHLParcel\Shipping\Observer\Autoshipment class is subscribed to the 'sales_order_save_after' event.

The event is dispatched in \Magento\Framework\Model\ResourceModel\Db\AbstractDb::save after the save action has been executed. The save operation happens using a database transaction and this event does not wait for the transaction to be committed.

Normally this wouldn't be a problem, because the afterSave() function in Magento\Framework\Model\AbstractModel updates its own stored data. So whenever you will use the order instance retrieved from the observer object itself, it will have the correct values that were actually supposed to be saved. You can see how that works here: https://github.com/magento/magento2/blob/cd08acf3e9f8ed73cb23a807e9960055c465c477/lib/internal/Magento/Framework/Model/AbstractModel.php#L837-L849

However, you are getting a fresh instance from the database through the OrderRepository and this instance doesn't have all the data yet that the one from the observer has: https://github.com/dhlparcel/plugin-magento2-release/blob/master/Observer/AutoShipment.php#L79

On line 96 you are saving the order yourself, but that is an older instance of the order without all the data which was passed through the 'sales_order_save_after' event. This eventually results in the AutoShipment observer overriding certain values: https://github.com/dhlparcel/plugin-magento2-release/blob/master/Observer/AutoShipment.php#L96

You can see this example happening if you try to get the order, change the state and status and then try to save it through the OrderRepository. When debugging your AutoShipment observer, we can clearly see it happening.

The observer order has the correct values (status and state processing):

But the one retrieved through the OrderRepository has not (status and state pending_payment):

I think this can maybe be fixed by either using the 'sales_order_save_commit_after' event, because that one actually seems to be implemented with a callback. More information about the differences of both of these events can be found in this article:
https://techflarestudio.com/what-is-the-difference-betweent-save_after-and-save_commit_after-events-magento-2/

Maybe another solution could also be to use a plugin after class instead of an event, but I haven't tested this.

If there are any questions, let me know.

[roerlemans]

Thanks for the detailed feedback and explanation. We will look at the problem and make adjustments where necessary.

Thanks again.

[Stefvanhezik]

Hello @Vinod-MultiSafepay,
We have looked into your comment and are not entirely sure what the issue would be that we are solving (and what the impact of this issue is). Could you possibly elaborate on this?

Thank you in advance

[Skoot08]

Hi @Vinod-MultiSafepay,
Would you be able to answer the question of @Stefvanhezik? We would like to close this or resolve it, but we will need to understand what the problem is you are trying to fix.

[vinodsowdagar]

Hi @Skoot08 , @Stefvanhezik ,

The problem is that if someone wants to save an order that is in pending_payment state that does not have a shipment yet with the OrderRepository, your AutoShipment feature will override the initial order Object that was passed, so all of the changes that were made before someone called the save() are then lost.

A workaround we are suggesting to people having this issue right now is to save the order twice. Once you will override it and the second time it will be saved like it should, because your AutoShipment feature already created a shipment and will then return early. But this is not a real solution to the original problem.

[davidtabat]

Guys here is a fix (if you notice I also modified the Magento core class, which is not ideal of course, but it is a quick workaround, otherwise I would have to rewrite the whole module's architecture, which breaks the whole point of using a third-party extension :D - we use third-parties because it's cheaper and more efficient than reinventing the bicycle and doing everything from scratch - by ourselves D )
https://github.com/davidtabat/dhl-parcel-patch/blob/main/dhl-parcel-fix.patch

[davidtabat]

The thing is that basically, we don't need to load new order objects via the repository because it contains outdated data. We can continue working with observer-provided objects, however, the problem here is that the Magento default shipment loader class itself loads orders by ID via the repository. To overcome that constraint I updated that method to support passing order objects as well. Also make that parameter not required so that other parts of Magento, where this loader is used, don't break.
and of course, subscribing to order sales_order_save_after is wrong, we should subscribe to sales_order_save_commit_after event as pointed out in the thread already.

as I mentioned, this is a workaround - the ideal solution would be to execute this code when order data is already updated in DB and the calling repository in the Magento core class would provide correct data. That however would not be achievable via observer, and in general, observers are not designed to modify objects themselves - we should use plugins for that purpose, so I hope module maintainers will fix it in the future.