In contrib it is common for workflows to be implemented by resaving the entity in a hook_entity_update() or a matching Rule.
So if my commerce order needs to go through 4 different statuses, and can't skip them by just doing hook_entity_presave(), it is going to do do a status change and a save as the last hook_entity_update() operation / rule.
The code that initializes $entity->original is contained in EntityStorageBase::doPreSave(), the method called before hook_entity_update() implementations are invoked, which contains the following code.
// Load the original entity, if any.
if ($id_exists && !isset($entity->original)) {
$entity->original = $this->loadUnchanged($id);
}
This prevents $entity->original from being reloaded, so when I react on a fourth status change, $entity->original->status is still pointing at the first one. Ideally workflows like this would be handled in events / hooks outside of the save pipeline, but Drupal is not currently discouraging this practice.
use Drupal\node\Entity\Node;
$node = Node::create(['type' => 'page']);
$node->set('title', 'Drupal 9 rocks!');
$node->save();
$node->set('title', 'Drupal 10 rocks!');
$node->save();
/**
* Implements hook_node_update().
*/
function mymodule_node_update(Node $node): void {
// The node has been created with the title 'Drupal 9 rocks!'.
// Then we update it to say "Drupal 10 rocks!".
// This hook catches that update, and changes it to "Drupal 11 rocks!".
// This is a simple example that could be replaced with a presave() hook,
// but in the case of workflow changes, every status needs to be cycled
// through, no skipping allowed.
if ($node->title == 'Drupal 10 rocks!') {
$node->set('title', 'Drupal 11 rocks!');
$node->save();
return;
}
// On second update, check the value of $node->original.
if ($node->title == 'Drupal 11 rocks!') {
if ($node->original->title == 'Drupal 10 rocks!') {
dump('The node has the correct original value');
}
if ($node->original->title == 'Drupal 9 rocks!') {
dump('The node has an incorrect original value');
}
}
}
Stop doing the isset check (it's premature optimization anyway), and then the concrete bug is fixed.
// Load the original entity, if any.
if ($id_exists) {
$entity->original = $this->loadUnchanged($id);
}
None.
None.
None.
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #1
bojanz commentedHere's the initial patch. We'll probably want some tests as well, and a D7 backport when this is done.
Added the Commerce and Entity API (D7 contrib) issues as related.
Comment #2
bojanz commentedComment #3
bojanz commentedComment #4
fagoI don't think we can remove this "premature" optimization, as it's need to allow mass updates to function efficiently; i.e. it allows you to (at least) multiple load the entities (while there is no multiple update).
The module issuing that change could already take care of updating $entity->original, what would be the most efficient anyway.
A generic, proper solution would be doing #1480696: Move $entity->original to a separate hook argument, i.e. instead of pre-populating $entity->original it could become an optional argument to save(). That would be an API change, but imo this would still justify doing it right *now*.
See #1729812: Separate storage operations from reactions.
Comment #5
bojanz commentedHow does that work? I don't see any other place (such as the load method) populating original, it seems to be loaded one by one?
Comment #6
fagocheck e.g. _node_mass_update_helper()
Comment #7
amateescu commented_menu_navigation_links_rebuild() also uses this (line 2763 in menu.inc) to not execute hundreds of queries on a menu rebuild, so it's not premature optimisation at all.
Comment #16
avpadernoComment #23
smustgrave commentedIs this still relevant? I can't seem to find the code in the description. And the other example seems to be centered around how D7 did it.
If still an issue can we get an updated issue summary please
Comment #24
avpadernoDrupal code has been changed, since this issue has been open. Now, the code that initializes
$entity->originalis contained inEntityStorageBase::doPreSave(), the method called beforehook_entity_update()implementations are invoked, which contains the following code.I take the issue is suggesting that
$this->loadUnchanged($id)should always be called for entities that are updated. The quoted code used byEntityStorageBase::doPreSave()I showed should therefore be changed to the following one.The issue summary should be updated: What the suggested change is should be made clearer and the Drupal 7 example code should be replaced by Drupal 9 code, or removed.
Comment #25
avpadernoComment #28
dieterholvoet commentedI updated the issue description and the code example based on the info provided by @apaderno and my own experience. I'll create a MR with the suggested fix since it actually fixed the issue for me.
Comment #29
dieterholvoet commentedComment #31
dieterholvoet commentedComment #32
smustgrave commentedFailure seems like it could be legit for this issue.
Also would need a test of it's own.
Good job on the issue summary!
Comment #38
vidorado commentedI've added tests, but I can't get them to fail even after undoing the fix... 🤔
It seems the original entity is already fine in D11.
Could you take a look, @dieterholvoet?
Edit: Now that the complete test suite has run in GitLab, I see the fix is breaking some tests. A deeper investigation of the side effects over entity revisions is needed.
Comment #39
dieterholvoet commentedI can confirm the issue is still present in Drupal 11.3.0 and merge request !10763 fixes it.