While reloading $entity->original in the controller, track the save_depth

I'm not sure this is really an issue, but one potential drawback is that with this change in effect $entity->original may be different from the execution of one update hook to the next.

Imagine a scenario where two modules implement the update hook and need to take action if the status changes from X to Y. If hook 1 is executed and results in a save because the status changed from X to Y, hook 2 will no longer be able to identify that that status change occurred. As far as it knows, the original version and the current version would no longer reflect a status change.

The issue really seems to be related to recursive evaluations needing their own locally scoped "original" version of the entity. So the initial save has its "original" that gets passed to the update hook, and if hook functions result in an additional save, they have their own "original" that gets passed to its update hooks, etc. But as you go back up a step in the recursion, you recover the previous "original" version of the entity.

I'm not sure that works, though, because you might have an order travel from status X to Y and then be updated to Z as a result of that update but then have another hook at the top level that's looking for any status changes that go directly from X to Z - you'd then trigger a false positive.

tl;dr; I'm not really sure what the best thing to do here is - either way we end up having to workaround less than ideal situations. It seems like we may actually need a post-save event that occurs outside of the controller. : ?

I mean, that's what would happen, but is that what we actually want to happen? We're basically saying that only the first implementation of the insert / update hook can be assured of having an accurate "original" entity.

As it is, you could argue that yes, there may be a chained set of updates, but if you treat them all as one transaction, the original entity represents the status of the sucker before we initiated the update hook process.

Ok, I thought I had posted an intermediate update in here based on some real-time chat w/ Bojan, but I must've forgot. What I realized through discussion was that the current situation is broken for multiple hook implementations anyways when the first triggers a save, because when that save is finished, the original version of the entity is unset anyways. Looks like this:

I. Original Save has two update hooks, A and B.
  A. Executes, triggers another save.
    A. Executes again.
    B. Executes again, after which the order's original entity is unset.
  B. Executes but no longer has the original entity, resulting in notices if it's examined at all.

Now, the patch above did at least allow me to have a save in hook A triggered by the first save and a separate save in hook B triggered by the save in hook A. That was impossible before.

However, I still end up with notices, because the original entity is still unset:

I. Original save occurs, hooks A and B triggered.
  A. Triggers a save.
    A. Pass through.
    B. Triggers a save.
      A. Pass through.
      B. Pass through.
      [Save B finished, $order->original unset.]
    [Save A finished, $order->original unset.]
  B. Pass through but with notices when $order->original is checked.

This kind of gets back to my earlier idea that we need to track our location in the call stack to some effect. In keeping with the spirit of the current proposal, it seems that we don't need to retain and restore a previous "original" version of the entity but could simply postpone unsetting it until we're completing the original save.

I'm gonna think about this on the drive home and update the issue, because I still feel like that may break the conceptual promise of "original" for subsequent hook implementations - but my mind is tied in a knot at the moment and I'm not able to think straight. : P

Ok, here's what I worked out on the car ride home:

We've already said above that we're ok with losing the context of changes for subsequent hook invocations in n-1 levels of the call stack. With that assumption, we've said that we'll reload the original entity at the start of each save process, at least maintaining the change context in child hook invocations at n+1. However, we still need to have an original entity at subsequent hook invocations at n-1 levels.

Since we aren't worried about preserving the change context, let's do this: if at the end of a save process I detect that this save process is a child of a parent save process, instead of unsetting the original entity I will set it to a clone of the current entity itself. This means that those higher level subsequent save invocations should simply operate under the assumption that nothing has changed.

To make it easier to determine if this is the case, let's also add a property that indicates the current and original entities are the same. I'm thinking perhaps we add an "unchanged" boolean to the entity itself, so if $entity == $entity->original, $entity->unchanged == TRUE.

Example code hooks:

function commerce_cart_commerce_order_update($order) {
  dpm($order, 'HOOK A BEFORE');

  if ($order->original->mail == 'example@gmail.com' && $order->mail == 'example+0@gmail.com') {
    dpm('Updated e-mail to example+1@gmail.com.');
    $order->mail = 'example+1@gmail.com';
    commerce_order_save($order);
  }

  dpm($order, 'HOOK A AFTER');
}

function commerce_line_item_commerce_order_update($order) {
  dpm($order, 'HOOK B BEFORE');

  if ($order->original->mail == 'example+0@gmail.com' && $order->mail == 'example+1@gmail.com') {
    dpm('Updated e-mail to example+2@gmail.com.');
    $order->mail = 'example+2@gmail.com';
    commerce_order_save($order);
  }

  dpm($order, 'HOOK B AFTER');
}

Then I did a save to change the e-mail from example@gmail.com to example+0@gmail.com.

Example output (with save depth output in the controller):

- Save depth: 0
- HOOK A BEFORE => ... (Object) stdClass
- Updated e-mail to example+1@gmail.com.
- Save depth: 1
- HOOK A BEFORE => ... (Object) stdClass
- HOOK A AFTER  => ... (Object) stdClass
- HOOK B BEFORE => ... (Object) stdClass
- Updated e-mail to example+2@gmail.com.
- Save depth: 2
- HOOK A BEFORE => ... (Object) stdClass
- HOOK A AFTER  => ... (Object) stdClass
- HOOK B BEFORE => ... (Object) stdClass
- HOOK B AFTER  => ... (Object) stdClass
- Exit save depth: 2
- [Note: $entity->unchanged is now TRUE and $entity->original is a clone of $entity]
- HOOK B AFTER  => ... (Object) stdClass
- Exit save depth: 1
- HOOK A AFTER  => ... (Object) stdClass
- HOOK B BEFORE => ... (Object) stdClass
- HOOK B AFTER  => ... (Object) stdClass
- Exit save depth: 0

Patch attached. Wanna review?

The basic assumption now is that updates may be chained, but once you chain updates any other update hooks at level n-1 will no longer have an "original" entity to compare against that differs from the entity itself. However, so long as you're chaining them to the n depth, you're fine to implement whatever workflow you need (such as the one the original post required).

I'm not sure I understand how it's worse. You actually would have an indication in the entity itself that the conditions of a save have resulted in the original and the current now being identical.

The problem cropping up elsewhere in the queue is that saves are occurring via Rules, resulting in Rules failing hard looking for the metadata of the original entity that just isn't around any longer. If we could somehow just throw a meaningful exception, that'd be one thing, but your post is the first time I've ever found the explanation for a problem that's been around for a year despite debugging attempts from the folks affected.

So what's easier - documenting the parameter or documenting the debugging process?

Also, I think I had the same conclusion in #2 - we should really have a post-save condition specifically used for chaining order status progressions. Right now I feel like "After an order has been updated" (and related events) is a bit of a misnomer since the event fires as part of the order update process itself. It's even part of the same transaction. : P

I come from #1430796: Checkout Error: Warning: call_user_func(rules_events_entity_unchanged) (Rules 2.6 bug) and want to fix "Unable to evaluate condition data_is." warnings. After reading your comments here i'm not sure if patch #9 is still ready for review? Thanks for any help.

@bojanz I'm wondering what it would look like to implement a both / and solution. Don't do anything novel with $entity->original, allowing your patch to affect the current behavior as is, but add a $entity->last_saved property (along with the $entity->save_depth) that is always present to provide the additional data you might need when debugging this behavior. Maybe it's overkill?

For now, I've gone ahead and committed your patch in #1 with some extended code comments, as we can easily add the save depth stuff in a follow-up. I'm open to ideas on how you think this should be documented for site builders.

Commit: http://drupalcode.org/project/commerce.git/commitdiff/d91ef09

Working with Commerce and Rules it appeared a rule's data comparison could sometimes not read commerce-order-unchanged:status. The Drupal logs contained something like:

Unable to evaluate condition data_is.

and the Rules log showed:

...
302.421 ms Unable to load variable commerce_order_unchanged, aborting.
304.66 ms Unable to evaluate condition data_is. [edit]
...

After some digging this appeared to be because of Commerce Shipping's rule commerce_shipping_cart_update_delete which is being fired before the ones that could not be evaluated. This rule calls commerce_line_item_delete_multiple() via commerce_shipping_delete_shipping_line_items() which apparently saves the order entity. This save unsets the order's property with the original entity, thus Rules could not get the commerce_order_unchanged entity nor it's properties.

Workaround

Make sure that rules that depend on unchanged entities are fired before all others.

This is rearing its ugly head again on a site I manage how can we verify that is run last. I changed its weight to 10 however still no luck.

@cmriley I have same issue

@Imeurs Thank you for your detailed response. This was my exact case and your suggestion solved it. All I did was pushed the weight of my shipping calculation rule to 10 (originally 0 like the other rules) so that it would fire after the others). Appreciate it!