Problem/Motivation

Webform elements and handlers "preSave" method are called twice in WebformEncryptSubmissionStorage::doPreSave().
This is because they are first called on parent::doPresave() and then at the end of the method too.

Proposed resolution

The best solution could be to let the parent class running the preSave, so handlers in pre-save can benefit from having unencrypted values.
We then encrypt after the parent::doPreSave has ran, so anything accessing the value after that will see encrypted data.

Remaining tasks

Validating if there is a reason for double running, otherwise fix the issue. Adding test coverage would be ideal.

User interface changes

Nope.

API changes

No, if double running is unwanted. Otherwise this should be documented.

Data model changes

No.

CommentFileSizeAuthor
#7 2943469-7.patch6.05 KBgambry
#7 interdiff-3-7.txt1.52 KBgambry
#3 2943469-3.patch6.03 KBgambry
#3 2943469-3-test-only.patch5.48 KBgambry
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gambry created an issue. See original summary.

gambry’s picture

gambry
Primary language Italian
Location Milan
CreditAttribution: gambry at manifesto commented

This is the incriminated bit from WebformEncryptSubmissionStorage::doPreSave():

protected function doPreSave(EntityInterface $entity) {
    /** @var \Drupal\webform\WebformSubmissionInterface $entity */
    $id = parent::doPreSave($entity);

    $data_original = $entity->getData();

    $webform = $entity->getWebform();

    $encrypted_data = $this->encryptElements($data_original, $webform);
    $entity->setData($encrypted_data);

    $this->invokeWebformElements('preSave', $entity);
    $this->invokeWebformHandlers('preSave', $entity);
    return $id;
  }
gambry’s picture

gambry
Primary language Italian
Location Milan
CreditAttribution: gambry at manifesto commented
Status: Active » Needs review
FileSize
2943469-3-test-only.patch5.48 KB
2943469-3.patch6.03 KB

Attached test-only - to demonstrate the issue - and full patch.

The last submitted patch, 3: 2943469-3-test-only.patch, failed testing. View results

Manuel Garcia’s picture

Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commented

Thanks @gambry I have done some thinking/digging, and I don't see why would we want to run these twice.

+++ b/tests/modules/webform_encrypt_test/src/Plugin/WebformHandler/TestWebformHandler.php
@@ -0,0 +1,68 @@
+    $counter = $this->state->get('test_webform_handler_counter', 0);
+    $this->state->set('test_webform_handler_counter', ++$counter);

Would it make sense to name this something more specific, say test_webform_handler_presave_counter? We we may want to track other things in the future.

gambry’s picture

gambry
Primary language Italian
Location Milan
CreditAttribution: gambry at manifesto commented
Status: Needs review » Needs work
Issue tags: +OpenSourceSprint

It totally does make sense. I will do the change ASAP.

Tagging OpenSourceSprint as I've done the work during the event.

gambry’s picture

gambry
Primary language Italian
Location Milan
CreditAttribution: gambry at manifesto commented
Status: Needs work » Needs review
FileSize
interdiff-3-7.txt1.52 KB
2943469-7.patch6.05 KB

Patch addressing #5 attached.

  • Manuel Garcia committed 0f0302f on 8.x-1.x authored by gambry 
    Issue #2943469 by gambry: "preSave" elements and handlers called twice
Manuel Garcia’s picture

Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commented
Status: Needs review » Fixed

Committed, thanks!

Status: Fixed » Closed (fixed)

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