Saving an order with unsaved order items results in tough-to-debug data inconsistency

Ran into this in recurring, namely 📌 Undo temporary workaround on order item/order save process Active . In that case, recurring does a lot of "programmatic" order building that is not spread over multiple bootstraps/client page views, and so the $order object that you are building, adding (unsaved) order items to, rebulding, recalculating, and saving results in an incorrect order total.

I believe the real issue here is that Drupal's core Entity API has no problem saving a yet-unsaved entity reference when the hosting entity is saved. For commerce, since we maintain a lot of bidirectional entity references, this means EntityReferenceItem::hasNewEntity() will return false when called during the order save... because the order item was just previously saved by EntityReferenceItem::preSave().

I did a little experimenting with explicitly setting the is-new value with ::enforceIsNew() but I started getting errors about a duplicate insert into the database and backed off. Could still be a path forward for us but perhaps it's a little too cute.

The big issue here is DX. You think the thing will work, because it really should and does in this way around the rest of Drupal... but Commerce is really doing wild stuff behind the scenes and can't take it. It doesn't fail, though, leading you to know somehow that you need to always save an order item before it goes on an order.

Since 3.x is open now this is an opportunity for BC breaking changes (if we can't stomach throwing these exceptions in 2.x) but I think perhaps adding this in 2.x would be a good idea to uncover errors that are very likely in the wild, today, e.g. wrong order totals.

Upping priority and changing component.

Filing this against 3.0.x because I think that's the correct thing to do, but this should hopefully also get a backport to 2.x. Blocks 📌 Undo temporary workaround on order item/order save process Active on Recurring which allows us to remove an ugly work-around and fix some test coverage.

Test failures have me worried that perhaps some code is depending on "broken" functionality? Hmm.

I opened 📌 Stop unsetting the order ID reference from the order refresh Active a while ago... The patch is actually applied on one of the projects I'm working on but the tests are failing, so never actually committed the change...

Re: #9 - yeah, I tinkered with that a bit as well (simply removing it seemed to yield the promised crash) but I think these are separate but similar issues?