Remove support for $form_state->setCached() for GET requests

Absolutely agreed on removing support for this. That we had a Drupal 7 SA, and 3 out of 22 critical issues are related to this shows just how much trouble it is.

I do think we also need the 'fast path' follow-up from #2263569: Bypass form caching by default for forms using #ajax. for AJAX requests opened too though - so that it's possible to post to a separate URL again for AJAX requests (with the ability to rebuild the form reliably).

Would it be fair to say that if we can implement this with #ajax['callback'] in Drupal 7, that we'd be able to implement with #ajax['callback'] in Drupal 8?

Panopoly's case is interesting, because what we're doing is auto submitting the form as the user is changing it (via the CTools auto submit functionality) and returning via AJAX the rendered version of the thing they're editing (a CTools content type plugin -- a "Panels block" for those who don't do lots of CTools/Panels) to provide live previews.

Using #ajax['url'] makes sense from Panopoly's perspective because we're not returning any part of the form, we're just taking it's values and rendering the CTools content type. However, if we rendered the preview in the form constructor into a '#type' => 'markup' element, then I think we could use #ajax['callback'] to just extract that element from the form.

But I'd need to actually try to do it to see if it's possible or not!

The extra interesting thing is that we're not including the preview markup inside the <form> tag because it caused some sort of issues in the past, but looking at the issue now (#2017159: Panopoly Magic: render preview outside of pane configuration form), I don't entirely understand what the problem was...

Anyway, if doing it in Drupal 7 would prove that it would work in Drupal 8, then I can do some experimentation!

The idea would be to try to accomplish everything with #ajax][callback, yes. You can get pretty creative with #ajax][wrapper too, targeting hidden divs or whatnot.

#3: Interesting, indeed you can get very creative using a special "ID" wrapper and returning ajax commands that replace the "#my-special-preview-id".

That should work regardless of the form's state or maybe using wrapper might work directly.

So, there are contrib needs to modify form_state during an ajax callback. I'm doing this in CTools 8.x-3.x right now by providing a different url route for wizard values during ajax. Looking at #2263569: Bypass form caching by default for forms using #ajax. it appears I'm going to need to switch to having a higher-priority event subscriber that tries to determine if this ajax request should be intercepted by it or let it fall through to core's. Any thoughts on that approach? Just making sure we give contrib proper consideration here.

Eclipse

#6: You can change $form_state all you want, you just cannot depend on any $form_state being set already on the first GET.

I'm just saying I want a generic way to alter the form state before it is passed to the ajax callback method specified in the #ajax array.

+1, the issue summary is overwhelmingly convincing.

For the record: We discussed this on the EU criticals call today and all participants have been in favor of removing it.

The main argument being:

If core can so easily run into it with _massive_ problems, then contrib will get the same problems and if D6/D7 history is any indication that will happen.

Therefore it is good to remove this before 8.0.0, so that no-one runs into that problem anymore. (and we are semantically correct not to write "state" changing data on a GET request)

@effulgentsia: I'm pretty sure Panopoly would be fine without #ajax['url'] (but I do still plan to do some experiments just in case) so no need to spend too much time responding. :-)

I created a Panopoly issue for my experiments: #2505369: Experiment with not using #ajax['url'] - it's going away in Drupal 8 (and shouldn't be necessary)

@catch, @effulgentsia, @webchick, @xjm and I discussed this issue and decided to keep it critical since we've made several errors through the d8 cycle because of this and not doing will make everything easier for contrib and custom. Also

Though named setCached(), what is actually persisted is state, and changing state during a GET request is something that the HTTP spec says SHOULD NOT be done.

is extremely compelling.

extremely compelling.

+1

This is a perfect example of where a decision made a long time ago for likely sensible reasons, but that didn't consider one small aspect (changing state during HTTP GET), causes enormous amounts of issues (in DX, scalability and cacheability).

#2500527: Rewrite \Drupal\file\Controller\FileWidgetAjaxController::upload() to not rely on form cache landed

Just to be clear, this is now only blocked on #2500523: Rewrite views_ui_add_ajax_trigger() to not rely on /system/ajax., right?

I think so yeah. If we have a patch here to ditch it, then it should fail until that goes in, then pass.

Since that issue is RTBC, I think it's worth having that patch here whether the other one is in or not.

Depends on how loud we want to be about it.
Let's see what happens.

Wow, actually found a bug in FormState.

FormState::isMethodType() and FormState::setMethod() both use strtoupper for comparisons.
But the default value of FormState::$method is 'post'.

This combines the two green patches, and also tries to mangle RenderElement enough that no one can use #ajax][url anymore.

I could not resist to try to debug some of the failures ... but there are still uses of #ajax][ in other places in the views UI.

This does not fixes all issues yet.

We still have to support to add custom query parameters.

We need to completely remove system.ajax here.

I was pretty confident with the changes, except those in drupalPostAjaxForm(). What do we fallback to here?

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -1826,7 +1827,8 @@ protected function drupalPostAjaxForm($path, $edit, $triggering_element, $ajax_p
+        // @todo Using '' is definitely wrong, what should it be?
+        $ajax_path = '';

What about ?

Questions we had during the talking:

• Is #ajax => []the expected behaviour, also UI wise?
• What happens in that case, we should point the wrapper to the form itself?

Mh, so in case you specify a callback, which renders the entire page the actual preview UI fails.

        '#submit' => array('::submitPreview'),
        '#id' => 'preview-submit',
        '#ajax' => array(
          'wrapper' => 'views-preview-wrapper',
          'event' => 'click',
          'progress' => array('type' => 'fullscreen'),
          'method' => 'replaceWith',
          'callback' => [get_called_class(), 'previewSubmit'],
        ),
      ),
    );

It posts to admin/structure/views/content for example. The form submission handling of the other form on the page is triggered
and you end up in throw new HttpException(500, 'The specified #ajax callback is empty or not callable.');
Interesting though is that its not the main edit form, but rather the exposed form, which causes issues, see debugging screenshot.

So the problem described in #38 is as said related with exposed forms in the preview.

Let's assume that problem does not exist, there is a different problem. The pager URLs contain ?ajax_form=1 which is then interpreted as form submits by the system.
We can workaround that problem using a fix in pager.inc

1. +++ b/core/lib/Drupal/Core/Form/FormState.php
   @@ -475,6 +475,9 @@ public function getButtons() {
   +      throw new \LogicException('NOPE');
   

   Needs better text.

2. +++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
   @@ -230,31 +222,30 @@ public static function preRenderAjaxForm($element) {
   +      if (isset($settings['url'])) {
   +        throw new \InvalidArgumentException('Custom ajax URLs are no longer supported');
   +      }
   +      $settings['url'] = NULL;
   

   The = NULL is overkill now with that exception.

3. +++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
   @@ -230,31 +222,30 @@ public static function preRenderAjaxForm($element) {
   +        throw new \InvalidArgumentException('Custom ajax URLs are no longer supported');
   

   AJAX

4. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1649,7 +1648,7 @@ protected function drupalPostForm($path, $edit, $submit, array $options = array(
   +          $action = $this->getAbsoluteUrl(!empty($submit['path']) ? $submit['path'] : '');
   
   @@ -1736,7 +1735,8 @@ protected function drupalPostForm($path, $edit, $submit, array $options = array(
   -   *   $ajax_path 'system/ajax' will be used.
   +   *   $ajax_path '' will be used.
   +   *   @todo Using '' is definitely wrong, what should it be?
   
   @@ -1792,6 +1792,7 @@ protected function drupalPostAjaxForm($path, $edit, $triggering_element, $ajax_p
   +      debug($ajax_settings);
   
   @@ -1807,7 +1808,8 @@ protected function drupalPostAjaxForm($path, $edit, $triggering_element, $ajax_p
   -    // Ajax settings, or else 'system/ajax'.
   +    // Ajax settings, or else ''.
   +    // @todo Using '' is definitely wrong, what should it be?
   
   @@ -1825,7 +1827,8 @@ protected function drupalPostAjaxForm($path, $edit, $triggering_element, $ajax_p
   -        $ajax_path = 'system/ajax';
   +        // @todo Using '' is definitely wrong, what should it be?
   +        $ajax_path = '';
   ...
        $ajax_path = $this->container->get('unrouted_url_assembler')->assemble('base://' . $ajax_path, $options);
   

   Not sure how we're getting around these empty strings now, but they will break if used with base://

5. +++ b/core/modules/system/src/Tests/Ajax/AjaxFormCacheTest.php
   @@ -44,8 +44,9 @@ public function testFormCacheUsage() {
   -    $this->assertEqual(3, count($key_value_expirable->getAll()));
   ...
   +    $this->assertEqual(0, count($key_value_expirable->getAll()));
   

   Yay!

6. +++ b/core/modules/system/tests/modules/ajax_forms_test/src/Form/AjaxFormsTestCachedForm.php
   @@ -34,9 +34,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      '#ajax' => [],
   
   +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -448,7 +448,6 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
          '#ajax' => array(
   -        'url' => views_ui_build_form_url($form_state),
          ),
   
   +++ b/core/modules/views_ui/src/Form/Ajax/ConfigHandler.php
   @@ -182,7 +182,6 @@ public function buildForm(array $form, FormStateInterface $form_state, Request $
            '#ajax' => array(
   -          'url' => Url::fromRoute('<current>'),
            ),
   
   +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -331,7 +327,6 @@ public function getStandardButtons(&$form, FormStateInterface $form_state, $form
            '#ajax' => array(
   -          'url' => $form_url,
            ),
   
   @@ -359,7 +354,6 @@ public function getStandardButtons(&$form, FormStateInterface $form_state, $form
          '#ajax' => array(
   -        'path' => $form_url,
          ),
   

   This seems odd having just #ajax => [] (I guess that's why you switched to !isset above)

7. +++ b/core/modules/views_ui/src/ViewPreviewForm.php
   @@ -89,16 +89,20 @@ protected function actions(array $form, FormStateInterface $form_state) {
   +  public static function previewSubmit($form) {
   +    return $form;
   +  }
   

   What a hack! Needs docs if we're going to keep it.

Did a little bit more of research.

I think something along those lines are needed in order to fix the exposed form in the views preview UI.

#40 is not adressed yet, as tim said he wants/promised/... to do it.

Yep, working on it now.

This breaks the Views UI visually for me, when triggering a pager during preview, the top part of the Views UI is duplicated.
The test coverage only uses the preview in isolation, not as a larger part of the form.

I addressed all of my own feedback from #40.

Yeah I can't reproduce this in a test, nor can I track down a fix. Here's a video.
Steps to reproduce:

Create 2 nodes
Go to admin/structure/views/view/content
Change the pager to show 1 item
Use the pager in the preview

Video:
https://www.drupal.org/files/issues/views_ui_fun.mp4

You know, you can never have enough views UI. It would be kinda a version history built into the interface, pretty neat.

Back to business, I had a look at why this is happening. The preview form itself,
is using the form rendering / ajax processing mechanism to render itself, so this means it just renders the form and then ajax.js replaces #views-preview-wrapper
with the rendered preview which contains both the form again, but also the renderer view.

If we have a look at the different between the output of this patch vs. HEAD you can see one major difference. The pager URLs are different.
In HEAD we point to admin/structure/views/content/preview, with #43 we though point to admin/structure/views/content itself. So what happens is that the pager
itself renders the entire page, not just the views preview itself.

If we have a look at \Drupal\views_ui\ViewUI::renderPreview we are doing quite some work in order to mock a request
and point to the entity.view.preview route. This would theoretically render the pager URLs in the context of admin/structure/views/content/preview,
too bad that it simply doesn't work. Let's fix that.

Not sure whether we can test this for real, but at least we should have a check that the links point to the right URL.

I also did a look at the interdiff in #43 and it looks alright for me

+++ b/core/modules/views_ui/src/Form/Ajax/ConfigHandler.php
@@ -181,8 +181,7 @@ public function buildForm(array $form, FormStateInterface $form_state, Request $
-        '#ajax' => array(
-        ),
+        '#ajax' => TRUE,

Yeah I agree, this is the less confusing API.

I reviewed the entire patch, without having been involved in this issue. Apologies if some things have been discussed earlier. I thought a fresh look would be useful.

1. +++ b/core/core.api.php
   @@ -2054,12 +2054,6 @@ function hook_validation_constraint_alter(array &$definitions) {
   - * - path: The URL path to use for the request. If omitted, defaults to
   
   +++ b/core/lib/Drupal/Core/Theme/AjaxBasePageNegotiator.php
   @@ -15,12 +15,12 @@
   + * Many different pages can invoke an Ajax request to a generic Ajax path. It is
   
   +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1593,15 +1593,14 @@ protected function drupalGetAjax($path, array $options = array(), array $headers
   +   *   - triggering_element: If the value for the 'path' key is a generic Ajax
   

   These seem to contradict?

2. +++ b/core/includes/pager.inc
   @@ -148,7 +148,7 @@ function pager_default_initialize($total, $limit, $element = 0) {
   -    $query = UrlHelper::filterQueryParameters(\Drupal::request()->query->all(), array('page'));
   +    $query = UrlHelper::filterQueryParameters(\Drupal::request()->query->all(), array('page', \Drupal\Core\Form\FormBuilderInterface::AJAX_FORM_REQUEST));
   

   So the reason for 'page' being excluded is crystal clear.

   The reason for the newly added exclusion is less clear. Perhaps add a line of documentation? Especially the fact that it's \Drupal\Core\Form\FormBuilderInterface::AJAX_FORM_REQUEST that we're excluding and not \Drupal\Core\EventSubscriber\MainContentViewSubscriber::WRAPPER_FORMAT is not intuitively clear. At least not to me.

3. +++ b/core/lib/Drupal/Core/Form/FormState.php
   @@ -475,6 +475,11 @@ public function getButtons() {
   +    // We try to determine whether this is the initial load of the form.
   

   Nit:

   // We determine whether this is the initial rendering of the form.
   

4. +++ b/core/lib/Drupal/Core/Form/FormState.php
   @@ -475,6 +475,11 @@ public function getButtons() {
   +    // For normal forms it is enough
   +    if ($this->isMethodType('GET') && !$this->isSubmitted()) {
   

   The comment is not clear, and I do not understand the !$this->isSubmitted() condition. I bet it's got something to do with how how AJAX GET forms are submitted, but without knowing the details of how that works, this does not seem understandable.
   So: a better comment would be most helpful :)

5. +++ b/core/lib/Drupal/Core/Form/FormState.php
   @@ -483,7 +488,7 @@ public function setCached($cache = TRUE) {
      public function isCached() {
   -    return empty($this->no_cache) && $this->cache;
   +    return empty($this->no_cache) && $this->cache && $this->isMethodType('POST');
      }
   

   Oh hrm… so this seems to contradict the previous bit, because it seems to say that only POST forms can ever be cached?

6. +++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
   @@ -128,14 +128,7 @@ public static function preRenderGroup($element) {
      public static function processAjaxForm(&$element, FormStateInterface $form_state, &$complete_form) {
   -    $element = static::preRenderAjaxForm($element);
   -
   -    // If the element was processed as an #ajax element, and a custom URL was
   -    // provided, set the form to be cached.
   -    if (!empty($element['#ajax_processed']) && !empty($element['#ajax']['url'])) {
   -      $form_state->setCached();
   -    }
   -    return $element;
   +    return static::preRenderAjaxForm($element);
      }
   

   Why not remove processAjaxForm() then, and just update all calls to call preRenderAjaxForm() instead?

   Or would that be a public API change/BC break?

7. +++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
   @@ -159,6 +151,9 @@ public static function processAjaxForm(&$element, FormStateInterface $form_state
   +   * @throws \InvalidArgumentException
   +   *   Thrown when an element provides a custom #ajax URL.
   

   This exists to aid the transition to D8, right? Perhaps add a @todo to remove that exception in D9?

8. +++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
   @@ -173,6 +168,12 @@ public static function preRenderAjaxForm($element) {
   +    // it will still be given an event.
   

   Nit: s/still be given an event/automatically be assigned a sensible event/

9. +++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
   @@ -230,31 +231,29 @@ public static function preRenderAjaxForm($element) {
   +      // Do not support custom URLs, only allow this method to use it for
   +      // internal reasons.
   

   An @see to point to the internals that use this would be helpful.

10. +++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
    @@ -230,31 +231,29 @@ public static function preRenderAjaxForm($element) {
    +      if (array_key_exists('callback', $settings)) {
    

    What happens if callback is not set? Given that neither path nor url are allowed, an initial reading would suggest that #ajax is then effectively a no-op?

11. +++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
    @@ -230,31 +231,29 @@ public static function preRenderAjaxForm($element) {
    +        $settings += ['options' => []];
    +        $settings['options'] += ['query' => []];
    +        $options['query'] = $settings['options']['query'] + $query;
    

    Woah, this is some very confusing code!

12. +++ b/core/modules/simpletest/src/WebTestBase.php
    @@ -1736,7 +1735,7 @@ protected function drupalPostForm($path, $edit, $submit, array $options = array(
    -   *   $ajax_path 'system/ajax' will be used.
    +   *   $ajax_path '/' will be used.
    
    @@ -1807,7 +1806,7 @@ protected function drupalPostAjaxForm($path, $edit, $triggering_element, $ajax_p
    -    // Ajax settings, or else 'system/ajax'.
    +    // Ajax settings, or else '/'.
    
    @@ -1824,11 +1823,8 @@ protected function drupalPostAjaxForm($path, $edit, $triggering_element, $ajax_p
    -    $ajax_path = $this->container->get('unrouted_url_assembler')->assemble('base://' . $ajax_path, $options);
    +    $ajax_path = $ajax_path ? $this->container->get('unrouted_url_assembler')->assemble('base://' . $ajax_path, $options) : '/';
    

    Oh wow, I wonder how that works! I wonder if that change is actually necessary?

13. +++ b/core/modules/simpletest/src/WebTestBase.php
    @@ -1824,11 +1823,8 @@ protected function drupalPostAjaxForm($path, $edit, $triggering_element, $ajax_p
    diff --git a/core/modules/system/src/Controller/FormAjaxController.php b/core/modules/system/src/Controller/FormAjaxController.php
    
    diff --git a/core/modules/system/src/Controller/FormAjaxController.php b/core/modules/system/src/Controller/FormAjaxController.php
    deleted file mode 100644
    

    Wow, nice!

14. +++ b/core/modules/system/src/Tests/Ajax/AjaxFormCacheTest.php
    @@ -37,15 +37,6 @@ public function testFormCacheUsage() {
    -    // Visit a form that is explicitly cached, 3 times.
    -    $cached_form_url = Url::fromRoute('ajax_forms_test.cached_form');
    -    $this->drupalGet($cached_form_url);
    -    $this->drupalGet($cached_form_url);
    -    $this->drupalGet($cached_form_url);
    -
    -    // The number of cache entries should be exactly 3.
    -    $this->assertEqual(3, count($key_value_expirable->getAll()));
    

    Just to check my understanding: this test is removed because it no longer makes sense, because we disallow forms from getting cached on the GET request, correct?

15. +++ b/core/modules/views_ui/src/ViewUI.php
    @@ -578,7 +573,7 @@ public function renderPreview($display_id, $args = array()) {
    -      foreach (array('view_name', 'view_display_id', 'view_args', 'view_path', 'view_dom_id', 'pager_element', 'view_base_path', AjaxResponseSubscriber::AJAX_REQUEST_PARAMETER, 'ajax_page_state', 'form_id', 'form_build_id', 'form_token') as $key) {
    +      foreach (array('view_name', 'view_display_id', 'view_args', 'view_path', 'view_dom_id', 'pager_element', 'view_base_path', FormBuilderInterface::AJAX_FORM_REQUEST, AjaxResponseSubscriber::AJAX_REQUEST_PARAMETER, 'ajax_page_state', 'form_id', 'form_build_id', 'form_token') as $key) {
    

    Here we also add \Drupal\Core\Form\FormBuilderInterface::AJAX_FORM_REQUEST. Probably the same explanation as the one above will apply.

16. +++ b/core/modules/views_ui/src/ViewUI.php
    @@ -855,6 +853,17 @@ public function renderPreview($display_id, $args = array()) {
    +      $elements = $output['preview'];
    +      $output['preview'] = [
    +        '#markup' => $renderer->renderPlain($elements),
    +      ];
    +      BubbleableMetadata::createFromRenderArray($elements)->applyTo($output['preview']);
    

    AFAICT this can be simplified to the following?

    $renderer->renderPlain($output['preview']);
    

17. +++ b/core/tests/Drupal/Tests/Core/Form/FormStateTest.php
    @@ -464,6 +469,40 @@ public function providerTestIsCached() {
    +   * @expectedException \LogicException
    

    Nit: include the exception message — this makes it clearer which exact logic exception you're expecting here (and tests that too).

18. +++ b/core/tests/Drupal/Tests/Core/Form/FormStateTest.php
    @@ -464,6 +469,40 @@ public function providerTestIsCached() {
    +    // Get form submissions should not be cached.
    

    Nit: s/Get/GET/

19. +++ b/core/tests/Drupal/Tests/Core/Render/Element/RenderElementTest.php
    @@ -0,0 +1,155 @@
    +    $prophecy = $this->prophesize('Drupal\Core\Routing\UrlGeneratorInterface');
    

    Wait, what?! I didn't know we had this. Interesting :)

#46.2 - wouldn't #2504709: Prevent _wrapper_format and ajax_form parameters from bleeding through to generated URLs be a more robust solution here? Besides pager links other links may be generated in the preview (e.g. tablesort links) and still take on the carried over querystring elements

#46.2 - wouldn't #2504709: Remove _wrapper_format and _ajax_form parameters from Request object and store in some other service be a more robust solution here? Besides pager links other links may be generated in the preview (e.g. tablesort links) and still take on the carried over querystring elements

Potentially yes, but I would like to see the critical fixed.

These seem to contradict?

Mh I'm not sure what you mean. While forms now always post to the same URL, the ajax framework itself still allows you to use different URLs. Views is heavily using that for example.
Its just the fact that #ajax as part for FAPI no longer can, because that URL controller relied on form cache to exist. Other examples, like views, rely on the usage of tempstore and work fine in the way how it works.

The comment is not clear, and I do not understand the !$this->isSubmitted() condition. I bet it's got something to do with how how AJAX GET forms are submitted, but without knowing the details of how that works, this does not seem understandable.
So: a better comment would be most helpful :)

Well, that is kinda the main question. How do we exactly treat GET form submissions.

  public function rebuildForm($form_id, FormStateInterface &$form_state, $old_form = NULL) {
    $form = $this->retrieveForm($form_id, $form_state);
    // All rebuilt forms will be cached.
    $form_state->setCached();

This is what happening in HEAD already so yeah setCached() ideally should not be called I think.

Why not remove processAjaxForm() then, and just update all calls to call preRenderAjaxForm() instead?

Or would that be a public API change/BC break?

Yeah

Why not remove processAjaxForm() then, and just update all calls to call preRenderAjaxForm() instead?

Or would that be a public API change/BC break?

Either we keep it like that or we write an update function + update test as this missing function would break things, unless the cache is cleared.

This exists to aid the transition to D8, right? Perhaps add a @todo to remove that exception in D9?

Well, the more I think about it, I think having two distinct function, one as a preRender and one as a process function makes sense and its a pure implementation detail that internally they can reuse the code.

What happens if callback is not set? Given that neither path nor url are allowed, an initial reading would suggest that #ajax is then effectively a no-op?

Well no, you can still render the entire form, see #ajax => TRUE

Woah, this is some very confusing code!

Meh

Oh wow, I wonder how that works! I wonder if that change is actually necessary?

Not sure, honestly.

Just to check my understanding: this test is removed because it no longer makes sense, because we disallow forms from getting cached on the GET request, correct?

Exactly.

Here we also add \Drupal\Core\Form\FormBuilderInterface::AJAX_FORM_REQUEST. Probably the same explanation as the one above will apply.

Yeah it doesn't, but its documented below already. Copy stuff but remove ajax-framework specific keys

Nit: include the exception message — this makes it clearer which exact logic exception you're expecting here (and tests that too).

I'd rather argue that we should have a dedicated exception type

Wait, what?! I didn't know we had this. Interesting :)

Let's open up a technical working group issue first, oh damnit, too bad, we already use prophesizy in core, too bad that it is too late.

Implemented Point 16.

.

Only responding to the parts I fully grok, the rest was @dawehner.

#46.4 I don't fully grok the addition of the isSubmitted() part as well, I'm not sure that's the right flag.
#46.5 Yes, this is now contradictory

#46.6 '#type' => 'link' cannot have a #process, it can only have #pre_render. Or is it the other way around? But yeah I think we still need this, effulgentsia knows.

#46.10 The 'event' processing will still happen. For example, if you now have '#ajax' => TRUE for a select element, ['event' => 'change', 'dialogType' => 'ajax'] will be passed to drupalSettings for that element.

#46.12 Well we had to change it to *something*, as we've removed /system/ajax

#46.14 Yes, exactly.

#46.6 '#type' => 'link' cannot have a #process, it can only have #pre_render. Or is it the other way around? But yeah I think we still need this, effulgentsia knows.

Yeah they are two separate concepts ...

When @amateesc, @wimleers and @plach talked about that, we thought that the logic place would be better in the form submission code.

#48.1/#46.1: I meant the fact that we remove the 'path' documentation, yet later refer to it. Still a problem.

#46.6: Either we keep it like that or we write an update function + update test as this missing function would break things, unless the cache is cleared. -> I guess let's defer to a non-critical follow-up then. Let's add a @todo?

#46.17: I'd rather argue that we should have a dedicated exception type Agreed, but I didn't want to impose more work on you :)

The changes in #52 are awesome :)

1. +++ b/core/includes/pager.inc
   @@ -148,7 +148,11 @@ function pager_default_initialize($total, $limit, $element = 0) {
   +    // There is a chance that the pager is rendered in the context of an ajax
   +    // form request. Therefore exclude AJAX_FORM_REQUEST from there, as
   +    // otherwise clicking the link would be interpreted potentially as form
   +    // request, especially when done as POST request using JS.
   

   Thanks for adding these docs to address #46.2. But it's not entirely clear to me yet.

   s/of an ajax/of an AJAX/

   s/from there/from the query arguments/

   s/be interpreted potentially/be potentially interpreted/

   s/as form/as an AJAX form/

2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -313,8 +313,12 @@ public function buildForm($form_id, FormStateInterface &$form_state) {
   +    // We don't allow to set state on GET requests.
   +    if ($form_state->isMethodType('POST')) {
   +      // All POST rebuilt forms will be cached.
   +      $form_state->setCached();
   +    }
   

   Let's keep the outer comment, check the if-test to not GET, and remove the inner comment.

3. +++ b/core/lib/Drupal/Core/Form/FormState.php
   @@ -475,6 +475,10 @@ public function getButtons() {
   +    // We determine whether this is the initial rendering of the form.
   +    if ($this->isMethodType('GET')) {
   +      throw new \LogicException('Initial form rendering is not allowed to cache the form.');
   +    }
   

   Don't we now disallow caching of a GET form ever? If so, then both the comment and exception need to be updated. And I think the comment can in fact be removed: the exception alone is sufficiently clear.

#46.6: Either we keep it like that or we write an update function + update test as this missing function would break things, unless the cache is cleared. -> I guess let's defer to a non-critical follow-up then. Let's add a @todo?

Well there is also the thing of #process vs. #pre_render

s/be interpreted potentially/be potentially interpreted/

There is not potentiallity here, it happens.

Addressing the feedback from wim for now. Berdir feedback is coming directly afterwards.

1. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1649,7 +1648,7 @@ protected function drupalPostForm($path, $edit, $submit, array $options = array(
            if ($ajax) {
   -          $action = $this->getAbsoluteUrl(!empty($submit['path']) ? $submit['path'] : 'system/ajax');
   +          $action = $this->getAbsoluteUrl(!empty($submit['path']) ? $submit['path'] : '');
   

   As discussed, empty string would be the frontpage, so that doesn't seem to make sense. It would have to either default to the current page or throw an exception if we know that it always exists.

2. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1736,7 +1735,7 @@ protected function drupalPostForm($path, $edit, $submit, array $options = array(
       *   element. In the absence of both the triggering element's Ajax path and
   -   *   $ajax_path 'system/ajax' will be used.
   +   *   $ajax_path '/' will be used.
   

   That might also affect this, not exactly sure how, though.

3. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1807,7 +1806,7 @@ protected function drupalPostAjaxForm($path, $edit, $triggering_element, $ajax_p
        // Unless a particular path is specified, use the one specified by the
   -    // Ajax settings, or else 'system/ajax'.
   +    // Ajax settings, or else '/'.
        if (!isset($ajax_path)) {
   
   @@ -1824,11 +1823,8 @@ protected function drupalPostAjaxForm($path, $edit, $triggering_element, $ajax_p
          }
   -      else {
   -        $ajax_path = 'system/ajax';
   -      }
        }
   

   This seems similar, no else anymore, so we should either throw an error if we can't find or rewrite this a bit so we just assume it's there?

+++ b/core/lib/Drupal/Core/Form/FormState.php
@@ -475,6 +475,10 @@ public function getButtons() {
+    // We don't allow to cache on GET requests.
+    if ($this->isMethodType('GET')) {
+      throw new \LogicException('Caching on GET requests is not allowed.');
+    }

Nit: I think the comment can be removed, the exception alone is sufficient.

This seems similar, no else anymore, so we should either throw an error if we can't find or rewrite this a bit so we just assume it's there?

If I remember all the cases, there should be always a path, so let's see whether we fail ...

The failure was a checking to the exception message ...

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -1648,7 +1648,10 @@ protected function drupalPostForm($path, $edit, $submit, array $options = array(
+            throw new \Exception('Missing ajax path specified');

@@ -1824,7 +1826,12 @@ protected function drupalPostAjaxForm($path, $edit, $triggering_element, $ajax_p
+      throw new \Exception('Missing ajax path specified');

Do we use trailing periods for exception messages?

Also: s/ajax/#ajax/ ?

The failure was a checking to the exception message ...

:P

.

1. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -447,9 +447,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   -      '#ajax' => array(
   -        'url' => views_ui_build_form_url($form_state),
   -      ),
   +      '#ajax' => TRUE,
   

   This does not work. If we dont set #ajax['url'], then changing formatter (when adding a field) will take you back to "Add fields" screen (similar problem as in #2248223: Adding a new Views filter and making it exposed returns user back to list of filters).

2. -      '#ajax' => ['url' => NULL],
   +      '#ajax' => TRUE,
   

   Nice.

3. +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -12,7 +12,9 @@
   +use Drupal\Core\Render\BubbleableMetadata;
   

   unused?

Its just the fact that #ajax as part for FAPI no longer can, because that URL controller relied on form cache to exist. Other examples, like views, rely on the usage of tempstore and work fine in the way how it works.

I still don't fully understand why we need to completely remove support for #ajax['url'] even for cases like views ui.

This does not work. If we dont set #ajax['url'], then changing formatter (when adding a field) will take you back to "Add fields" screen (similar problem as in #2248223: Adding a new Views filter and making it exposed returns user back to list of filters).

Oh I've tried it manually, but you are right, the functionality is broken with that.

I still don't fully understand why we need to completely remove support for #ajax['url'] even for cases like views ui.

For views UI we don't rely on form caching but rather we use tempstore and create the form from that object, right.

Overall looks really great!

1. +++ b/core/includes/pager.inc
   @@ -148,7 +148,12 @@ function pager_default_initialize($total, $limit, $element = 0) {
   +    // using JS.
   +    $query = UrlHelper::filterQueryParameters(\Drupal::request()->query->all(), array('page', \Drupal\Core\Form\FormBuilderInterface::AJAX_FORM_REQUEST));
   

   I think we should link to that other issue that provides methods to get the query parameters automatically with removed _format and ajax, etc.

2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -313,8 +313,11 @@ public function buildForm($form_id, FormStateInterface &$form_state) {
   +    // We don't allow to set state on GET requests.
   +    if (!$form_state->isMethodType('GET')) {
   +      $form_state->setCached();
   +    }
   

   Yes!

   I assume we treat HEAD as GET requests. (not that it matters much here)

   For those not knowing:

   In $request there is a method, isMethodSafe() for that.

3. +++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
   @@ -173,6 +169,12 @@ public static function preRenderAjaxForm($element) {
   +    // If #ajax is TRUE, convert it to an empty array. Depending on the #type,
   +    // it will still be given an event.
   +    if (!is_array($element['#ajax'])) {
   +      $element['#ajax'] = [];
   +    }
   

   Is there a reason why we could not check $element['#ajax'] === TRUE

   instead?

4. +++ b/core/lib/Drupal/Core/Theme/AjaxBasePageNegotiator.php
   @@ -15,12 +15,12 @@
   + * Many different pages can invoke an Ajax request to a generic Ajax path. It is
   + * almost always desired for an Ajax response to be rendered using the same
   + * theme as the base page, because most themes are built with the assumption
   

   Oh, nice that means we can remove that now :)

5. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -447,9 +447,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   -      '#ajax' => array(
   -        'url' => views_ui_build_form_url($form_state),
   -      ),
   +      '#ajax' => TRUE,
   

   Is this function still used?

   Likely just needs a different callback instead.

6. +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -358,9 +353,7 @@ public function getStandardButtons(&$form, FormStateInterface $form_state, $form
   -      '#ajax' => array(
   -        'path' => $form_url,
   -      ),
   +      '#ajax' => TRUE,
   

   This feels really good overall.

   It simplifies our ajax system a lot.

7. +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -612,10 +607,13 @@ public function renderPreview($display_id, $args = array()) {
          $request_stack->push($request);
   +      $executable->setRequest($request_stack->getCurrentRequest());
   

   Uh why is this needed and would that not always be $request that we get from the stack?

8. +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -855,6 +853,14 @@ public function renderPreview($display_id, $args = array()) {
   +    if (!empty($output['preview'])) {
   +      // We render the preview now, in order to render it in the context of the
   +      // previous setup request, because for example pager links depend on the
   +      // "current" URL.
   +      $renderer->render($output['preview']);
   +      $output['preview']['#printed'] = FALSE;
   +    }
   

   Actually that is wrong and the legacy render() function needs to be called, because else it would potentially be double rendered.

   Another possibility is to use getCacheableRenderArray() here ...

   Can be follow-up though ...

   render() really is a lost child except for twig.

@olli
Working on it the entire morning .. the fields work fine for me, its rather the views UI which is broken, once you click preview twice... still try to find a proper solution.

Yes!

I assume we treat HEAD as GET requests. (not that it matters much here)

For those not knowing:

Maybe this means we should change it back to !POST. Or just use isMethodSafe().

I still don't fully understand why we need to completely remove support for #ajax['url'] even for cases like views ui.

I tried to keep the old way, but I think we really don't need to. Do you mind providing the example which doesn't work? I manually tested it for quite a while and things continue to work, as expected, at least for me.

I think we should link to that other issue that provides methods to get the query parameters automatically with removed _format and ajax, etc.

Yeah we have an issue to discuss this already: #2504709: Prevent _wrapper_format and ajax_form parameters from bleeding through to generated URLs

In $request there is a method, isMethodSafe() for that.

Ah good to remember.

Is there a reason why we could not check $element['#ajax'] === TRUE
instead?

I like that.

Oh, nice that means we can remove that now :)

No we can't, views/ajax for example still exists ... but yeah I also made often the assumption that ajax.js === #ajax basically.

s this function still used?

Likely just needs a different callback instead.

There is still some usage in core/modules/views_ui/src/Form/Ajax/ViewsFormBase.php:142 left

Uh why is this needed and would that not always be $request that we get from the stack?

Well, we inject the $request into the view, ... this is a great DX thing in general, given that its add less "global"-ness but rather keeps things local. Nothing new though.

Actually that is wrong and the legacy render() function needs to be called, because else it would potentially be double rendered.

Another possibility is to use getCacheableRenderArray() here ...

Can be follow-up though ...

render() really is a lost child except for twig.

Fixed it.

#66: What I meant was:

$request_stack->push($request);
+      $executable->setRequest($request_stack->getCurrentRequest());

Why do we need to get it again from the request_stack when we just pushed $request to the stack, could we not use:

+      $executable->setRequest($request);

instead?

Why do we need to get it again from the request_stack when we just pushed $request to the stack, could we not use:

Well, there are still things, like the pager, we call out to the request stack itself.

Thank you alex!

I agree, its a separate discussion needed. Also in general a custom URL is useful for other examples, for ones which don't care about caching, like views potentially which has everything in tempstore, so we can recover from that.

#69 Links should be fine, unless we allow them to use a callback parameter.

The problems we had with the ajax system for posting to system/ajax were:

- Creating state on GET so that system/ajax knows the state
- Having a callback that had no access control or context of its own (only what system/ajax had)

So if those use neither I think we could be okay to allowing an ajax request to go to another URL - if it does not involve Form API.

- Creating state on GET so that system/ajax knows the state
- Having a callback that had no access control or context of its own (only what system/ajax had)

So if those use neither I think we could be okay to allowing an ajax request to go to another URL - if it does not involve Form API.

Right, we now don't cache on the GET request anymore, so people cannot rely on it any longer.

Opened #2527740: Consider to remove support for #ajax['url'] as follow-up, thanks #73!

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -313,8 +313,11 @@ public function buildForm($form_id, FormStateInterface &$form_state) {
+    if (!$form_state->isMethodType('GET')) {

Should we also check HEAD too? We don't want those to write to cache either.

Also if we do check that, should we also check it when throwing the exception?

Looks great to me apart from that question.

To be clear +1 to remove the decision whether to keep #ajax][url or not.

Removing many test failures for now.

Actually TRACE and OPTIONS seems to be safe, but well, this is kinda an implementation detail.

+++ b/core/lib/Drupal/Core/Form/FormState.php
@@ -105,6 +106,24 @@ class FormState implements FormStateInterface {
+  protected $request;

Just wondering : doesn't this potentially add to the dependency-chain hell when the form state does get stored/serialized ?

It seems earlier approaches didn't involve storing a ref to the Request in the FormState ?

I agree with #87.

While I strongly believe in using a proper isMethodSafe() from request I think what would be fine is to use dawehners #83, but include a \Drupal::request()->isMethodSafe() check - which should be transparent.

Alternatively not the request should be injected, but however the getMethod() gets it information, should use the same for that.

.

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -185,9 +185,19 @@
   +    $form_state->setRequest($request);
   

   It could be $form_state->updateFromRequest($request) to keep our API open for future internal changes, but for now we just store the method type.

2. +++ b/core/lib/Drupal/Core/Form/FormState.php
   @@ -105,6 +106,24 @@
   +   *
   +   * @var \Symfony\Component\HttpFoundation\Request
   +   */
   +  protected $request;
   

   Well yeah I also think we should avoid storing the request, unless we really have to.

I like #92.

Thanks for the clarifications in #91, my view of the form_state persistence flow was a bit naive.
#92 does indeed sound nice though.

I'm fine with #92
In case nobody beats me I'll work on it in a while, but I'm currently into writing stupid tests for tokens.

I did not wanted to kill the test on purpose, for now just a reupload of #85

1. +++ b/core/lib/Drupal/Core/Form/FormState.php
   @@ -141,16 +141,29 @@ class FormState implements FormStateInterface {
   +  protected $request_method = 'GET';
   

   I think we only snake_case'd the legacy properties, new ones like $anyErrors are lowerCamelCase

2. +++ b/core/lib/Drupal/Core/Form/FormState.php
   @@ -475,6 +488,12 @@ public function getButtons() {
   +      throw new \LogicException(sprintf('Form state caching on %s requests is not allowed.', $this->request->getMethod()));
   

   I guess we either need a getRequestMethod(), or more likely, just use $this->request_method directly.

I think it should be possible to still opt into caching, with an extra parameter to bypass the validation.

Fix the remaining one.

A few general questions etc, haven't read previous reviews/comments so apologies if going over old ground

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -185,9 +185,22 @@ public function buildForm($form_id, FormStateInterface &$form_state) {
   +    // Inform $form_state about the request method that's building it, so that
   +    // it can prevent persisting state changes during HTTP methods for which
   +    // that is disallowed by HTTP.
   +    $form_state->setRequestMethod($request->getMethod());
   +
   +    // Initialize the form's user input. The user input should include only the
   

   This comment doesn't read well. Suggest 'HTTP methods which are not supported' perhaps?

2. +++ b/core/lib/Drupal/Core/Form/FormStateInterface.php
   @@ -638,10 +638,17 @@ public function getButtons();
   +   *   you can opt into that.
   

   suggest
   By default GET request cannot be cached. Set this to TRUE to allow caching of forms using a GET request.

3. +++ b/core/lib/Drupal/Core/Form/FormStateInterface.php
   @@ -731,18 +738,35 @@ public function setLimitValidationErrors($limit_validation_errors);
   +   *   Either "GET" or "POST". Other HTTP methods are not valid form submission
   +   *   methods.
   

   This is not enforced on FormState::setMethod - should it be?

4. +++ b/core/lib/Drupal/Core/Form/FormStateInterface.php
   @@ -731,18 +738,35 @@ public function setLimitValidationErrors($limit_validation_errors);
   +  public function setRequestMethod($method);
   

   Should we also add a @see to setMethod()

5. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1824,10 +1825,12 @@ protected function drupalPostAjaxForm($path, $edit, $triggering_element, $ajax_p
   +      throw new \Exception('No #ajax path specified.');
   

   Should we be adding a test for this too?

6. +++ b/core/modules/system/src/Tests/Ajax/AjaxFormPageCacheTest.php
   @@ -35,7 +35,7 @@ protected function getFormBuildId() {
   +   * Create a simple form, then POST via AJAX to change to it.
   

   nit: realise its existing but 'to change to it' doesn't really make much sense?

7. +++ b/core/modules/system/src/Tests/Form/FormStoragePageCacheTest.php
   @@ -96,10 +96,15 @@ public function testRebuildFormStorageOnCachedPage() {
   +    $this->assertNoText('No old build id', 'There is an old build id on the page.');
   +    $this->assertNoText($build_id_initial, 'The old build id is not the initial build id.');
   

   We have to assertNoText here but no assertText (i.e. no positive assert), should we add one to ensure we're getting what we were expecting and not e.g. an error page etc.

Mh, that is fair. I was a little bit influenced yesterday night. I'll work on it now.

Daniel has been distracted by another issue, taking over for now :)

Posting what I have so far, since I'll switch to another issue for a bit.

I started with the patch from #101, so the interdiff is relative to that one.

This comment doesn't read well. Suggest 'HTTP methods which are not supported' perhaps?

I added GET and HEAD, so I think this explains more what is going on.

This is not enforced on FormState::setMethod - should it be?

Mh, in an ideal world yes, but also it practically never matters.

Should we be adding a test for this too?

Meh, its something just dealt for developers, which run simpletests, and not had a problem on the actual site.
Not even sure how to test that properly with a limited amount of insanity.

I removed tests which IMHO simply doesn't make sense anymore and fixed some other ones,
but I'm not entirely sure ATM. what the expected result of \Drupal\system\Tests\Form\StorageTest::testImmutableFormLegacyProtection and \Drupal\system\Tests\Form\StorageTest::testImmutableForm is.

@effulgentsia I would be happy about the remaining ones, but I'm not exactly sure what the expected behaviour is, so just changing the test is not right.

+++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestStorageForm.php
@@ -82,8 +82,19 @@
+        $form_state->setRequestMethod('FAKE');

OOH that seems evil. It rather let me ask myself, whether that method should do validation. I guess that is a bit of too much babysitting.

So there's nothing wrong with allowing a FAKE method. And, it's fine to cache forms for that method, since HTTP 1.1 doesn't define FAKE as a method that must be safe. What's dangerous is that $form_state->setRequestMethod() can be called with something that isn't $request->getMethod(), which means it can be tricked into doing something inappropriate for the actual request, but I don't know if or how to babysit that, because even $form_state->setRequest() or $form_state->(set|update)FromRequest() would have that problem, since you can pass in a fake $request to that. Even \Drupal::request() can be circumvented by temporarily pushing to the request stack and then popping.

And all that is totally fine, we are not nazis. If there are advanced usecases, allow people to deal with it, but well then they need to live with the consequences.

Strictly speaking some of the new test coverage is now for code which is not touched, but honestly, new test coverage is not bad, especially
when we want to change something in the follow up issue.

We already have a change record.

RTBC + 1

--

Proposed commit message:

Issue #2502785 by dawehner, effulgentsia, tim.plunkett, amateescu, Wim Leers, Fabianx, catch, dsnopek, EclipseGc, yched, mondrake, larowlan, olli, Berdir: Remove support for $form_state->setCached() for GET requests

Core committers can obviously decide differently, but I read every comment and all those made useful contributions to the patch / approach.

ConfigFormOverrideTest had two failures.

1. +++ b/core/core.api.php
   @@ -2054,12 +2054,6 @@ function hook_validation_constraint_alter(array &$definitions) {
   - * - path: The URL path to use for the request. If omitted, defaults to
   - *   'system/ajax', which invokes the default Drupal Ajax processing (this will
   - *   call the callback supplied in the 'callback' element). If you supply a
   - *   path, you must set up a routing entry to handle the request yourself and
   - *   return output described in @ref sub_callback below. See the
   - *   @link menu Routing topic @endlink for more information on routing.
     * - wrapper: The HTML 'id' attribute of the area where the content returned by
     *   the callback should be placed. Note that callbacks have a choice of
     *   returning content or JavaScript commands; 'wrapper' is used for content
   @@ -2085,6 +2079,13 @@ function hook_validation_constraint_alter(array &$definitions) {
   
   @@ -2085,6 +2079,13 @@ function hook_validation_constraint_alter(array &$definitions) {
     *   - message: Translated message to display.
     *   - url: For a bar progress indicator, URL path for determining progress.
     *   - interval: For a bar progress indicator, how often to update it.
   + * - url: A \Drupal\Core\Url to which to submit the Ajax request. If omitted,
   + *   defaults to either the same URL as the form or link destination is for
   + *   someone with JavaScript disabled, or a slightly modified version (e.g.,
   + *   with a query parameter added, removed, or changed) of that URL if
   + *   necessary to support Drupal's content negotiation. It is recommended to
   + *   omit this key and use Drupal's content negotiation rather than using
   + *   substantially different URLs between Ajax and non-Ajax.
   

   Shouldn't this change be in the CR. Talked about this with @dawehner - this change was earlier and this patch is just making it explicit and fixing the docs.

2.     $form['actions']['cancel'] = array(
         '#type' => 'submit',
         '#value' => !$form_state->get('ok_button') ? t('Cancel') : t('Ok'),
         '#submit' => array($cancel_submit),
         '#validate' => array(),
         '#ajax' => array(
           'path' => $form_url,
         ),
         '#limit_validation_errors' => array(),
       );
   

   Given point 1 - this code in core/modules/views_ui/src/ViewUI.php looks wrong... I think it should just be 'url' => $form_url - but according to @dawehner it's working atm. This should be a followup - or perhaps part of #2527740: Consider to remove support for #ajax['url']

3. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1593,15 +1593,14 @@ protected function drupalGetAjax($path, array $options = array(), array $headers
       *   - path: A path to submit the form values to for Ajax-specific processing,
       *     which is likely different than the $path parameter used for retrieving
   -   *     the initial form. Defaults to 'system/ajax'.
   -   *   - triggering_element: If the value for the 'path' key is 'system/ajax' or
   -   *     another generic Ajax processing path, this needs to be set to the name
   -   *     of the element. If the name doesn't identify the element uniquely, then
   -   *     this should instead be an array with a single key/value pair,
   -   *     corresponding to the element name and value. The callback for the
   -   *     generic Ajax processing path uses this to find the #ajax information
   -   *     for the element, including which specific callback to use for
   -   *     processing the request.
   +   *     the initial form.
   

   The docs for path look incorrect. I don;t think that is likely different than the $path parameter used for retrieving the initial form

4. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1593,15 +1593,14 @@ protected function drupalGetAjax($path, array $options = array(), array $headers
   +   *     element name and value. The callback for the generic Ajax processing
   +   *     path uses this to find the #ajax information for the element, including
   

   Does the "callback for the generic Ajax processing path" really exist anymore?

5. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1649,7 +1648,10 @@ protected function drupalPostForm($path, $edit, $submit, array $options = array(
   +          if (empty($submit['path'])) {
   +            throw new \Exception('No #ajax path specified.');
   +          }
   +          $action = $this->getAbsoluteUrl($submit['path']);
   

   It seems weird to me that this change has not caused a single change to any calls to drupalPostForm... Ah I see we've changed drupalPostAjaxForm and we're getting the path from $this->drupalSettings for calls like $this->drupalPostAjaxForm(NULL, $edit, 'editor_configure');. Nice.

Given the discussion in IRC, this is back to RTBC.

Thanks for addressing my feedback. Committed 7eb616f and pushed to 8.0.x. Thanks!

Now that this has landed, let's continue in #2524408: Remove $form_state immutability!