TypeError: Argument 1 passed to _editor_get_file_uuids_by_field() must implement interface Drupal\\Core\\Entity\\EntityInterface

I experienced the same error when programmatically updating a revisionable entity within it's update hook and not creating a new revision. Your patch fixes the issue.

However, I'm wondering, why not using an elseif instead of wrapping the contents of the else block in another if condition?

Thanks. It works

Just needs skip process if $entity has no original.

I experienced the same error when programmatically updating a Commerce order due to change the order state transition in OrderEventSubscriber. Your patch fixes the problem. Many thanks!

In order to commit a bug fix we need an automated to test to prove that we've fixed the bug and ensure that we don't break it again in the future. For more information about writing tests in Drupal 8 see the following links:

1. https://www.drupal.org/docs/8/testing
2. https://api.drupal.org/api/drupal/core%21core.api.php/group/testing/8.7.x

Also I'm not entirely sure the change is correct. I think we need to get to the second part of the if and record file usage and do something like

    $original_uuids_by_field = empty($entity->original) ? [] : _editor_get_file_uuids_by_field($entity->original);

This error can happen when you try to save an entity when it is already in the process of a save.

For example:

/**
 * Implements hook_entity_update().
 */
function mymodule_entity_update(EntityInterface $entity) {
  // Avoid infinite loop by only going through our post save logic once.
  if (!empty($entity->mymodule_updating)) {
    return;
  }

  // Set flag for whether or not the entity needs to be resaved.
  $needs_update = FALSE;

  // Perform our post save logic.
  // (...)

  if ($needs_update) {
    // Set flag on entity that our logic was already executed.
    $entity->mymodule_updating = TRUE;
    // And resave entity.
    $entity->save();
  }
}

\Drupal\Core\Entity\EntityStorageBase::doPostSave() ends with unsetting $entitiy->original. So following the code example above, before $entity->save(), $entitiy->original exists, but afterwards it doesn't.

After the function above is finished, Drupal continues invoking the entity update hook that was started after the first entity save call.

In my opinion it is bad practice to save an entity when it is already in the process of being saved. In theory, this can even lead to infinite loops: an entity save call could result into another save call for the same entity, which could result into yet another save call for the same entity, and so on.

One way to avoid the bad practice could be to enforce disallowing saving an entity in presave, insert and update hooks: throw an exception in \Drupal\Core\Entity\EntityStorageBase::save() when the entity is already in the process of being saved.

However, this may be too disruptive. Would there be any legit reason to resave an entity in these hooks? The patch attached isn't necessarily meant to be committed, but an attempt to find out if such legit reasons actually exist in core.

So resaving an entity is a valid use case in an entity insert hook. Good to know. Is that also the case for an entity update hook? Let's find out.

There is no example in a core (test) module that resaves an entity in an update hook. The test failure from #16 is caused by not having the "is_updating" property reset on an exception during saving.

So the question now is: should resaving an entity in an update hook be supported?

If not, then this issue can either be closed as "won't fix" or we could decide to trigger a warning whenever an entity gets resaved in an update hook. By using a warning instead of throwing an exception we would keep backwards compatibility.

I would say it should not be supported to resave an entity in an update hook because of the risk to cause an infinite loop (entity save > invoke update hook > entity save > invoke update hook > entity save, etc.).

Opinions?

I tend to agree with you MegaChriz, if a developer is attempting to call $entity->save() within an presave/update/insert then I think that's asking for trouble.

Surely in most cases it's enough to use hook_entity_presave() to set the properties required and let the original save() call save that data?

I have a use case for re-saving an entity in _update()

I have a parent entity, which has a field with references to child entities.

The parent has a status field.

When ever a child is updated, I need the parent to check all children and depending on the state of the children, this will alter the status of the parent.

So when a child is updated, in the _update() method saves the parent: $parent->save()

In the parents _update() method, it loops over all children and if the status of the children hit a different criteria, the parent will its status field and then save itself to take effect.

My logic is sound as to avoid infinite loops.

I also have several use cases in my Drupal application to call $entity->save from an update hook.

I don't know any other event or hook to use to trigger my business logic... It would be very good to have a hook that works after an entity is safely stored and I can do some changes on it, e.g. setting a custom field to "processed" when several other states are matched.

https://www.drupal.org/node/2892654 seems to be relevant.

I think this issue is clearly a bug that should be fixed, as the original code is not very clear and this error caught an edge case.

It built a new patch with Alex' suggestion from #12.

Still needs tests.

Adding this extra check as I found it can pass a null value into the function _editor_record_file_usage

Nice, I ran into the same problem, and have an additional catch: In my case I got a PHP notice that some fields were not present in the original entity.

Patch 27 solved my issue.

Patch #27 solved my issue too. Thanks a lot.

In my case I was saving an order in a post_transition event subscriber. I came to this issue from https://www.drupal.org/project/commerce/issues/2961440

patch #27 remove the error but doesn't solve my problem.
Like @samtimg in #31, I was trying to save an entity in a state_machine post_transition event subscriber; but with the added complicatie that I was trying to perform another transition inside the event subscriber.
In other words:

1-[Button click] $stateItem->applyTransitionById('id1')
2-[Inside state_machine post_transition event subscriber] if ($something){$stateItem->applyTransitionById(id2);$entity->save();}

After patch #27, the entity was well saved, (with the transition 'id2' applied) BUT, the post_transition event of the 'id2' transition was never called.

FYI: I have tried to put my code (containing the transition'id2') in a destruct() function (implementing DestructableInterface) like explained by https://www.drupal.org/project/commerce/issues/3044681#comment-13220754 without luck

I end up by tweaking my code at the form level, which is not very nice

NW for adding test(s)

I end up here again:

First time (#32) : When I was trying to save an entity in a state_machine post_transition event subscriber; but with the added complicatie that I was trying to perform another transition inside the event subscriber.
=>As I could not get rid of the error, I decided to handle my logic at the form level... and I unpatched #27

Now : Like #5, I am updating (and saving) a node in hook_node_update (could not do it in hook_presave due to the fact that I must update the node itself and its' translations which isn't supported by core)

#27 save my day: thank you!!

Got this same error on Drupal 9 when programmatically updating translated nodes.

Hopefully after applying patch #27 the issue is fixed for me. Thankyou

As per #33:

NW for adding test(s)

Could someone post steps to reproduce this in the issue summary, starting from a clean installation of Drupal core? If we had those, it would be a lot easier to puzzle out how to write a test.

Hello, here you can see an example:

https://www.drupal.org/project/commerce/issues/2961440

@phenaproxima
I updated the issue summary to include the steps to reproduce. I also added a test to the issue branch "2974156-type-error-tests_only" to demontrate the issue.

@MegaChriz: The testing in the MR looks good. What we are missing is the fix. Usually we do 2 patches with the first one having the fix and the testing and the second patch only has the testing. The second one should fail the testbot, like your MR is doing. The first patch should pass the testbot. Writing the testing is almost always the most work and that is done. Almost there.

I think the fix should be something like:
$original_uuids_by_field = !empty($entity->original) ? _editor_get_file_uuids_by_field($entity->original) : [];

@daffie
There is a possible fix in #27. To me it looks like it is fixing a symptom of the problem, the problem being that resaving an entity in an update hook results in a process like the following:

1. \Drupal\Core\Entity\EntityStorageBase::doPostSave() gets called.
2. editor_test_entity_update() gets called.
3. A resave of the updated entity gets triggered (second save call).
4. \Drupal\Core\Entity\EntityStorageBase::doPostSave() gets called.
5. editor_test_entity_update() gets called.
6. editor_entity_update() gets called (caused by the second save call).
7. editor_entity_update() gets called (caused by the first save call).

In the process above you see that two save calls on the same entity happened. And also two times hook_entity_update() gets invoked. The perhaps weird thing is that the first time editor_entity_update() gets called, it is triggered by the second invocation of hook_entity_update(). The second time it gets called, it is triggered by the first invocation of the same hook. It is at this point that $entity->original no longer exists, because it gets unset in \Drupal\Core\Entity\EntityStorageBase::doPostSave() after the second hook invocation is done (and the first hook invocation has yet to complete). See https://git.drupalcode.org/project/drupal/-/blob/9.3.x/core/lib/Drupal/C...

But perhaps we're okay with that a process like above can exist. In that case the fix in #27 is probably the way to go (I haven't tested that one yet).

The fix and the added testing look good to me.
The added testing will fail the testbot when the fix is removed. With the same error as the one from the IS.
For me it is RTBC.

Committed and pushed cd735be4dd to 9.3.x and b5cfa7cfa4 to 9.2.x. Thanks!