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.

Comments

FiNeX created an issue. See original summary.

finex’s picture

Issue summary: View changes
finex’s picture

Issue summary: View changes
shailja179’s picture

Assigned: Unassigned » shailja179
Status: Active » Needs work
finex’s picture

Assigned: shailja179 » finex
Status: Needs work » Active
StatusFileSize
new791 bytes

Hi, I've found the problem and created a patch. Let me know if it is ok to merge.

finex’s picture

Status: Active » Needs review
shailja179’s picture

Assigned: finex » Unassigned
Status: Needs review » Needs work
shailja179’s picture

Status: Needs work » Needs review
finex’s picture

StatusFileSize
new1.01 KB

Hi @shailja179, why did you hide my patch? Anyway, this is a more efficient patch.

finex’s picture

StatusFileSize
new706 bytes

Final version: this should be ok.

finex’s picture

StatusFileSize
new1.12 KB

Uploaded the wrong file. Sry.

shailja179’s picture

@FiNeX,sorry it may be by mistake.I was reviewing your patch, may be accidentally. Sorry for that.

mtalt’s picture

StatusFileSize
new2.1 KB
new2.35 KB

@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!

gena.io’s picture

Hi 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

weseze’s picture

@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...

mtalt’s picture

Assigned: Unassigned » mtalt
Status: Needs review » Reviewed & tested by the community

@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.

  • mtalt committed be1d15b9 on 1.0.x authored by FiNeX
    Issue #3361590 by FiNeX, mtalt, gena.io, shailja179, weseze: Estimated...
mtalt’s picture

Status: Reviewed & tested by the community » Fixed

This has been committed to dev.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.