Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
#2263569: Bypass form caching by default for forms using #ajax. is moving forms away from relying on /system/ajax, since that requires that forms always be cached.
views_ui_add_ajax_trigger() currently relies on the form being cached.
Proposed resolution
Remove the system.ajax #ajax URL from views_ui_add_ajax_trigger()
Remaining tasks
N/A
User interface changes
N/A
API changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#26 | 2500523-views_ui-26-do-not-test.patch | 8.95 KB | tim.plunkett |
#26 | 2500523-views_ui-26.patch | 10.43 KB | tim.plunkett |
#26 | interdiff.txt | 8.83 KB | tim.plunkett |
#23 | 2500523-views-23.patch | 2.16 KB | effulgentsia |
#20 | 2500523-views-18.patch | 11.07 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettThis will fail. It includes #2263569: Bypass form caching by default for forms using #ajax. (which is passing), but removes the workaround for the Views UI.
The steps to reproduce:
Go to admin/structure/views/add
Note that "sorted by" is "Newest first"
Switch the "Show" select list from "Content" to "Comments"
Expected:
"sorted by" will become "Unsorted", with no error
Actual:
Form validation error for "sorted by".
There are several select lists that are updated when switching the base table of the view.
In HEAD, the original form corresponding to 'node' (the default), is cached. On #ajax, the node-based form is loaded, and the callback operates on that to switch everything over to the new base table (comment).
With the patch, it instead rebuilds the whole form but with the input of 'comments' for the base table, and the old node-based sort key for the sort.
This is taken into consideration when building the form, so before the callback has even run, the form is built with a base table of 'comment' and the node-based sort key is considered invalid.
The do-not-test patch is just the change needed after the other issue lands.
Comment #3
dawehnerI guess we need to replace the entire form with the ajax callback
OR
start with no default and force people to always select an entity type first?
Comment #4
tim.plunkettEven if we start with no default, that would only solve the initial switch, it wouldn't help if you switched the base table after picking some selects.
Comment #5
effulgentsia CreditAttribution: effulgentsia at Acquia commentedRaising to critical per #2502785-10: Remove support for $form_state->setCached() for GET requests.
Comment #6
dawehnerOne possible approach would be to prefix the entire form, or rather put it into a wizard plugin specific form structure, like
'wizard__' . 'node'
The additional step then would require to rewrite a bunch of other stuff ... or actually quite a lot.
This interdiff just shows what this could like ...
Comment #7
tim.plunkettThis was discussed on the "D8 critical issues discussion" call, I was not present, but @catch basically confirmed what I wanted to do, which is throw away all of the irrelevant POST data when changing a #ajax-ified select.
Discussion starts here https://www.youtube.com/watch?v=7HcVntLrryU&feature=youtu.be&t=2560
This attempts to remove the offending user input, but doesn't work for "Include an RSS feed" checkbox within the "Create a Page" section.
Comment #8
jibran+1 for the idea.
Comment #10
tim.plunkettLogic wasn't complete yet. Still doesn't work for the feed checkbox.
Added a unit test for \Drupal\views\Plugin\views\wizard\WizardPluginBase::getSelected() which helped me refactor it.
Comment #12
dawehnerSo yeah what we kind of discussed has been to change the UI into a two step process, similar to the field UI.
Comment #13
jibranI think we should explore this option first before moving to two step from. If we'll be able to fix it like this then we don't have to deal with UX issues. Thoughts??
Comment #15
dawehnerI totally agree ...
I don't know much about fapi, but this particular change feels really dangerous
+1 for moving the additional logic into this method. This indeed is the natural place for it
Comment #16
tim.plunkettThis should handle the feed row style bug. It needs to be determined dynamically, same as the others.
Not sure if that's the last cause of fails or not...
Comment #18
dawehnerNice!
Tim do you mind explaining what is going on in the first hunk, see comment #15
Comment #19
effulgentsia CreditAttribution: effulgentsia at Acquia commentedRe #18, that first hunk is trying to make it so that the form builder function (e.g., ViewAddForm::form() via its call to WizardPluginBase::getSelected()) can modify $form_state['input']. Which is potentially problematic if the form is cached, because in such a situation, the form builder wouldn't run. Even if we remove the case of caching on the initial GET request, we should consider what would happen if some other module made the form cacheable after a validation error (e.g., Mollom does so that if you answered the captcha correctly, but got a validation error for some other field, you don't need to re-answer the captcha again). In such a case, when you submit after correcting your validation problems, the form is retrieved from cache, so the form builder function is skipped, so its changes to $form_state['input'] don't happen, which means $form_state['input'] is different as an unintended side-effect of enabling Mollom (or any other module that turns on form caching after a validation error).
Comment #20
tim.plunkettMore work, less fails. Something is still wrong though.
Comment #21
tim.plunkettBecause these form elements are not executing the form, the form will be rebuilt after each one.
All rebuilt forms are cached.
Therefore, ... um I don't know any more. I need some time away from this code, I'll be back tomorrow.
Comment #23
effulgentsia CreditAttribution: effulgentsia at Acquia commentedDoes this work?
Comment #24
effulgentsia CreditAttribution: effulgentsia at Acquia commentedOn the first AJAX submission, when the form is uncached, this lets the form get built the first time the same way it did for the GET request.
Without this, the first AJAX submission results in views_ui_ajax_update_form() returning $form['displays'] but without that element having a #prefix with the wrapper ID. I think because that first submission builds the form twice, so the 2nd build looks to be a wrapper duplication but isn't. Then, AJAX submissions 2-N end up doing a replaceWith() with the correct content returned from the server but don't have a wrapper element in the DOM to do it on, so the front-end doesn't get updated to match what's on the back-end.
Comment #26
tim.plunkettYep, that's it. The getUserInput() change corresponds directly to my debugging, except I was checking the wrong flag and trying to fake getting back to an empty array. Doing it this way is exactly what I wanted to do, but couldn't figure out.
The duplicate_wrapper change also resolves the last 10% of what I was trying to do. Changing code in views_ui_add_ajax_wrapper() now that duplicate_wrapper is no more.
Copying my test changes, which are still necessary due to the double build.
The do-no-test is just git diff -w
Hopefully @dawehner can review, because I think we're done here.
Comment #27
dawehnerI'm a bit confused that we no longer need a reference, but I see, we just get values, so there is no need for a reference.
Yeah I don't see an issue with doing it.
Does that documentation add any value on top of version control, I don't really think so
Awesome!
Comment #28
tim.plunkett27.3, that's just outdented docs, notice the do-not-test patch using -w doesn't have that line
Comment #29
Fabianx CreditAttribution: Fabianx as a volunteer commentedRTBC + 1, Great work! - Fantastic simple solution, can we document that approach somewhere, please?
Comment #30
alexpottCommitted 40004e3 and pushed to 8.0.x. Thanks!
@Fabianx I talk about documenting the approach in IRC with @tim.plunkett. He said
I'm inclined to agree.