Hi, after a long debug I've discovered that the latest Estimated Read Time version has a huge impact on Webform module. I've reproduced the bug on a clean D10 installation with default Webform and Estimated Read Time settings.
Just create a simple webform with an email handler and try to make a submission: the email will be sent twice if Estimated Read Time is enabled.
The bug is on estimated_read_time_entity_insert():
/**
* Implements hook_entity_insert().
*/
function estimated_read_time_entity_insert(EntityInterface $entity) {
// Set the read time for new entities.
if (!$entity instanceof FieldableEntityInterface) {
return;
}
estimated_read_time_set_estimated_read_time($entity);
$entity->save();
}
More precisely the $entity->save triggers the webform submission (which also is an entity). This should not be done because it can generate errors like this.
As reference please look at the hook_entity_insert documentation:
https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Entity!entity.api...
Let me know what you think about.
Thank you.
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | 3361590-13.patch | 2.35 KB | mtalt |
| #13 | interdiff_11-13.patch | 2.1 KB | mtalt |
| #11 | fix_improper_saving-3361590-v4.patch | 1.12 KB | finex |
Comments
Comment #2
finex commentedComment #3
finex commentedComment #4
shailja179 commentedComment #5
finex commentedHi, I've found the problem and created a patch. Let me know if it is ok to merge.
Comment #6
finex commentedComment #7
shailja179 commentedComment #8
shailja179 commentedComment #9
finex commentedHi @shailja179, why did you hide my patch? Anyway, this is a more efficient patch.
Comment #10
finex commentedFinal version: this should be ok.
Comment #11
finex commentedUploaded the wrong file. Sry.
Comment #12
shailja179 commented@FiNeX,sorry it may be by mistake.I was reviewing your patch, may be accidentally. Sorry for that.
Comment #13
mtalt commented@FiNeX - Thank you for the issue and patch.
The switch to using a combination of hook_entity_presave() and hook_entity_insert() instead of just hook_entity_presave was done in an attempt to solve for the link field issue you had reported in issue 3322934. The patch in #11 would revert that fix and cause the error when creating new nodes that display the links field.
But it does look like it was a mistake to attempt to workaround the issue by using hook_entity_insert() because you run into issues like yours with Webform when the entity is saved again. In order to attempt to solve for both the original links field issue and the improper use of hook_entity_insert(), I have updated the patch to include code to set the in_preview property for nodes to TRUE, which will prevent the links field links from being built.
Please review!
Comment #14
gena.io commentedHi everyone!
I would like to propose another logic in the following patch. It keeps the original logix with the entity saving but in another, appropriate place.
Check it out
Comment #15
weseze commented@mtalt: tried your patch and works for us. Don't known about the link field issue tough, since we are not using that kind of setup.
@gena.io (and also to the maintainer of this module): calling save() function on an entity in any of the insert/update/presave/postsave/... hooks should never be done. These hooks are called when drupal is already saving the entity, so you basically save it twice. That makes no sense and can lead to al sorts of issues with core/contrib (now and later on) Please don't go down that route...
Comment #16
mtalt commented@weseze Thank you for testing. Agreed that the code should never have been saving the entity in the presave or insert hooks. I'll get this into the dev branch.
Comment #18
mtalt commentedThis has been committed to dev.