Encryption settings are only saved per field

Here it is, works with the current data structure.

@vijaycs85 yeah, but that would require an upgrade path, so I went with the path of least resistance...

Yeah @fenstrat you are correct in your assessment of the way the module currently saves the data.

Ideally this configuration should be saved with the webform itself, so inside webform.webform.WEBFORMID.elements.FIELDNAME

I believe this may be is a sign of a bigger problem, perhaps this module should be making use of the hooks provided by webform module, instead of using hook_form_FORM_ID_alter etc.

#2855702: Allow encryption for more elements was committed a few days ago.

I've spent a bit of time on this today, here are my findindings:

1. None of the hooks provided by webform module allow us to hook into webform_ui_element_form. So it looks like we're stuck with our current approach.
2. On webform_ui_element_form, which is where WebformElementEncrypt::processWebformElementEncrypt() currently adds our settings form fields, has no information regarding to which webform this field is attached to. So unless someone figures this out, I'm not sure how we're going to do #5.

OK, nothing like stating that you're blocked to unblock yourself apparently...

Anyway, here is a patch addressing:

1. The schema name as mentioned on #3.
2. Saving the configuration per webform per field. In other words, addressing #5.
3. Updated the hook_uninstall to reflect these changes.

We should write a test for our uninstall hook, as it is pretty important for us not to break it accidentally, we should open up a follow up for this.

Re-titling and modifying the issue description to reflect the new scope.

Re #9.1:
Yes, so that field configurations are not shared across webforms. Eg. so you can have fields with the same name on different webforms, but on one webform its encrypted and on another webform it is not. (see #5).

Re #9.2:
Not necessary there, we actually want the the whole configuration, so that inside the loop for the submissions:

+++ b/webform_encrypt.install
@@ -15,11 +15,11 @@ function webform_encrypt_uninstall() {
-    if (isset($config[$submission->name]['encrypt']) && $config[$submission->name]['encrypt']) {
-      $encryption_profile = \Drupal\encrypt\Entity\EncryptionProfile::load($config[$submission->name]['encrypt_profile']);
+    if (isset($config[$submission->webform_id][$submission->name]['encrypt']) && $config[$submission->webform_id][$submission->name]['encrypt']) {
+      $encryption_profile = \Drupal\encrypt\Entity\EncryptionProfile::load($config[$submission->webform_id][$submission->name]['encrypt_profile']);

we can check the configuration for each specific submission by using the webform_id as well as the field name (so we can figure out if we need to decrypt or not).

Brilliant thanks @fenstrat!
Just committed #2923249: Fix test dir, retesting #11.

+++ b/src/Element/WebformElementEncrypt.php
@@ -79,16 +80,28 @@ class WebformElementEncrypt extends FormElement {
-    $config[$field_name] = array(
-      'encrypt' => $values['encrypt'],
-      'encrypt_profile' => $values['encrypt_profile'],
-    );
+    if (empty($values['encrypt'])) {
+      unset($config['settings'][$field_name]);
+    }
+    else {
+      $config['settings'][$field_name] = array(
+        'encrypt' => $values['encrypt'],
+        'encrypt_profile' => $values['encrypt_profile'],
+      );
+    }
 
-    \Drupal::service('config.factory')->getEditable('webform.encrypt')->set('element.settings', $config)->save();
+    if (empty($config['settings'])) {
+      $webform->unsetThirdPartySetting('webform_encrypt', 'element');
+    }
+    else {
...
+    }

This looks unnecessarily complex... is there a reason why we can't just $webform->setThirdPartySetting('webform_encrypt', 'element', $config); no matter what the values are?

We should proceed from #11

Again, thanks @fenstrat for working on this :)

Patch is looking good to me now, just adding a comment explaning why we need this logic in this patch.

We should provide an upgrade path, we have to assume is a lot more than 14 sites, since those are just the ones that are pinging drupal.org.
More over, even if it was "just" 14 sites, their webforms will stop encrypting data, they may not even notice and get into trouble legally or otherwise.

Here is the upgrade path, I've tested it a bit locally but we should test it further.

oops wrong interdiff file extension...

Some more work on this:

1. Clearing the cache on hook_udpate so that the old config is cleared from cache.
2. Turning our encrypt setting to be a boolean in the schema to match the expected value of the checkbox.

The last change I made for consistency, but also because with this patch I see that the checkbox is not shown as checked when you have saved it as enabled. As in if you enable encryption for a field, and save it, when you edit it again, the checkbox is not checked. Not sure what is going on there, but we need to fix that before we can commit this...

Alright I managed to figure this all out I think...

1. I have simplified the new schema, so its easier to get things, and more in line with how third party settings is meant to be used.
2. I have changed our tests so they validate our schema since that should work now.
3. Double checked our migration path, looks good to me but could use a second hand of testing (any takers?)
4. The logic on WebformElementEncrypt::validateWebformElementEncrypt() is now a lot simpler due to the new schema.

Let's see if this does the trick :)

Thanks @vijaycs85 for the review!

Re #38.1 - you're right, fixed and verified that the value is indeed typecasted on save.

Re #38.2 - Tests++

Re #38.4 - I know it looks weird, I struggled with it as well. The thing is that we need to delete the old config AND clear the cache at the end, so I did it like this in order not to have to do this on two places...

Re #38.5 - That's what I thought at first too, but even though we deleted it, the old config is stil accessible on the site (checked with config_inspector). That's not the case if we call drupal_flush_all_caches() here so perhaps not all the cache is cleared on hook_update?

argh again wrong file extension for the interdiff... sorry bot!

Using config_inspector, this is how the Raw configuration data looks with this patch, for a webform with two fields,:

'third_party_settings' => 
  array (
    'webform_encrypt' => 
    array (
      'field2' => 
      array (
        'encrypt' => true,
        'encrypt_profile' => 'pfizer_encrypt',
      ),
      'field1' => 
      array (
        'encrypt' => true,
        'encrypt_profile' => 'pfizer_encrypt',
      ),
    ),
  ),

And the Tree of configuration data:

Are you sure we want to add another level? I ask because right now its realy clean to get the config for an element like this: $webform->getThirdPartySetting('webform_encrypt', $element_name);

OK, I suppose it makes sense if we want to add some other webform-wide setting and if webform does it like this, we should follow their lead in any case, so here it is, hopefully I didn't forget anything.

Some highlights:

• Had to restore a bit of the logic on WebformElementEncrypt::validateWebformElementEncrypt().
• Was able to simplify a bit the update hook (only set the third party settings once per webform).

Just realized we were breaking the uninstall process, fixing it now. We really need test coverage for this.

Opened up #2924905: Data gets destroyed due to hook_uninstall.

Thanks @fenstrat

Bumping priority here.

Also, we should have a look at #2924957: Unable to encrypt nested/multiple nested fields before cutting the first alpha release.

#2924905: Data gets destroyed due to hook_uninstall is in now, let's see where we are with this patch now that we have a test for uninstalling.

Test is still green, back to RTBC

Just a heads up that the update hook introduced here is faulty. If your webforms have fields inside containers, the configuration will not be migrated properly, see #2935893: webform_encrypt_update_8101 ignores fields in containers