Editing a view leaves old keys hanging, results in invalid schema

Let's see how the first bit can be solved.

Here is one for the #1

Making testing the ajax form is hard! I cannot manage to figure it out at the moment. Let' see later.

Less big patch.

The D8 committers discussed this and we agreed that this may even be critical because of the data integrity issues.

working on this at DrupalCamp NJ

To recreate:
Activate the "Archive" view.
Change the "Format" of the "Archive" view to "HTML List" and check the "Force using fields" checkbox.
Add a Field to the View (one that supports multiple formatters, "Content: Tags" will work.
Export the view config (Admin->Configuration->Configuration Syncronization->Export->Single Item->
Configuration type=View->Configuration name=Archive)
Now go back to the view settings and change the formatter, re-export.

With the last posted patch applied, the field->field_tags->type is different. Before it was not.

Going to keep working on the recreation steps in the issue summary for the next couple hours.

Thanks, because I was following this issue but don't know how to reproduce it.

I just followed those steps. I don't have the patch applied. But, I do see that the formatter changed.

*Edit: Previously changed it to "author" and the export showed the change. Changed it a 2nd time to "rendered enitity" and the export did not show the change. Then it worked fine 4 times in a row.

After more testing, I doubt that field type didn't change. That would be even more of a problem. Probably sidharrell and I did not save the view?

However, after changing from
type: entity_reference_entity_view
to
type: entity_reference_label
the

          settings:
            view_mode: full

is probably supposed to go away. It should actually revert to

          settings:
            link: true

updated issue summary with recreation steps. Still getting varied test results. Sometimes I get no change to the config output, Occasionally it will work.

Taking off for now. DrupalCamp is tomorrow, but I'll pick it back up on Sun.

Going to look into changing as much of the recreation steps into drush commands as possible.
Found this:
drush views-enable archive
Working on this now at DrupalCampNJ Sprint.

The changes to the test from #5, slightly modified.

testme, please.
Forgot to change status

interdiff of the changes in the test from #5 to #14
Without the changes, locally testing shows "Undefined index: ajax" on FieldUITest.php line 98.
Why the change works, I have no clue.

did not review the whole thing.

1. +++ b/core/modules/views/src/Plugin/views/HandlerBase.php
   @@ -787,8 +787,15 @@ public function displayExposedForm($form, FormStateInterface $form_state) {
   +   * @param $form
   

   missing type.

2. +++ b/core/modules/views/src/Plugin/views/HandlerBase.php
   @@ -787,8 +787,15 @@ public function displayExposedForm($form, FormStateInterface $form_state) {
   +   * @param callable $options_update_callable
   +   *   A callable which is called to merge the options.
   

   has a default value, should say (optional) in the @param docs https://www.drupal.org/node/1354#param

   huh. as of 5.4 this is a type, but looks like https://www.drupal.org/node/1354#types needs updated to include it.

3. +++ b/core/modules/views/src/Plugin/views/HandlerBase.php
   @@ -787,8 +787,15 @@ public function displayExposedForm($form, FormStateInterface $form_state) {
   +  public function submitTemporaryForm($form, FormStateInterface $form_state, callable $options_update_callable = NULL) {
   

   ok. adding an optional additional parameter with a default is not a BC break.

4. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -496,6 +496,18 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +  public function submitTemporaryFormForFormatterType($form, FormStateInterface $form_state) {
   

   needs docs.

Made a change trying to unset the old option from the old formatter in the callable function, but it did not fix it. The old formatter option should be in the $options array. Not sure why unsetting it did not remove it.

.

Possible we need to get the view again after changing it. Attempting in patch.

Can this also work with nojs or we need the ajax post? I tried nojs test, but got a ConfigSchemaChecker error :(

@lokapujya We certainly have to fix both cases, this is for sure. There might be multiple bugs here, but at least the bug inside the ajax code can be just triggered from some ajax post, which is problematic, of course.

+++ b/core/modules/views/src/Plugin/views/HandlerBase.php
@@ -787,8 +787,15 @@ public function displayExposedForm($form, FormStateInterface $form_state) {
+  public function submitTemporaryForm($form, FormStateInterface $form_state, callable $options_update_callable = NULL) {

I debugged the test a bit. As far as I can tell, submitTemporaryForm() never runs via the test - neither patch or in HEAD. I see $form['type'] being built, but not the submit callback being run. Also left my debug in and tried a couple of other ViewsUI tests that deal with options forms/handlers and couldn't see it run from there either.

Also don't see it getting called when doing similar to the test in the views UI. (ignore that bit, I missed a step).

That was some fun debugging.

+++ b/core/modules/views/src/Plugin/views/field/Field.php
@@ -454,7 +454,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
-      '#submit' => array(array($this, 'submitTemporaryForm')),
+      '#submit' => array(array($this, 'submitTemporaryFormForFormatterType')),

I don't get how this is working.

Actually, I do get it, I think. That piece of code should revert back. It passes because we aren't testing the AJAX submittal, so we need to add tests for AJAX still.

Actually, I do get it, I think. That piece of code should revert back. It passes because we aren't testing the AJAX submittal, so we need to add tests for AJAX still.

OH damnit you are absolutely right about that. This was a leftover from the previous approach. What do you think about the other part of the solution. Does it make sense for you?

Yes, the formatter settings now get cleared when the formatter changes.

1. Do we really need to add an AJAX test for the problem that existed only in a patch? I'm not sure if drupalPostAjaxForm() supports testing AJAX on a page generated by AJAX. Maybe there a way to replicate the AJAX call (maybe using drupalProcessAjaxResponse() Edit: drupalGetAjax()) that happens when the formatter is selected and assert that it didn't have an error?

2. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -498,6 +498,21 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +  public function submitFormCalculateOptions(array $options, array $form_state_options) {
   +    // When we change the formatter type we don't want to keep any of the
   +    // previous configured formatter settings, as there might be schema
   +    // conflict.
   

   Seems that this is code is more generic than for just formatters, it could be any configHandler? Does it affect (fix) any other configHandlers?

@lokapujya
Well it is still caused by this weird way how we dynamically construct the form array, so this still applies to formatter. I'm totally fine with not testing the ajax functionality.

Also think it's OK to not test the AJAX functionality here - that's something we should be converting to mink once it's up and running anyway.

Do we need an 8.1 patch?

Yep, looks like we do.

Maybe this 8.2 patch works on 8.1 also.

OK, rerolled with interdiff.

+++ b/core/modules/views/src/Plugin/views/field/Field.php
@@ -942,13 +958,16 @@ protected function addSelfTokens(&$tokens, $item) {
+    if(!isset($format)) {

Should be: if (!isset

Thanks.

@alexpott, @effulgentsia, @Cottser, and I discussed this issue today and still agree that it may be a critical. However, so far, the data integrity risk from the bug is theoretical (it might just be a major bug for the stale data). Ideally, we should manually test scenarios where this bug could cause data integrity problems, and document those steps and the results in the issue summary to make the impact clear. (Edit: not just steps to reproduce that there are invalid keys, but steps to reproduce that the invalid keys actually cause further critical problems.) So deferring triage on that and marking for issue summary update.

Thanks @lokapujya and @dagmar for continuing work on this issue!

We've got JavascriptTestBase in core now so we should be able to properly test the ajax path.

Adding JS test tag.

Here is a start, but I cannot figure it out yet. Debugging phantomjs is a pain to say the least.

@dawehner Behat\Mink\Exception\ElementNotFoundException: Form field with id|name|label|value "options[type]" not found.

Your selector 'div.views-display-column div.field div.views-ui-display-tab-setting a' returns 7 elements on a standard views edit page, not sure which one you want to click.

+++ b/core/modules/views/tests/src/FunctionalJavascript/Plugin/views/Handler/FieldTest.php
@@ -0,0 +1,71 @@
+namespace Drupal\Tests\views\FunctionalJavasript;

And as food for discussion, shouldn't functional javascript tests not pretty much always be in the views_ui namespace and not the views namespace? (oh and not contain typos ;-) FunctionalJavasript => FunctionalJavascript). Or do you have some thoughts about what should go where? Just to prevent them being split up willy-nilly over both views and views_ui.

@dawehner Behat\Mink\Exception\ElementNotFoundException: Form field with id|name|label|value "options[type]" not found.

Thank you for pointing out where I gave up :P

And as food for discussion, shouldn't functional javascript tests not pretty much always be in the views_ui namespace and not the views namespace? (oh and not contain typos ;-) FunctionalJavasript => FunctionalJavascript). Or do you have some thoughts about what should go where? Just to prevent them being split up willy-nilly over both views and views_ui.

Well, for me this is testing code which lives inside the views module ...

@dawehner this passes for me locally now. Like I said on IRC, not sure where this goes from here, so do with it as you see fit. I just wanted some more javascript test exercise.

Added assertAjaxRequest method. I know this is critical issue if we don't like it then ignore this patch.

Thank you @larowlan and @jibran!

So yeah the next step is now to reproduce the failure of the issue in the test.

+++ b/core/modules/views/tests/src/FunctionalJavascript/Plugin/views/Handler/FieldTest.php
@@ -56,20 +56,21 @@ protected function setUp() {
-    $this->getSession()->wait(5000, "jQuery('.ui-dialog-titlebar').length > 0");
+    $web_assert->assertAjaxRequest();
 

Mh, does this method name really tell you that? assertWaitOnAjaxRequest would make it clearer

Yeah sure.

Added the failing steps to the javascript test. Lets see what this does.

+++ b/core/modules/views/tests/src/FunctionalJavascript/Plugin/views/Handler/FieldTest.php
@@ -0,0 +1,107 @@
+
+namespace Drupal\Tests\views\FunctionalJavascript\Plugin\views\Handler;
+

The problem was the wrong namespace here.

Oops, sorry, left taking a screenshot in there by mistake. Removed now.

It's really nice to see a fix here.

Do we need to do anything to fix existing views - and can we?

+++ b/core/tests/Drupal/Tests/WebAssert.php
@@ -247,4 +247,23 @@ public function assert($condition, $message) {
+  /**
+   * Waits for AJAX request to be completed.
+   *
+   * @param int $timeout
+   *   (Optional) Timeout in milliseconds, defaults to 10000.
+   * @param string $message
+   *   (optional) A message for exception.
+   *
+   * @throws \RuntimeException
+   *   When the request is not completed. If left blank, a default message will
+   *   be displayed.
+   */
+  public function assertWaitOnAjaxRequest($timeout = 10000, $message = 'Unable to complete AJAX request.') {
+    $result = $this->session->wait($timeout, '(typeof(jQuery)=="undefined" || (0 === jQuery.active && 0 === jQuery(\':animated\').length))');
+    if (!$result) {
+      throw new \RuntimeException($message);
+    }
+  }

I think this should be on JavascriptTestBase and not WebAssert - WebAssert is for all BrowserTestBase tests. Also once it is part of JavascriptTestBase it should use JavascriptTestBase::assertJsCondition().

Do we need to do anything to fix existing views - and can we?

That's tricky. As far as I understand we still don't require schema, given that there might be formatter plugins which don't comply to schema yet, so fixing those configs would break things.

@dawehner but that can be determined - right?

Discussed with @dawehner - I think we can fix views where we have schema for the plugins. However given that this is just old data lying around - and not breaking anything (apart from @bojanz's tests) I think we should file a major followup to implement an upgrade path to fix existing views (where possible).

Follow-up for the upgrade path sounds good, more important to stop any new data integrity issues being introduced.

#2753541: Cleanup views with invalid data is the follow up.

Still needs work for second part of #63

I think this should be on JavascriptTestBase and not WebAssert - WebAssert is for all BrowserTestBase tests. Also once it is part of JavascriptTestBase it should use JavascriptTestBase::assertJsCondition().

I disagree, imo assertJsCondition is in the wrong class. It should live in WebAssert/JsWebAssert because it is totally related to webpage. If a driver doesn't support certain operation then an exception will be thrown so we are safe there with the misuses. At least Mink handles it like that. And we should keep our base classes as skinny as possible.

Right, well in that case we should have a JSWebAssert ...

Ok, reroll with JSWebAssert.

That looks great! In a follow up we could move the assertJsCondition over as well.

RTBC++ and +1 to #73. Un-related fails so retesting.

Committed 793b7f1 and pushed to 8.1.x and 8.2.x. Thanks!