Problem/Motivation

We have a custom Webform Handler in which we used the postSave method. We implemented something incorrectly in this method containing a die() at some point. While this is quite dirty and incorrect, we still did this confidently because the postSave() method name suggests that the submission already has been saved to the database safely.

This has worked correctly for many years, however as off our latest module updates this suddenly broke. It turned out that during the execution of the postSave method, the submission was available, but the database transaction wasn't yet committed. So at the point of our die() statement, the submission was never actually being saved to the database, causing a lot of loss of data.

While this was due to dirty code on our side (which is now fixed), this may still occur if for example a fatal PHP error occurs during the execution of a webform handler.

Steps to reproduce

- Implement a webform handler in a webform
- put a die() statement in the postSave() method of the webform handler
- Submit the webform
- See if the submission has (not) been saved to the database

Proposed resolution

Make sure the database transaction has been committed to the database, before executing the postSave handlers.

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork webform-3344266

Command icon Show commands

Start 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:

About issue forks

Comments

Paul Dudink created an issue. See original summary.

jrockowitz’s picture

jrockowitz commented
Status: Active » Closed (won't fix)

I hesitate to change how a handler's postSave() method works. It will most likely cause issues for other people.

ricardopeters’s picture

ricardopeters commented
Status: Closed (won't fix) » Needs review

Would it be an idea, to document somewhere that the actual committing(saving) actually happens after the postSave, so any code that breaks the flow will abandon the save? Maybe it's an edge case, but at least having it down in writing might alert people.

jrockowitz’s picture

jrockowitz commented

I am always open to documentation improvements. Would you be able to create a PR with a suggestion?

RenatoG made their first commit to this issue’s fork.

renatog’s picture

renatog commented
Assigned: Unassigned » renatog

Makes sense. I can do it for us

RenatoG opened merge request !311

renatog’s picture

renatog commented
Version: 6.1.3 » 6.1.x-dev
Assigned: renatog » Unassigned

'

renatog’s picture

renatog commented

Implemented: https://git.drupalcode.org/project/webform/-/merge_requests/311

ricardopeters’s picture

ricardopeters commented

Nice! I just thought of this part:

breaks the PHP execution it may cause a fail in the final save.

I would change it up to this:

breaks the PHP execution it may prevent the sql transaction from committing.

Or do you guys think that's a tad to technical for the documentation? In my opinion it shed's a light on why it wouldn't be saved.

paul dudink’s picture

paul dudink commented

@jrockowitz

I hesitate to change how a handler's postSave() method works. It will most likely cause issues for other people.

Just checking, because your answer seems to contradict what actually happened:
Our code has not changed for years, but suddenly broke after updating Webform and/or Drupal core.

If you know for sure that this change in handling the postSave() is being caused by a change in Drupal core, than I think it's fine to simply document it.
However if not, I think it still should be investigated as a bug, because contrary to what you say this possible bug most likely cause issues for other people; fixing it will fix it :)

Could you elaborate on your vision about this? Don't you think that a postSave() should be an actual post-save, and not a before-commit-save?

renatog’s picture

renatog commented
Status: Needs review » Needs work

breaks the PHP execution it may prevent the sql transaction from committing.

Totally makes sense. Agreed

renatog’s picture

renatog commented
Status: Needs work » Needs review

MR updated with that suggestion at #10: https://git.drupalcode.org/project/webform/-/merge_requests/311

rviner’s picture

rviner commented

This causes a problem in the use case i've got.

I'm using the webform rest module to trigger a get submission on PostSave but it always get the previous data as it hasn't been committed to the database yet.

  • jrockowitz committed 49eacf68 on 6.2.x authored by renatog 
    Issue #3344266: Webform handler postSave() method is triggered before...
jrockowitz’s picture

jrockowitz commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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