Customer profile copy calls order save on stale object

This seems a bit sketchy... any ideas why you'd have to load the order and override the form_state with it? Is it in entitycache and not updated or something maybe?

I used this patch to solve an issue of order discounts not being applied after checkout https://www.drupal.org/node/2328357#comment-10095740 but manually added the code into my production version 7.x-1.11.

Any chance of a commit to the most current production version or at least a patch?

@capfive if you believe the code is solid and you've tried it then you should change the status to "Reviewed and tested by the community" aka RTBC.

Although to me there is a bit of an underlying problem here that needs to be solved and this is just a bandaid which may mask that problem and make it re-appear more troublesome ways.

Regardless there is a patch on the issue so the status should be at very least "needs review"

I can confirm, that this patch resolves issue which capfive referenced, we are at least three to confirm this patch works so changing status to RTBC. Also changing priority to major because Commerce Discount module needs this patch to proper working (used on almost 10k sites).

I don't use this patch and don't believe it resolves that particular problem and still concerned.

@parisek can you explain how this code resolves the problem?

I was experiencing the issue outlined in https://www.drupal.org/node/2328357

Applying the above patch resolved this issue for me.

@joel #2:
> This seems a bit sketchy...

I also think so and there might be a pattern in it. The backandforthlinking of order and line item is a fragile thing. And modules must coordinate what they do to it.
What about something like this:

/**
 * Static cache for "the official" order wrapper. Modules should use this if they manipulate the order. It also cares for autosaving the order if changed.
 */
function commerce_order_the_official_wrapper() {
...
return $order_wrapper;
}

(maybe also include the line items array?)

@dpolant: Can you shed a little more light on this (as per #2 and #9)?

To me, this sounds very much related to #1804592: During AJAX form submission in checkout, the $order argument passed by the Form API is incorrect. (despite "Ajax" being in the subject). It seems that a lot of developers experience issues whenever they are attempting to update or otherwise manipulate an order that has been stored in the checkout $form array. In that ticket, we had discussed always loading the order object fresh, rather than using the data that was stored in the form, since the form data can be stale.

#9: If you look at #1804592: During AJAX form submission in checkout, the $order argument passed by the Form API is incorrect. you'll see that the idea of a centralized API method for manipulating an order is discussed, but I don't know yet if there's been any real debate on this topic. It seems "just load the $order fresh" has been the answer so far.

@joelpittet: Have you checked out that issue at all? The common denominator seems to be $order being stored in the $form. Not sure what else can be done, other than a centralized API for ensuring/enforcing that $form data is always current - though I'm not sure how feasible that is.

I dug a little bit deeper, and I think that this approach is valid (even if a bit kludgy). The function commerce_customer_profile_copy_validate() is called as long as a billing or shipping address pane has been configured to allow copying that data from another pane. As long as that evaluates as TRUE for one of those panes, that validation hook is fired when the checkbox is rendered.

$pane_form['commerce_customer_profile_copy'] = array(
        '#type' => 'checkbox',
        '#title' => t('My %target is the same as my %source.', array('%target' => $target_profile_type['name'], '%source' => $source_profile_type['name'])),
        '#element_validate' => array('commerce_customer_profile_copy_validate'),
...

Since there's apparently no way to send $order to the validate function by reference, commerce_customer_profile_copy_validate() relies on manipulating $form_state['order']. It seems to me, despite the fact that "always loading a fresh copy of the order" seems to be the best way to go, and it has been mentioned that Contrib should do this, even Commerce core does not follow this as a standard. These quotes from @mglaman in the referenced issue are helpful:

It seems the biggest issue here is that modules aren't always grabbing up to date data and might be getting something stale from the form state. I wonder if this is a works as designed and there just needs to be some good old elbow work done in the contributed project space.
...
So accessing the entity from $form_state seems to be the pretty standard thing to do in core -- albeit it isn't even standardized. Node sends $form['node'] and $form_state['node'] for instance. I think it falls "what the hell system am I in, where do I get the entity?!" and having API docs strictly state "Load a fresh copy. Always, especially in Commerce where fresh data is crucial." The latter is why I always load fresh, because you never know, even though objects are passed by reference.

Would love to discuss this further, and would love even more to get this issue fixed in Commerce core, and better yet, have someone from Commerce Guys figure out exactly what the best solution going forward is. Personally I don't trust form data at all anymore, and we should really move all instances of $form_state['order'] to some sort of centralized API method that we can use to better ensure our data integrity.

I mean, it works okay for the majority of the time, but these edge cases make it obvious that this approach is rather brittle.

@dpolant:

I just realized that this patch helps to fix the "incompatibility" introduced by the patch in #2026321: Avoid overwriting an already updated order.

Tagging this issue as well, though its symptoms seem to point to a larger issue. However the patch here seems to have helped us and others, so might be worth investigating more closely.

Linking to possibly related issue due to form state #1804592: During AJAX form submission in checkout, the $order argument passed by the Form API is incorrect..

Can someone experiencing the issue test the patch in #1804592: During AJAX form submission in checkout, the $order argument passed by the Form API is incorrect. to see if that fixes the problem? I don't think we should be handling this at the contrib level since so many modules are experiencing the same issue, so removing RTBC

#10:
> #9: If you look at #1804592: During AJAX form submission in checkout, the $order argument passed by the Form API is incorrect. you'll see that the idea of a centralized API method for manipulating an order is discussed, but I don't know yet if there's been any real debate on this topic. It seems "just load the $order fresh" has been the answer so far.

That's the core problem: We need a single point of truth for the order (and its line items!). If it's the DB, all the gazillions of modules that manipulate the order will produce a gazillionplex of DB traffic and performance nightmares... Isnogood...

Also note that a cached version of the order is not enough, we also need the line items, which for some reason are referenced and referencing.

@axel #15: Have you tested using the latest patch in #1804592: During AJAX form submission in checkout, the $order argument passed by the Form API is incorrect.? It so far has solved all of my issues.

We need a single point of truth for the order (and its line items!). If it's the DB, all the gazillions of modules that manipulate the order will produce a gazillionplex of DB traffic and performance nightmares... Isnogood...

I don't think it'll be an issue, since the idea is to perform a commerce_order_load() with every checkout pane form build, that is covered by entity_load()'s built in static caching. At least, I think. If not then we may consider just putting the result of commerce_order_load() into a static variable, and checking the timestamp to see if we need to modify the static version. Seems like it'd still be fairly quick code, but I could be wrong. Please test the patch and let us know if it works for you.

@torgosPizza (comment 16) - I can confirm that the fix brought in for #1804592: During AJAX form submission in checkout, the $order argument passed by the Form API is incorrect. does not fix this one. That can help fix issues caused during the form build (e.g. information returned from an AJAX call). But anything updating the order object during the form process cycle (including on validation, such as this particular one for copying the customer profile) runs before that. The order is just coming from the AJAX cache of the form, and there isn't chance to rebuild/replace that data before the validation hook is run, as far as I can see. It won't even be coming from entitycache, as the object is just part of the form state loaded from the cache_form bin.

So, yes, a 'global' solution would be ideal, but in the meantime this is still quite an issue! This issue's patch solves this particular instance for me. I really don't know how a wider solution could be made, aside from dropping $form_state['order'] altogether, to force validation/etc hooks to load the order afresh. Can we commit this patch for the short-term, and then follow-up to figure out a wider solution? It's crazy that the issue exists within commerce core, even if contrib modules might have similar issues.

Does anyone still use patch from current issue nowadays? Or this issue can be closed? I tried to reproduce but didn't notice any bugs.