Problem/Motivation
Editing a View's Field's Formatter and exporting the view's configuration does not show the changes to the Formatter.
Steps to Reproduce
From fresh install / db Go to http://localhost/admin/structure/views
Create a new View called "test". Edit the new View and add a Body Field. Change the Formatter from Default to "Summary or trimmed". Change the Trimmed limit to 100. Apply, then Save.
Now Edit the Field, change the Formatter back to Default. Apply, then Save.
Use the Config Inspector contrib module. (drush dl config_inspector; drush en config_inspector)
Go to http://localhost/admin/config/development/configuration/inspect
Scroll down to views.view.test. Boom, shows error.
Proposed resolution
Ensure that we don't do a simple merging of the settings, but rather clean out the old configuration first.
Remaining tasks
Commit
User interface changes
None
API changes
None
Data model changes
Should not be needed
Original report by bojanz
dawehner wanted an imperfect bug report :P
Two ways I managed to reproduce this:
1) Changed the formatter of a field. The settings of the previous formatter stayed in the export. Boom, invalid schema.
2) Cloning a view, then editing the menu settings resulted in some old keys persisting.
#1 is worth chasing at least. No clue what's causing it at this point.
Comment | File | Size | Author |
---|---|---|---|
#72 | editing_a_view_leaves-2648956-72.patch | 15.75 KB | Lendude |
#72 | interdiff-2648956-61-72.txt | 2.84 KB | Lendude |
#61 | editing_a_view_leaves-2648956-61.patch | 15 KB | Lendude |
Comments
Comment #2
dawehnerLet's see how the first bit can be solved.
Comment #3
dawehnerHere is one for the #1
Comment #4
dawehnerMaking testing the ajax form is hard! I cannot manage to figure it out at the moment. Let' see later.
Comment #5
dawehnerLess big patch.
Comment #7
xjmThe D8 committers discussed this and we agreed that this may even be critical because of the data integrity issues.
Comment #8
sidharrell CreditAttribution: sidharrell commentedworking on this at DrupalCamp NJ
Comment #9
sidharrell CreditAttribution: sidharrell commentedTo 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.
Comment #10
lokapujyaThanks, 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.
Comment #11
lokapujyaAfter 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
is probably supposed to go away. It should actually revert to
Comment #12
sidharrell CreditAttribution: sidharrell commentedupdated 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.
Comment #13
sidharrell CreditAttribution: sidharrell commentedGoing 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.
Comment #14
sidharrell CreditAttribution: sidharrell commentedComment #15
sidharrell CreditAttribution: sidharrell commentedThe changes to the test from #5, slightly modified.
Comment #16
sidharrell CreditAttribution: sidharrell commentedtestme, please.
Forgot to change status
Comment #17
sidharrell CreditAttribution: sidharrell commentedinterdiff 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.
Comment #19
YesCT CreditAttribution: YesCT commenteddid not review the whole thing.
missing type.
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.
ok. adding an optional additional parameter with a default is not a BC break.
needs docs.
Comment #20
sidharrell CreditAttribution: sidharrell commentedMade 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.
Comment #21
dawehner.
Comment #23
lokapujyaPossible 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 :(
Comment #25
dawehner@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.
Comment #26
catchI 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).Comment #27
dawehnerThat was some fun debugging.
Comment #29
lokapujyaI don't get how this is working.
Comment #30
lokapujyaActually, 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.
Comment #31
lokapujyaComment #32
dawehnerOH 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?
Comment #33
lokapujyaYes, the formatter settings now get cleared when the formatter changes.
drupalProcessAjaxResponse()Edit: drupalGetAjax()) that happens when the formatter is selected and assert that it didn't have an error?Seems that this is code is more generic than for just formatters, it could be any configHandler? Does it affect (fix) any other configHandlers?
Comment #34
dawehner@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.
Comment #35
catchAlso 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.
Comment #36
lokapujyaComment #38
catchComment #40
lokapujyaDo we need an 8.1 patch?
Comment #41
catchYep, looks like we do.
Comment #42
lokapujyaMaybe this 8.2 patch works on 8.1 also.
Comment #44
lokapujyaOK, rerolled with interdiff.
Comment #45
lokapujyaComment #46
dagmarShould be: if (!isset
Comment #47
lokapujyaThanks.
Comment #48
xjm@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!
Comment #49
alexpottWe've got JavascriptTestBase in core now so we should be able to properly test the ajax path.
Comment #50
jibranAdding JS test tag.
Comment #51
dawehnerHere is a start, but I cannot figure it out yet. Debugging phantomjs is a pain to say the least.
Comment #52
Lendude@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.
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.
Comment #53
dawehnerThank you for pointing out where I gave up :P
Well, for me this is testing code which lives inside the views module ...
Comment #54
Lendude@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.
Comment #55
jibranAdded
assertAjaxRequest
method. I know this is critical issue if we don't like it then ignore this patch.Comment #57
dawehnerThank you @larowlan and @jibran!
So yeah the next step is now to reproduce the failure of the issue in the test.
Mh, does this method name really tell you that?
assertWaitOnAjaxRequest
would make it clearerComment #58
jibranYeah sure.
Comment #59
LendudeAdded the failing steps to the javascript test. Lets see what this does.
Comment #60
dawehnerThe problem was the wrong namespace here.
Comment #61
LendudeOops, sorry, left taking a screenshot in there by mistake. Removed now.
Comment #63
alexpottIt's really nice to see a fix here.
Do we need to do anything to fix existing views - and can we?
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().
Comment #64
dawehnerThat'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.
Comment #65
alexpott@dawehner but that can be determined - right?
Comment #66
alexpottDiscussed 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).
Comment #67
catchFollow-up for the upgrade path sounds good, more important to stop any new data integrity issues being introduced.
Comment #68
dawehner#2753541: Cleanup views with invalid data is the follow up.
Comment #69
alexpottStill needs work for second part of #63
Comment #70
jibranI 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.Comment #71
dawehnerRight, well in that case we should have a JSWebAssert ...
Comment #72
LendudeOk, reroll with JSWebAssert.
Comment #73
dawehnerThat looks great! In a follow up we could move the
assertJsCondition
over as well.Comment #75
jibranRTBC++ and +1 to #73. Un-related fails so retesting.
Comment #76
alexpottCommitted 793b7f1 and pushed to 8.1.x and 8.2.x. Thanks!