Bypass form caching by default for forms using #ajax.

Some work went into fixing this at #2183275: Use cache for $form, kv for $form_state. Not sure if we should postpone this or what.

Is there anything which blocks implementing the current proposed solution: just store it per session per URL?

Note that #2242749: Port Form API security fix SA-CORE-2014-002 to Drupal 8 fixes that for forms displayed on cacheable pages.

Let's play the devil's advocate, is this more of a problem than in Drupal 7?

The only difference from my point of view, is that in Drupal 8 we potentially store more data in there, but the problem described in the title of the issue
is true for D7 as well.

It is a *bigger problem* (Yes, also a D7 problem, I have seen insanely huge cache_form tables, both due to # of rows and single large rows) due to form objects and dependency injection. Forms in D8 tend to have a lot more objects.

Another difference is that it is now a key value table, but it was always documented that cache_form wasn't really a cache table either.

It's more of a problem than Drupal 7 because admin/content now triggers the condition, see issue summary of #2252763: Views exposed filter form causes enormous form state cache entries.

Stupid question here, but the proposed resolution has

Reduce these form cache entries to one per session, or better.

If I have the same form open in different tabs (which I commonly do), doesn't this eliminate the per-session idea?

Are we worried about the number of rows or the size of each row? This may be a really dumb idea, and a performance hit, but what about gzdeflate/gzinflate around the serialized data in the KV store if the string is bigger than 4K or so?

Or adding a parameter to FormCache::setCache() to allow forms to specify their own expiration? I don't think I have ever stared at a views page for six hours (the expiration time).

Should we make this a dupe of #2183275-24: Use cache for $form, kv for $form_state? I think thats the next step here, no?

That might be one way, although I still don't quite understand what that issue there is doing.

Apart from that, I think we also need to identify bad specific forms and find ways to serialize less there, like Damien is doing in #2252763: Views exposed filter form causes enormous form state cache entries. I could imagine a node edit form with lots of fields also being pretty bad.

@Moshe I don't think it's a dup. The other issue is about reducing the amount of data that gets serialized.

This issue is about reducing the necessity to write data every request when viewing a cache-enabled form. Discussed briefly with Damian that we could try something like writing a single entry per-user per-form per-session, then once a form has been submitted update the form to reflect this. That covers the worst case of a personalized form, but then there's also the cacheable_csrf situation where you have an AJAX form that's the same for everyone - which can't be HTML cached at the moment since if there's nothing written to from cache it won't submit at all.

Discussed this a bit with Damian. This isn't a full plan yet, but some progress:

1. We should be able to avoid writing to form state when a form is just viewed - there's no real need to.
2. This means that on first submit of a form, we'll need to generate a unique ID which then follows along.
3. Rather than #2183275: Use cache for $form, kv for $form_state I think we could consider using the session for form state. It's all user-specific data, and that may help with garbage collection too.

We should be able to avoid writing to form state when a form is just viewed - there's no real need to.

Wouldn't that be nice? The problem you'll run into though is explained in #774876-11: Form cache entries can expire before their host page's cache entry, resulting in all AJAX failing. #597280: Introduce form registry to fix various problems and increase security is one way to solve that. Open to other ideas too.

Maybe it's possible to solve #18 with a session variable that only stores a list of form IDs (or equivalent form constructor information) that the user has viewed, which we can then validate against when processing an AJAX submission.

Maybe it's possible to solve #18 with a session variable that only stores a list of form IDs (or equivalent form constructor information) that the user has viewed,

That won't work for anonymous users and page caching - we can't start a session just because someone viewed a form.

We could however store only a form ID (not all of form_state, just an ID to avoid arbitrary submissions) in state or similar, and then use a session variable for authenticated users.

We will be doing #2351015: URL generation does not bubble cache contexts for render cache effectiveness. Not sure if that affects anything here.

This is the summary of a chat conversation with Catch to kill $form_state:

Historical Background

The main reasons for needing a form_state are:

- 1. Security (AJAX)
- 2. Multi-Step forms
- 3. Form Defaults (#default_value => 123)
- 4. Form parameters (drupal_get_form('foo', 'bar'))

The reason why AJAX needs $form_state is because it uses a different route to get the ajax data, so its the access of the main route that is problematic here.

Why $form_state is problematic for Drupal 8

- Render Caching is difficult / impossible when the form is unique
- Cached form data can get huge, especially with serialization, serialized objects are our new Arrays of Doom! :)

How to kill $form_state

To kill $form_state all 4 steps need to be addressed adequately:

AJAX, Multi-Step, Form Defaults, Parameters

1. AJAX

The simplest to address the ajax problem is to use the original route for access checking and have the form POST to the same URL as the _GET / _POST one.

While it before was impossible / well difficult to not call and execute the main request and hi-jack the request, this is in a route and format enabled world now much simpler.

Already now drupal.ajax requests send e.g. ajax_page_state and ajax_page_html_ids, so why not a simple ajax=1 using for getting the right controller, while still using the main route for access checks and providing the right $request data to the form.

Not sure how exactly to implement, but thats idea 1) and it works well for forms that are just once on a page and e.g. the user_login_block could either define a cache_context.page OR POST to /user/login (or some other defined path) always, where in both cases the route for POSTing normally is the same as for posting AJAX.

Idea 2) is adding of an AccessInterface to form controllers itself, which could be as simple as deferring to a route definition or defining a role or permission or custom callback. Same story as with routes really.

Given however that any custom access could be solved with 1) by defining a route with the right permission, where e.g the block is displayed I still tend to 1).

Idea 3) is similar to 1), but just defining a signed real_route parameter, which is used by the ajax callback to perform access and set the $request for the FormController.

It is solvable however and maybe simpler than ever before in Drupals history.

Multi-Step Forms

Have been one of the complicated things always

There is two cases:

a) Authenticated multi-step

That is a no-brainer just store your own data in $_SESSION. If you put big serialized objects in there, ... not our problem. Many already used $_SESSION or a custom entity for such undertaking to store progress.

b) Anonymous multi-step

_SESSION is not suitable here, but its pretty simple to keep an identifier yourself, that you could store in a cookie (cache_context.cookie) or a path parameter (cache_context.request_parameter) or both. Then based on that identifier you can just store to tempstore yourself.

I need to ask Yuri Gerasimov if we ever published some code that worked based on an identifier and stored data to an entity.

Many leaks have occurred where data blurred from one multi-step session to another that I had seen, so this approaches are better than that.

Therefore my vote would be to drop multi-step support from core and rather encourage contrib/ to create a good solution for that of which I explained above.

I don't think its a use-case core needs to support if its so simple with K/V store to implement yourself.

3. Form Defaults

This will probably be the biggest WTF, but the same as elsewhere in core/ you will need to learn to set proper cache_contexts.

If your defaults depend on something that is not in the request and not the form, then you need to declare that as a cache_context.

However that is not different than writing anything else that can be cached, just maybe a little less used to ...

4. Form Parameters

This is the same dilemma really as #pre_render and #post_render_cache, those form parameters should be no big entities or such, but just simple integers, strings, etc.

For more complex cases a loader function could be provided maybe, which then can use the request and set a cache context, etc.

It is solvable however.

This loader is used for when an ajax request is used to get to the FormController directly without going through the execution of the normal page controller.

e.g. solved in cacheable_csrf with http://cgit.drupalcode.org/cacheable_csrf/tree/cacheable_csrf.module?h=7...

Summary

Killing $form_state completely is possible.

It would need to:

- Find a solution for the routing of ajax requests (1./2. or 3.)
- Possibly kill multi-step or add another API that is less brittle
- Ensure #default_value sets the right cache contexts, might default to a cache_context per route / request / page to make it simpler that you can opt-out of by unsetting it or overriding the FormController? Yes, likely have BaseFormController implement CacheableInterface and overwrite getCacheContexts() with your own.
- Give a different way of loading and providing default parameters

As much as I agree with "serialized objects are our new Arrays of Doom!", killing multistep hardly feels like an option ?

An Ajax form is basically "just" a multistep form that posts to specific paths when JS is enabled. Multistep forms are what you degrade to when JS is off.
We have full UIs built on that (Views UI, Field UI are the 1st that come to my mind, I guess lots of others for filters, image effects...)
Or siply any content entity form with an "add new field value" button - without JS, this is just a multistep form ?

#23:

This is a good point, but we have to distinguish two cases of multi-step:

a) Data that is in the form itself: E.g. an ajax enabled form that changes a widget based on the value of some option field.

The state of this form is always the same and it must be as there is no internal state that changes something, its purely the users choice that affects the state:

- AJAX request -> build form -> form
- POST request -> validate form -> fail -> form
- POST request -> build form -> form

This is not multi-step, the same if you e.g. use a 'step' variable and store it hidden in the form.

Even for file fields with AJAX we store the state that a file was uploaded directly in the form itself by providing the fid=123456 hidden variable.

I should be able to remove that form_state and it should still POST properly (gonna try that!).

I would argue that much of our AJAX and UIs is this case.

b) Data that is stored as internal state and has changed internally, but not on the form itself.

This is the difficult case, which never worked for anonymous users properly unless you took very special care.

And for authenticated users this could well be $_SESSION instead.

--

It needs testing for sure and also see if e.g. $form_state['entity'] = $entity can be adequately rebuild, but in the most cases the form is rebuild already with current values anyway as for non-AJAX forms we don't store $form_state anyway.

Catch wrote in IRC:

- We could keep $form_state['storage'] as BC API, but store in $_SESSION always for anonymous and authenticated users as a simple scheme.

Tagging to discuss this in more depth with the core committers.

Related: #2473759: Form caches should be deleted after submission

Other tag.

Tagging.

Pair programmed with @effulgentsia.

Warning: early PoC!

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -180,6 +181,10 @@ public function getForm($form_arg) {
+    $form_build_key = 'form_build_key:' . Crypt::hmacBase64(serialize($form_id) . serialize($form_state), 'form_key' . $private_key->get() . Settings::getHashSalt());

its a bit odd that we don't add the user here, at least, you would kinda expect that, wouldn't you? I'm curious whether this would be a feature some forms would opt in, like ajax forms, or would it replace the way how we cache form state, so instead of relying on form build ID, rely on the serialized representation of the form state?

Rebased.

Note that the PoC didn't yet solve the STR in the IS. The root cause is ViewsForm calling ::setCached(), which effulgentsia and I don't understand why it's necessary. See #2486433: Make ViewsForm stop marking itself as needing to be cached for a patch where that is removed, the test results will hopefully make it clear if it is necessary or not.

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -180,6 +181,10 @@ public function getForm($form_arg) {
   +    $private_key = \Drupal::service('private_key');
   +    $keyvalue = \Drupal::service('keyvalue.expirable');
   
   +++ b/core/modules/system/src/Controller/FormAjaxController.php
   @@ -216,4 +214,27 @@ protected function getForm(Request $request) {
   +    $keyvalue = \Drupal::service('keyvalue.expirable');
   +    $result = $keyvalue->get('form_build_key')->get($form_build_key);
   

   Any reason not to inject these?
   Also, we already have the form cache service, which is already injected - so perhaps these responsibilities should live there?
   $this->formCache->get|setFormBuildKey or similar?

2. +++ b/core/modules/system/src/Controller/FormAjaxController.php
   @@ -216,4 +214,27 @@ protected function getForm(Request $request) {
   +  protected function buildForm(Request $request) {
   

   Needs a docblock

ViewsForm calling ::setCached(),

But is that actually a problem we care about? Its a rare happening form, in comparison to admin/content for example or even random frontend forms.

But is that actually a problem we care about? Its a rare happening form, in comparison to admin/content for example or even random frontend forms.

Yes; it's the entire reason why /admin/content causes the key_value_expire table to grow.

OH sorry, I thought that would the views edit form.

#2486433: Make ViewsForm stop marking itself as needing to be cached came back green, but Tim Plunkett said

Honestly I would assume this to be an indication of insufficient test coverage more than anything

But it's not clear to me why that's not the case. See that issue for more about that; it's closely related to this issue.

Working on this

I think I agree with moving this to FormCache, but I did not do that yet.
Currently getting Exception: Serialization of 'Closure' is not allowed in some unit tests.

Note larowlan did some work in #2421503-20: SA-CORE-2014-002 forward port only checks internal cache to move form_state into $_SESSION, which is one of the approaches we've discussed here.

Known issues:

• Installer doesn't have a hash_salt at that point
• Serializing the form is really not working out. Even though it is using DependencySerializationTrait.
• All ajax form tests are failing :)

Ugh.

Wouldn't that be nice? The problem you'll run into though is explained in #774876-11: Form cache entries can expire before their host page's cache entry. #597280: Introduce form registry to fix various problems and increase security is one way to solve that. Open to other ideas too.

I'm not sure a form registry would be enough, as we still need the contextual data to generate the form again. But couldn't we not leverage the same mechanism as for return form redirects to return ajax response if an ajax request is sent to the same URL? Then we could just rebuild the form instead of requiring the cache.

Serializing the form is really not working out. Even though it is using DependencySerializationTrait.

I guess the problem is that we also support anonymous functions at the moment?

#48 is right and if I understood correctly that is more or less also what @Fabianx in #22 meant.

In my opinion we should try to kill any kind of form cache altogether in order to make this work in a secure way. I just fixed the second issue in Authcache which was caused by an user id leaking into the form cache through a value element. Whenever a cloned version of the cached form was reused by another user, the original value was still present and caused trouble. This only happened for Ajax enabled forms when the markup was cached for authenticated users. The same thing could happen if anything (including form alters) decides to add some per-user value into the form state. Maybe it would be possible to mitigate that risk with cache-context, but a better option would be to rebuild the form on every request from scratch.

Some forms require incremental updates (Ajax / multi-step). The problem that we have now with form state is that it is already populated during the initial form build. I think that we should only start to use it when rebuilding the form for the first time. I.e. no more form_build_id in cached markup.

But couldn't we not leverage the same mechanism as for return form redirects to return ajax response if an ajax request is sent to the same URL? Then we could just rebuild the form instead of requiring the cache.

This sounds ideal. We currently rely on the cache entry to ensure the user has access to the form that's being submitted, but if we just post to the same URL then that's covered too.

@catch asked me to post my patch from #2421503: SA-CORE-2014-002 forward port only checks internal cache
So here it is.

More work on #52 - more passes locally.

holy ship, 18 fails

so remaining fails are because ajax form now triggers session which means no page cache

It seems to me that this patch simply shifts the problem from the keyvalue table to the session table?

Yeah, wondering that too.

Also keep in mind that all session data is always loaded, on every request. So all you need to do is click around a bit on pages that have cache enabled forms, e.g. just reload node/add/article a few times and your session table will grow and grow... and unlike expirable key/value, we can't delete old records here?

Right, @catch asked me to post it here, it's more to address #2421503: SA-CORE-2014-002 forward port only checks internal cache, but I think the idea is we could possibly kill both birds with one stone here, but I assume that would mean combining this patch with the earlier one?

Edit: disregard this idea and skip to #62.

1. If a form uses GET it should not depend on state. That clashes with the semantics of GET I think and I do not think there's a use for it anyways. So any time a GET request arrives we can build from ground up.
2. For POST / AJAX we can store the form cache in a hidden field encrypted. We can perhaps use JSON instead of PHP serialize, compress it and then encrypt it. The only thing we would store server side is the encryption key, I would say once per session.

Disregard. I have a better idea. Let's review the problem.

Let's say you have a form. You know the form id, you know the arguments, you create the form, this is state S₀. Now an input I₁ happens, the form changes its state to S₁, currently we store this state so that the next input can act on it. So when I₂ happens, we retrieve S₁, apply I₂ and the form changes its state to S₂.

But we could instead build the S₀ without any stored data, apply I₁ (if stored somewhere) and then I₂. We can even add a capability to the form so that it return a reduced I^'₂ which can be applied to S₀ to immediately arrive to S₂. I_x can safely be stored client side (edit: in a hidden form field): it is client input after all.

Here's a draft.

Still draft but perhaps a little less broken.

A typical "edit a View" flow can include 10+ form-rebuilding interactions (add field, reorder, add other field, adjust params, preview... until you finally hit "save"). Replaying all input steps from scratch each time sounds like each additional step gets slower and slower ?

Also, replaying each submit several time sounds dangerous if a given submit() has persistent effects (like creating or removing a record somewhere) ? We currently have no requirement whatsoever that the code in a submit() has to be re-entrant ?

> Replaying all input steps from scratch each time sounds like each additional step gets slower and slower ?

We can even add a capability to the form so that it return a reduced I^'₂ which can be applied to S₀ to immediately arrive to S₂

Now that I am coding this, it seems a mergeDeepArray() will be enough in most (all? not sure) cases.

> Also, replaying each submit several time sounds dangerous if a given submit() has persistent effects. We currently have no requirement whatsoever that the code in a submit() has to be re-entrant

Let's say you remove a field item, even if it is a file. Then you close the tab without saving the entity. Did the entity change? I think not. We do not have such a requirement in documentation but perhaps we do in user expectation? Absolutely not sure. I am not going to force this. I am just offering a solution. Didn't even update the issue summary yet.

@chx: I think file management is an interesting case, but should be looked at the other way around : instead of speaking about file item deletions, an interesting case is the file upload. That's probably one of the ajax form usage that every Drupal user is going to face at one point, and that's one of the non re-entrant submit callback @yched mentionned.

Right now, when someone uploads a file through the ajax upload callback, a temporary file entity is created in the submit callback, and that file fid is stored in the form_state. From that point on, the only information about the file round tripping between the server and the client is the file identifier.

In a world of purely re-entrant submit callbacks, you can't create that temporary file anymore. That means you'll have to send it back in full to the client (base64 encoded to be JSON friendly ?) for it to be stored in that hidden input, and the client will then have to send that data back at every subsequent request, which means both for the final non-ajax form submission and for example every time a "Add another item" button is clicked.

That's doable, but it pretty much kills all the advantages of having AJAX file uploading in the first place ; I don't have the issue at hand, but I'm pretty sure that one of the goal when introducing it was to prevent the long upload file phase with nothing for the user to see when the file is POST-ed without AJAX.

I guess file upload are probably the upper boundary for the I_x growth, but I fear it's not the only problem of this proposal.

Another end of the spectrum is related to multi-step forms that are doing some not cheap calculations in intermediary steps. For example, a multi-step form that takes a country / zip code pair in step 1, and then sends that to an external kinda slow webservice to get a list of whatever you want in that zip code, and correctly display options based on that. In that case, the user input is small and can easily be stored in an hidden field. The processing cost isn't though, and having to pay it for every subsequent request seems bad.

I might have missed something in the proposal though.

I think the most important idea in #62 is that I₀ should be reconstructable, i.e. the initial form must be stateless. There is no problem of saving intermediate state in a key-value store and pass a reference to it forth and back between the user agent and the server.

Yes agreed with #71. The performance issue here is not that we store state once the form has been submitted, it's that we store state when just viewing the form whether it's submitted or not. If we can resolve that (while keeping AJAX requests secure), then it'd be plenty to fix this issue - and help to unblock several others.

I agree, it is the I_0 we need to have stateless, for the remainder it is okay to track state.

I think the most important idea in #62 is that I₀ should be reconstructable, i.e. the initial form must be stateless. There is no problem of saving intermediate state in a key-value store and pass a reference to it forth and back between the user agent and the server.

… and this is exactly what @effulgentsia and I did in #33! :) But rather than requiring statelessness entirely, we ensure that all instances of the form reuse the same I₀, and then branch off from there in the same was as HEAD does, i.e. upon actually interacting with the form.

Working more on the original approach.

We can divide the errors into 3 distinct piles:

• Entity forms do not have their entity on rebuild: Call to a member function getEntityTypeId() on a non-object in /var/www/d8/core/lib/Drupal/Core/Entity/EntityForm.php on line 73
• Installer tests: RuntimeException: Missing $settings['hash_salt'] in settings.php. in Drupal\Core\Site\Settings::getHashSalt() (line 146 of core/lib/Drupal/Core/Site/Settings.php).
• Everything else

This works around the installer tests, is it okay to skip the hash salt for those?

As far as I can see there's no loss of security by removing CSRF protection from the installer form -- that's what it is, right?

Do we have a problem with CSRF (form token) or form cache (form build id) in the installer?

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -313,6 +313,18 @@ public function buildForm($form_id, FormStateInterface &$form_state) {
+    // @todo What to return here?
+    return '';

If the hash_salt is not available we usually return a Random number (e.g. in Element/Messages? Forgot the class name ... will look up ...).

That is way less secure, but better than no "salt" at all.

Thanks! It was indeed in \Drupal\Core\Render\Element\StatusMessages::generatePlaceholder(), I borrowed that approach.

10:29 timplunkett: WimLeers: ping re: the cached form issue
10:29 WimLeers: timplunkett: ack
10:32 timplunkett: WimLeers: when an entity form is initially built, it's called from HtmlEntityFormController or EntityFormBuilder, which sets the entity directly via setEntity
10:33 timplunkett: WimLeers: in the case of an entity form being rebuilt via FormAjaxController, it does not call that
10:33 timplunkett: any ideas on how to fix that *without* posting to the original path instead of system/ajax?
10:37 WimLeers: timplunkett: looking/thinking
10:39 WimLeers: timplunkett: If the entity would be stored in FormState instead, there'd be no problem.
10:39 WimLeers: timplunkett: any chance you can change it to do that?
10:40 timplunkett: hah, i moved it out of form_state, lol
10:40 timplunkett: whoops
10:40 timplunkett: (a long while ago)
10:40 WimLeers: Because hilariously
10:40 WimLeers: $this->entity = $form_state->getFormObject()->getEntity();
10:40 WimLeers: Well, you couldn't possibly have anticipated all this :)
10:41 WimLeers: It's the fact that the flow/performance of forms has been so vaguely defined all this time that is the real problem.
10:41 WimLeers: i.e. you had the *choice*: you could do it *either* using a FormInterface object, *or* using FormState
10:42 WimLeers: that kind of choice is usually not good
10:42 WimLeers: timplunkett: ^^
10:42 WimLeers: timplunkett: But that's basically the only thing I can think of.
10:42 timplunkett: i will try it
10:52 timplunkett: well shit
10:52 timplunkett: then we'd need to pass the $form_state to getEntity and setEntity everywhere
10:52 timplunkett: which is exactly why i removed this
10:55 timplunkett: 1913618
10:55 Druplicon: https://drupal.org/node/1913618 => Convert EntityFormControllerInterface to extend FormInterface #1913618: Convert EntityFormControllerInterface to extend FormInterface => 66 comments, 14 IRC mentions
10:55 timplunkett: -    $entity = $this->getEntity($form_state);
10:55 timplunkett: +    $entity = $this->entity;
10:55 timplunkett: april 30th 2013
11:00 timplunkett: WimLeers: $form_state->getFormObject()->getEntity($form_state); would be immensely stupid :)
11:01 timplunkett: unless we bite the bullet and add $form_state->getEntity() and be okay with it returning NULL outside of an entity form
11:10 timplunkett: :(
11:25 catch: timplunkett: posting to the original path would be good for other reasons.
11:25 catch: timplunkett: since we could rely on the route access instead of form cache to ensure the user has access to the form.
11:25 timplunkett: catch: i'll need to talk to eff more, he explicitly didn't want to do that
11:26 catch: timplunkett: it'll be slower for the POST requests.
11:26 catch: Because you have to rebuild that entire page.
11:26 timplunkett: and he's going to be out of commission for a bit longer...
11:26 timplunkett: right
11:27 catch: But that's a bit meh for me compared to everything else.
11:27 catch: And non-ajax forms have the same problem.
11:27 catch: Would love to fix form submissions so we can keep state without a complete page rebuild, but then if we did that it'd fix both.
11:27 catch: And this also opens up the possibility to render cache those forms.
11:28 catch: Because again the POST will just work then.

On the form requests being slower:

Not necessarily, we only need to pass "access" and "have route context" then can go the normal system/ajax route / Controller.

The point we need here is TADA, a _GET parameter to select a subformat of e.g. ?format=drupal_ajax_post

Then either during routing or afterwards or however that works we can switch from the 'original' controller to the ajax controller ...

That keeps flexibility, but also gives us the original request. This is also part of my battle plan for ESI (even though I use a middle ware there.)

I think the main thing is to route first normally, then switch out the controller if format == drupal_ajax_post.

#83 sounds promising. First, I want to see what breaks with the most simplistic change possible.

+++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
@@ -237,8 +237,9 @@ public static function preRenderAjaxForm($element) {
+      $route_match = \Drupal::routeMatch();
...
+        'url' => isset($settings['callback']) ? Url::fromRoute($route_match->getRouteName(), $route_match->getRawParameters()->all()) : NULL,

Just Url::fromRouteMatch(\Drupal::routeMatch())

Just Url::fromRouteMatch(\Drupal::routeMatch())

/me feels stupid now.

Or even easier: Url::fromRoute('<current>');

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -179,6 +200,17 @@ public function getForm($form_arg) {
+    $temp_form_id = is_object($form_id) ? get_class($form_id) : $form_id;
+    try {
+      $hash_salt = Settings::getHashSalt();
+    }
+    catch (\RuntimeException $e) {
+      // During the installer no hash salt is defined yet.
+      $hash_salt = Crypt::randomBytes(8);
+    }
+    $form_build_key = 'form_build_key:' . Crypt::hmacBase64($temp_form_id . serialize($form_state), 'form_key' . $this->privateKey->get() . $hash_salt);
+    $this->keyValueExpirableFactory->get('form_build_key')->setWithExpire($form_build_key, [$temp_form_id, $form_state], 21600);

Let's ensure on the longrun that we have good documentation about what is going on here.

Hah, thanks @dawehner, I knew there had to be something less awkward.

Rewriting the IS.

This needs a ton of comments and a longer write-up than I have time for right now. Could probably use some more tests too.

After talking to @effulgentsia more, we've temporarily abandoned the form_build_key approach, and are first just handling the _form and _entity_form based forms.

No interdiff as this is essentially a new patch.

All forms that are the entirety of their response (aka those from _form or _entity_form) are run through \Drupal\Core\Controller\FormController, and $form_state->setPostToOriginalUrl() is run to mark this to be posted to the original URL.

During form building, \Drupal\Core\Render\Element\RenderElement::processAjaxForm() will call $form_state->setPostToOriginalUrl(FALSE) if an element specifies it's own custom URL (like file.ajax_progress for file uploads).

@TODO What happens if one element needs to be posted through, but the other doesn't? Maybe postpone this until $form_state->getTriggeringElement() is checked?

After that, \Drupal\Core\Render\Element\RenderElement::processAjaxForm() will set the $element['#ajax']['url'] to Url::fromRoute('<current>'), and set a query parameter for drupal_ajax_post

Once the #ajax element is triggered, a new request begins, posted to that URL with the query parameter.

When choosing the correct controller (during KernelEvents::CONTROLLER), \Drupal\Core\EventSubscriber\FormControllerSubscriber::onKernelController() inspects the query string for drupal_ajax_post. It then wraps the original controller in a closure.
That closure takes the resulting $form and $form_state, it passes it off to \Drupal\Core\Form\FormAjaxHandler::handle(), which calls the AJAX callable (formerly done by /system/ajax

In order for KernelEvents::CONTROLLER to have access to both the $form *and* $form_state, \Drupal\Core\Controller\FormController::getContentResult() cannot just return the render array. It now returns a value object, creatively named FormAndFormState, which contains the $form and $form_state.

In order to turn that into a render array later, we have \Drupal\Core\EventSubscriber\FormControllerSubscriber::onViewRenderArray().

And that's it!

The patch does not handle any forms that are built directly (like in _controller pages or in a block).
Nor does it handle #ajax elements that specify their own URL like file uploads.

I don't know how much time I'll have this weekend (and Monday is a holiday here), so unassigning myself for now.

A bit of cleanup and a rudimentary test for FormControllerSubscriber. Still haven't fixed those fails. Or gone to bed.

+1 to #94 / #96:

That looks like a very promising approach!

1. +++ b/core/lib/Drupal/Core/EventSubscriber/FormControllerSubscriber.php
   @@ -0,0 +1,114 @@
   + * @todo.
   

   For sure we should document why we do things ...

2. +++ b/core/lib/Drupal/Core/EventSubscriber/FormControllerSubscriber.php
   @@ -0,0 +1,114 @@
   +        $response = call_user_func_array($original_controller, $arguments);
   ...
   +        /** @var \Drupal\Core\Form\FormAndFormState $response */
   +        $form = $response->getForm();
   

   Should we check for an instanceof and otherwise throw some exception?

3. +++ b/core/lib/Drupal/Core/EventSubscriber/FormControllerSubscriber.php
   @@ -0,0 +1,114 @@
   +        $form_build_id = $request->request->get('form_build_id');
   

   Is that the right behaviour for GET forms? Maybe you should $request->get('form_build_id') instead?

4. +++ b/core/lib/Drupal/Core/Form/FormAjaxHandler.php
   @@ -0,0 +1,82 @@
   +class FormAjaxHandler implements FormAjaxHandlerInterface {
   

   What about renaming it at least to FormAjaxResponseHandler? The current name is sort of not optimal.

5. +++ b/core/lib/Drupal/Core/Form/FormAjaxHandler.php
   @@ -0,0 +1,82 @@
   +      throw new HttpException(500, 'The specified #ajax callback is empty or not callable.');
   

   Afaik you can throw a random exception, which then will be converted to a 500 anyway.

6. +++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
   @@ -127,8 +128,17 @@ public static function preRenderGroup($element) {
   +        $element['#ajax']['url'] = Url::fromRoute('<current>');
   +        $element['#ajax']['options']['query'][FormControllerSubscriber::MAGIC_STRING_TO_BE_RENAMED] = 'drupal_ajax_post';
   

   We could put the options directly onto the Url object here

7. +++ b/core/modules/system/src/Controller/FormAjaxController.php
   @@ -121,38 +95,7 @@ public function content(Request $request) {
        $this->formBuilder->processForm($form['#form_id'], $form, $form_state);
    
   -    // We need to return the part of the form (or some other content) that needs
   -    // to be re-rendered so the browser can update the page with changed content.
   ...
   +    return $this->formAjaxHandler->handle($request, $form, $form_state, $commands);
   

   <3

8. +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/FormControllerSubscriberTest.php
   @@ -0,0 +1,97 @@
   +    $form_ajax_handler = $this->getMock('Drupal\Core\Form\FormAjaxHandlerInterface');
   +    $form_ajax_handler->expects($this->once())
   +      ->method('handle')
   +      ->with($request, $form, $form_state, $commands)
   +      ->willReturn('the ajax result');
   +    $controller_resolver = $this->getMock('Drupal\Core\Controller\ControllerResolverInterface');
   +    $controller_resolver->expects($this->once())
   +      ->method('getArguments')
   +      ->willReturn([$form, $form_state]);
   

   Quick hint, try out prophecy :)

Part review

1. +++ b/core/lib/Drupal/Core/EventSubscriber/FormControllerSubscriber.php
   @@ -0,0 +1,114 @@
   +    if ($request->query->get(static::MAGIC_STRING_TO_BE_RENAMED) === 'drupal_ajax_post') {
   
   +++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
   @@ -127,8 +128,17 @@ public static function preRenderGroup($element) {
   +        $element['#ajax']['options']['query'][FormControllerSubscriber::MAGIC_STRING_TO_BE_RENAMED] = 'drupal_ajax_post';
   

   Can we use a constant for this instead

2. +++ b/core/lib/Drupal/Core/EventSubscriber/FormControllerSubscriber.php
   @@ -0,0 +1,114 @@
   +        if ($form_build_id != $form['#build_id']) {
   

   I think we should be using !== here

3. +++ b/core/lib/Drupal/Core/Form/FormAjaxHandler.php
   @@ -0,0 +1,82 @@
   +    // to be re-rendered so the browser can update the page with changed content.
   

   nit > 80 (just)

4. +++ b/core/lib/Drupal/Core/Form/FormAjaxHandler.php
   @@ -0,0 +1,82 @@
   +      $callback = $triggering_element['#ajax']['callback'];
   

   Should we be checking isset($triggering_element['#ajax']['callback'])?

5. +++ b/core/lib/Drupal/Core/Form/FormAjaxHandler.php
   @@ -0,0 +1,82 @@
   +    // we got earlier; typically the UpdateBuildIdCommand when handling an AJAX
   

   %s/got/received ?

6. +++ b/core/lib/Drupal/Core/Form/FormAjaxHandlerInterface.php
   @@ -0,0 +1,31 @@
   +  public function handle(Request $request, array $form, FormStateInterface $form_state, array $commands);
   

   Could we use Drupal\system\FileAjaxForm here and trim the arguments to just the request and the that?

7. +++ b/core/lib/Drupal/Core/Form/FormAndFormState.php
   @@ -0,0 +1,60 @@
   +class FormAndFormState {
   

   Could we re-purpose Drupal\system\FileAjaxForm here? Very similar problem-space. Would need a more generic name.

+++ b/core/modules/system/src/FileAjaxForm.php
@@ -14,21 +15,7 @@
+class FileAjaxForm extends FormAndFormState {

Ah, didn't get this far in last review... so we are using this, in which case we should definitely move it out of the Drupal\system namespace

#99
1) I'll get there
2) Done (and added test coverage)
3) Done (and added test coverage)
4) Done
5) This is from HEAD
6) Done

#100
1) @TODO
2) Done
3) Done
4) Done (and added test coverage)
5) This is from HEAD
6) I'm not sure about doing this. Undone for now

FileAjaxForm is still used only in \Drupal\system, so leaving it there for now.
Not sure if having it separate (subclassed) from FormAndFormState is a good or bad thing.

Silly mistake on that unit test.

I think the remaining failures may be asserting the wrong thing now. Because we're not caching the form anymore, the form build ID *is* going to be different. Should I just remove those assertions? Seems like a question for @effulgentsia.

#92: wow suddenly, magically from 87 to 2 failures! Awesome!

#94: What will it take to handle _controller or block forms? Or are you suggesting that we don't need to support those? I'm concerned that this approach may only work for some forms, whereas we need it to work for all forms. Or don't we?

We went from 87 to 2 solely because I removed everything that affected _controller and block forms.

IMO only _form and _entity_form can post back to their original URL, because they're the only ones where the entire response is a form.

I think @effulgentsia's idea for the other forms was just to modify the current usage of setCache() to mimic the idea of form_build_key from earlier, without introducing a separate concept.
That is, build the cache ID from the serialized form and form_state, which will cause old cache entries to be overwritten instead of adding new entries always.

IMO only _form and _entity_form can post back to their original URL, because they're the only ones where the entire response is a form.

Mh, so this doesn't solve the problematic cases like admin/content?

admin/content is #2486433: Make ViewsForm stop marking itself as needing to be cached

The real problematic case here is file uploads, \Drupal\file\Controller\FileWidgetAjaxController::upload() depends on the form being cached.

#105: There is two possibilities we could do once _form and _entity_form work correctly to make that work in a similar way:

a) We could just call the page normally and such take a performance hit, but the ajax response would be correct.
b) We could possibly rely on placeholdering all forms:

- If forms were creating a render array with #pre_render_cache and the information how to re-create it (using the Request and what not), then we would end up with a placeholder for each form on the page. In which case we could short-circuit a) if the page / block was already cached as we then have the information how to access the Form available directly.

However lets get _form and entity_form working first, then we can explore further.

I think forms in blocks would be the other major use-case anyway.
Forms in _controller => Well, then you are on your own ...

The real problematic case here is file uploads, \Drupal\file\Controller\FileWidgetAjaxController::upload() depends on the form being cached.

Do you see a way to make that work differently, so that it no longer depends on the form being cached?

#111: File Ajax uploads btw. also had huge problems with Cacheable CSRF, however when analyzing what needed to be done, we found that simply the same logic we used for the default system/ajax path / controller would need to apply to that one as well.

This means a slightly more complicated path, but its do-able.

- Post to original url
- Do routing normally
- Skip to system/ajax controller normally
- Rebuild the form usually (if we POSTed before, we have a state already, which is fine - so file uploads are saved correctly)
- Now when we know the element and know it has a different path, we actually call the FileAjaxController (?)
- FileAjaxController does what it needs to do to upload the file and then just does the callback
- Returns to system/ajax controller, which does the rest

So we need to wrap the FileAjaxController inside the system/ajax controller kinda.

Cleaning up all of the docs, naming, and @todos.
Included a separate interdiff for the test changes.

Going to hopefully walk through this with @effulgentsia later today.

Assuming this passes, I think it might be worthwhile to get this in as is, and continue work on _controller/block forms (probably following #113) in a new issue.

However, I don't mind continuing on with this issue, I leave that decision to someone else.

1. +++ b/core/lib/Drupal/Core/Controller/FormController.php
   @@ -76,7 +79,16 @@ public function getContentResult(Request $request, RouteMatchInterface $route_ma
   +    // If this form is being submitted back to its original URL, disable all
   +    // redirects.
   +    if ($request->query->get(FormControllerSubscriber::POST_TO_ORIGINAL_URL) === FormControllerSubscriber::DRUPAL_AJAX_POST) {
   +      $form_state->disableRedirect();
   +    }
   

   This could be moved to \Drupal\Core\EventSubscriber\FormControllerSubscriber::onViewFormAndFormState() I think?

2. +++ b/core/lib/Drupal/Core/EventSubscriber/FormControllerSubscriber.php
   @@ -0,0 +1,125 @@
   +    if ($request->query->get(static::POST_TO_ORIGINAL_URL) === static::DRUPAL_AJAX_POST) {
   

   Do we need both of these constants? Should we just check for static::POST_TO_ORIGINAL_URL === TRUE? Or will we need separate values for _controller later?
   This just seems overly verbose.

3. +++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
   @@ -127,8 +129,16 @@ public static function preRenderGroup($element) {
   +        $form_state->setPostToOriginalUrl(FALSE);
   

   Still worried about this.

#115.1 is too late, it has to be done here.

Still need answers on 2 and 3.

The first green patch! Go Tim!

Great work here!

#115:

2. My reasoning originally was that the magic parameter is like a 'format' chosen for the route. If we go with query parameter based formats, I think this is just a special ajax request like drupal_dialog, etc.

So I would have used ?_format=drupal_system_ajax (or whatever drupal_dialog now uses - I forgot!)

that would then be consistent with the rest of core - as far as I understand it.

Not sure however if that is in anyway conflicting with the rest of content negotiation via query parameters and if dialogs are more special here.

However if using the same as drupal_dialog is not feasible, then I think something like ?_ajax_format=system_ajax would work well for now. It being constants we can change it later anyway.

3. Could you elaborate some more, please, why you are concerned about this? I was not able to find the original comment, where you discussed that. Thanks!

From #94:

During form building, \Drupal\Core\Render\Element\RenderElement::processAjaxForm() will call $form_state->setPostToOriginalUrl(FALSE) if an element specifies it's own custom URL (like file.ajax_progress for file uploads).

@TODO What happens if one element needs to be posted through, but the other doesn't?

So what happens if a form has an #ajax-ified element that is supposed to post back to the original URL, but also a file upload field?

Apply #117, navigate to admin/content and then reload the page a couple of times. This still generates one form and one form state entry on every request.

What is the status of this patch? What are the next steps, i.e., what is the plan to resolve the original issue?

Can we verify if this helps solving #2465053: Drupal 8 only allows one user every 6 hours to register when page caching is enabled — caused by entity UUID in form state, by removing the no_cache flag from the user/register route?

Does someone mind answering the question of damiankloip in #112?

Patch in #117 is not affected by #112, because we always rebuild the form. #112 discussed the other approach that Wim and Alex had been working on.

I assume #121 fails manual testing, because views is not using a native form controller route, but has it _somewhere_ on the page. This however is handled by #2486433: Make ViewsForm stop marking itself as needing to be cached as views does not need AJAX POST for exposed forms, but more stateless AJAX GET - which is a quite special case.

This is a three-step issue:

a) _form and _entity_form => Verify and Fix (#117 is WIP) - This is useful independent of the rest.
b) ajax file uploads for e.g. _entity_form (Follow-up 1)
c) _controller and _block forms (Follow-up 2)

By splitting it up this way, this issue is kept actionable.

===

#119 / #120:

2. Thought about it again, I think a simple _ajax_request=1 is enough.

3. How do we manage right now with ajax field? I don't think I have overall seen many contrib/ modules except core file uploads using a different ajax path than system/ajax.

So I would say that it is a necessity that file ajax uploads also go via the original path - as there is no form to work on.

Also: Why _do_ they have their own path anyway? Is there some historical background?

===

Idea:

Similar to how #pre_render_cache enables placeholders, we could enable FormControllers to specify an Interface method how to (re-)build themselves.

(onAjax()? afterAjax()? ajaxHelper()? ...)

If a FormController (which we should know) has such a method, we can use Wim' and effulgentsia's approach to store one entry for that form that just contains the controller and method.

Then we can do this even for forms without _form and _entity_form in a more efficient way.

- If you forget to specify that method (and hence your dependencies, ajax is slow, but correct).
- If you put it, we can optimize the path.

e,g, the logic in onController() then would be like:

- Is this original controller a _form and _entity_form thingy? Yes => Fast-Path (already kinda done)
- Has the _ajax_request key a form state ajax rebuild id? Check K/V, If Yes, set controller => (re)build function() => Fast-Path
- Else => Slow path (execute whole page)

That also answers 2: The magic key should remain, but the magic value should be '1' for _form and _entity_form fast paths and '[hash]' for other fast-paths.

This would address Follow-up 2 from my list above and it might not even be that difficult:

$magic_key = 1;
If ($form_controller instanceof AjaxFastPathCallMeSomethingFormController) {
  $magic_key = $this->ensureKeyValue($form_controller->getControllerName()); // However that works ...
}

TL;DR:

- AJAX requests go the slow path of a whole page request always for sanity reasons.
- We optimize the fast-path for _form and _entity_form
- We optimize the fast-path if and only if the form controller specifies a method (to be named) that has all the dependencies and would work on its own path. ( The views issue could likely use this. )

That gives us the best out of both worlds and addresses #112 with a new interface.

Said otherwise:

We "cheat", we always use _form semantics, but once you specify it via a route and once you specify it with a special interface to inform us.

Analyzed file_ajax_upload some more (don't know the D8 equivalent, but I assume its similar).

All it (and any other callback) does is in essence:

- Check $_POST['form_build_id'] - we can control that
- Call ajax_get_form() with that form_build_id, which checks the cache / key/value for $form and $form_state.

So to wrap ajax file uploads and any other ajax controller we need two things:

- Rebuild the form (see previous comment), but set a cache entry this time
- Call the original _controller by resolving the path to the controller

e.g.

/node/1/edit?_ajax=1&_ajax_wrapper_path=file/ajax/upload

As it is an entity form we can fast-path, as we know the wrapper path, we can just rebuild the form, set the form_build_id into POST (request) and then call the original controller.

---

Unrelated to all of that we still need to get the 'slow-path' working after we know where the _form is on the page - likely an Exception. Or as ajax is a rather special case, we could also enforce that new interface to regain sanity (and _form and _entity_form have that explicitly).

Repeated the test with examples/dbtng_example/update from the Examples project. And indeed this patch prevents that a form/form-state pair is created on every request. The entries are only generated as soon as an Ajax request is submitted.

The form even works when the form_build_id hidden element is removed from the initial form (manually via browser developer tools). Therefore please let's suppress rendering of that element in that case.

3. How do we manage right now with ajax field? I don't think I have overall seen many contrib/ modules except core file uploads using a different ajax path than system/ajax.

Basically those which were affected by SA-CORE-2014-002. I found very few: #2242751: Ajax form page callback require updates for Drupal 7.27, #2243427: Update to Drupal 7.27 for SA-CORE-2014-002, #2242753: Ajax form page callback require updates for Drupal 7.27

Discussed the plan on the D8 Core (subsystem) maintainers call.

In a nut-shell, much easier - we don't even need to do onController:

Strategy:

- Make it correct (same execution-path as normal forms)
- Make it fast (fast-path from my comments above)

Approach:

- Only allow storing form + form_cache on POST - ever
- Use the same execution-path as normal form submissions

First:

- Normal form submissions already go via the full page rendering
- When reaching a form that we want to execute (form_id matches)
AND
- When the special MAGIC AJAX parameter is set

=>

throw new FormAjaxExcpetion() with the builded form.

Have a FormAjaxException subscriber, that does essentially what Tim does in his patches for _form and _entity_form right now already.

For BC reasons we could then even store the form_cache, set form build ID and then even file/ajax/upload would work by calling the original controller on that path.

Workflow in short:

- Render page
- Find the right form and build it with the parameters available there
- Abort rendering and throw FormAjaxException()

- Use the normal ajax controller with the now build form

Posting some progress here, switching to the exception-based flow. Still many bugs to work out.

This currently punts on the Views UI, all file uploads, and Field UI "add more" buttons. See @todos in patch.

Just posting this, was hoping to get more work done but am pretty well blocked by testbot.

#134 already looks really great!

Just some quick general feedback.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/FormAjaxSubscriber.php
   @@ -0,0 +1,81 @@
   +      if ($form_build_id !== $form['#build_id']) {
   +        // If the form build ID has changed, issue an Ajax command to update it.
   +        $commands[] = new UpdateBuildIdCommand($form_build_id, $form['#build_id']);
   +      }
   

   Is there a specific reason why this logic is in the subscriber and not the Update...Command?

2. +++ b/core/lib/Drupal/Core/Form/FormAjaxResponseHandlerInterface.php
   @@ -0,0 +1,34 @@
   +interface FormAjaxResponseHandlerInterface {
   

   What about naming it FormAjaxResponseBuilderInterface, given that this is what this interface is used for?

3. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -2622,7 +2624,8 @@ protected function prepareRequestForGenerator($clean_urls = TRUE, $override_serv
      protected function buildUrl($path, array $options = array()) {
   ...
   -      return $path->setAbsolute()->toString();
   +      $absolute_path = clone $path;
   +      return $absolute_path->setAbsolute()->toString();
   

   You might run into the bug that we don't apply the $options on here.

1)
The other user of FormAjaxResponseBuilderInterface is \Drupal\system\Controller\FormAjaxController::getForm(), which uses $form['#build_id_old'] instead of $request->request->get('form_build_id') to find the old form build ID.
So I'm going to punt on that (though it does seem like a nice idea)

2)
Agreed, done

3) Fixed. This was fixing a situation like this:

$url = Url::fromRoute('someroute');
$this->drupalGet($url);
$this->assertSame('somepath', $url->toString());

Which would then fail because 'somepath' !== 'http://whatever/somepath' (since drupalGet()/buildUrl() were calling ->setAbsolute() on the object.
The clone fixes that.

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -2625,7 +2625,10 @@ protected function prepareRequestForGenerator($clean_urls = TRUE, $override_serv
+      return $absolute_path
+        ->setOptions($options)

You better merge the options ... this was afaik needed when I worked on #2481453: Implement query parameter based content negotiation as alternative to extensions

I'm just going to remove that hunk entirely and split it out, I don't think it's 100% necessary for this issue any more.

Going to work on fixing add_more, will open follow-ups for file upload and the Views UI.

#2500527: Rewrite \Drupal\file\Controller\FileWidgetAjaxController::upload() to not rely on form cache
#2500523: Rewrite views_ui_add_ajax_trigger() to not rely on /system/ajax.

Okay, solved WidgetBase (it wasn't any problem with the callback, just the tests themselves).
All of the remaining @todos link to issues (the ones from the last comment).

Also took care of #139.1

I think this is ready for review.

Just skimmed the patch and it looks great to me!

Nice work on providing (temporary) BC for the more tricky cases :).

Thanks!

One silly mistake, the other just a brittle test (the ordering of AJAX commands is shifted now by the UpdateBuildIdCommand).

1. +++ b/core/lib/Drupal/Core/Form/FormAjaxException.php
   @@ -0,0 +1,69 @@
   +   * The form to cache.
   

   This should say "The form definition."

2. +++ b/core/lib/Drupal/Core/Form/FormAjaxException.php
   @@ -0,0 +1,69 @@
   +   *   (optional) The previous exception for nested exceptions
   

   This is missing a trailing period.

Going to leave those alone for now, waiting on reviews now.

Just selfishly fixed the nits

The patch looks great!

Here are first two general questions:

• Do we have a good documentation to explain the general flow of code, that is happening?
• Can we add test coverage, which ensures, that the key value store is not filled up?

1. +++ b/core/lib/Drupal/Core/EventSubscriber/FormAjaxSubscriber.php
   @@ -0,0 +1,77 @@
   +namespace Drupal\Core\EventSubscriber;
   ...
   +class FormAjaxSubscriber implements EventSubscriberInterface {
   

   What do you think about moving the class to Drupal\Core\Form\EventSubscriber ? I mean all this logic belongs central to the way how forms actually work

2. +++ b/core/lib/Drupal/Core/EventSubscriber/FormAjaxSubscriber.php
   @@ -0,0 +1,77 @@
   +/**
   + * Wraps controllers that return forms for their response.
   + */
   

   That doc is c&pasted :)

3. +++ b/core/lib/Drupal/Core/EventSubscriber/FormAjaxSubscriber.php
   @@ -0,0 +1,77 @@
   +  /**
   +   * Request key for a form that will post to original URL on AJAX submission.
   +   */
   +  const POST_TO_ORIGINAL_URL = 'post_to_original_url';
   

   Maybe a bad question. Can't we add this constant to the FormAjaxResponseBuilderInterface?

4. +++ b/core/lib/Drupal/Core/Form/FormAjaxResponseBuilder.php
   @@ -0,0 +1,95 @@
   +      throw new HttpException(500, 'The specified #ajax callback is empty or not callable.');
   

   General question about the flow of code. Now this throws an exception as part of the exception handling workflow. Is that a problem? If I understand it right, we would basically end up with the index.php/DrupalKernel catch statement. What about try catch in the FormAjaxSubscriber and update $event->exception?

5. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -244,6 +245,12 @@ public function buildForm($form_id, FormStateInterface &$form_state) {
   +    // If this form should post to the original URL, disable all form redirects.
   

   Do you mind adding some docs about why?

0)
We should expand the docs and the test coverage

1)
Sure!

2)
Fixed

3)
Here are the only (non-test) classes that use the constant:
lib/Drupal/Core/EventSubscriber/FormAjaxSubscriber.php
lib/Drupal/Core/Form/FormBuilder.php
lib/Drupal/Core/Render/Element/RenderElement.php
I agree putting it on the subscriber is weird, but putting it on FormAjaxResponseBuilderInterface seems even weirder. Any one else have an opinion?

4)
I'm not sure I understand. First of all, this exception was already thrown in HEAD, it is just moved. Yes, it is called more. But with KernelEvent::EXCEPTION, if you set a response, it will act as it's own catch statement. This is from HttpKernel, before that index.php/DrupalKernel code. If you read the docs for \Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent, it will hopefully be more clear.

5) I can try to add more docs tomorrow, but this is also borrowed from FormAjaxController

I'm not sure I understand. First of all, this exception was already thrown in HEAD, it is just moved.

Sure I know its moved.

5) I can try to add more docs tomorrow, but this is also borrowed from FormAjaxController

Well, documenting somewhere about the actual flow which is happening would be nice. Feel free to ignore it.

I'm not sure I understand. First of all, this exception was already thrown in HEAD, it is just moved. Yes, it is called more. But with KernelEvent::EXCEPTION, if you set a response, it will act as it's own catch statement. This is from HttpKernel, before that index.php/DrupalKernel code. If you read the docs for \Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent, it will hopefully be more clear.

Yeah, so this is not about the case where things actually work, but in case the exception is thrown.

Well, so in HEAD, the 500 throw new HttpException(500, 'The specified #ajax callback is empty or not callable.'); is thrown,
catched by the exception bit in HTTPKernel and then things are rendered properly.

With your patch, the 500 is thrown (in \Drupal\Core\Form\FormAjaxResponseBuilderInterface::buildResponse). Then the exception propagates
to \Drupal\Core\Form\EventSubscriber\FormAjaxSubscriber::onException, then to \Symfony\Component\HttpKernel\HttpKernel::handleException
, nothing in catching there, so it propagates to DrupalKernel::handle(). As written before, its fine to set this exception onto the exception event and let other exception handlers handle it.

Okay I moved the constant to FormBuilderInterface.
I expanded the docs in FormBuilder.
I wrote a web test that tracks the form cache entries.
I finally figured out what @dawehner meant and better handled the exception in the subscriber.

1. +++ b/core/lib/Drupal/Core/Form/EventSubscriber/FormAjaxSubscriber.php
   @@ -56,11 +52,18 @@ public function onException(GetResponseForExceptionEvent $event) {
   +      catch (HttpException $e) {
   

   Any reason to not use any exception?

2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -265,8 +264,12 @@ public function buildForm($form_id, FormStateInterface &$form_state) {
   +    // interrupt form rendering and return by throwing an exception that
   +    // contains the processed form and form state. This exception will be caught
   +    // by \Drupal\Core\Form\EventSubscriber\FormAjaxSubscriber::onException()
   +    // and then passed through
   +    // \Drupal\Core\Form\FormAjaxResponseBuilderInterface::buildResponse() to
   +    // build a proper AJAX response.
   

   +1

Good point, the call_user_func_array() on the callback in \Drupal\Core\Form\FormAjaxResponseBuilderInterface::buildResponse() could theoretically thrown many kinds of exceptions.

Adjusting credit, per Tim, to make sure eff, dawehner, and Fabianx get in there.

GIven that we have an API change/addition we should add a change record

Added https://www.drupal.org/node/2501435

Just because Bad religion is playing in the TV ;)

Adding suggested commit message to the IS, and retitling a bit

I'd like to have a look at this before it goes in - although likely full review will be Monday morning.

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -257,6 +263,17 @@ public function buildForm($form_id, FormStateInterface &$form_state) {
+    // After processing the form, if this is to be posted to the original URL,
+    // interrupt form rendering and return by throwing an exception that
+    // contains the processed form and form state. This exception will be caught
+    // by \Drupal\Core\Form\EventSubscriber\FormAjaxSubscriber::onException()
+    // and then passed through
+    // \Drupal\Core\Form\FormAjaxResponseBuilderInterface::buildResponse() to
+    // build a proper AJAX response.
+    if ($post_to_original_url) {
+      throw new FormAjaxException($form, $form_state);
+    }
+

Throwing an exception here feels really weird and counter-intuitive in my world, raising my eyebrows to a relatively high level in my face. Maybe I'm just not ready for it, that's possible. I don't have a counter proposal, but it feels like a) the wrong place to interrupt and b) a wrong usage of exceptions here.

This FormBuilderInterface::POST_TO_ORIGINAL_URL usage still seems slightly disturbing to me. This is basically a flag that this is an AJAX submission right? Lots of forms can post to the same url without anything to do with ajax, so it seems weird.

I think the idea though here is that eventually we make posting to the original URL the exception (pun not intended) and 'normal' POST forms also go to a dedicated URL - then we'd no longer need to render the form to process it, and could dramatically improve the performance of regular form submissions (especially forms in blocks like the user login form). If we end up using the same mechanism for those then I think it's fine to make it not-specific to AJAX now, but would be good to define the interim behaviour if it's set.

1. +++ b/core/lib/Drupal/Core/Form/FormAjaxResponseBuilderInterface.php
   @@ -0,0 +1,37 @@
   +   * @throws \Symfony\Component\HttpKernel\Exception\HttpException
   +   *   Thrown if the AJAX callback is not a callable.
   

   Minor: Not sure whether HTTPException fits here. Isn't it more some LogicException or so if the callback is wrong? It's not a regular error case, but a developer error.

2. +++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
   @@ -13,6 +13,11 @@
   +  const POST_TO_ORIGINAL_URL = 'post_to_original_url';
   +
   

   That flag does not make so much sense to me when thinking about the request. I'd rather add a ajax=1 error paramter - that tells an average developer more about what's going on.

3. +++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
   @@ -69,6 +74,14 @@ public function getForm($form_arg);
   +   * @throws \Drupal\Core\Form\FormAjaxException
   +   *   Thrown when a form is triggered via an AJAX submission. It will be
   +   *   handled by \Drupal\Core\Form\EventSubscriber\FormAjaxSubscriber.
   +   * @throws \Drupal\Core\Form\EnforcedResponseException
   +   *   Thrown when a form builder returns an exception directly, usually a
   +   *   \Symfony\Component\HttpFoundation\RedirectResponse. It will be handled by
   

   Reading that, I think the ajax exception should be an enforce repsonse exception also? To me, redirect + ajax response are just two cases of the enforced response feature.

4. +++ b/core/modules/file/src/Controller/FileWidgetAjaxController.php
   @@ -21,6 +26,42 @@
   +    parent::__construct($logger, $form_builder, $form_ajax_response_builder);
   +    $this->renderer = $renderer;
   

   Wondering whether the added renderer is used somewhere?

ad change record:
>AJAX forms are no longer cached by default

It tells how to opt-in into caching again - but as it creates troubles with page-caching like #2465053: Drupal 8 only allows one user every 6 hours to register when page caching is enabled — caused by entity UUID in form state it might be the better to choice to stop caching forms during the initial rendering *at all*. Maybe we should re-phrase it at least do suggest *not* doing it? If there is no reason any more to have that feature, I'd vote for dropping it as a whole as it does not play well with render caching.

I think the 'post_to_original_url' topic could need some clarification, thus tentatively setting back to needs review for that.

Oh I forgot, I'm +1 on the general approach here. It would be good to have some summary of the plans regarding fast path etc. though.

Then, although no API change having ajax request not going to system/ajax any more is a change in behaviour that's of interest to a lot of developers imo. Thus, I think that would deserve its own change record also.

#166
We already throw an exception at that same location for other reasons, look below it.

#167
I am not attached to the naming. Give me a concrete suggestion for the constant's name and string value, and I'll change it.

#169
1) That exception is not new or my choice, it's just being moved from the middle of a controller to a service.
2) See my response to #167
3) I disagree, we need our own class to trigger our own processing
4) Absolutely. Why else would I include that? We're just changing the parent constructor to *not* need it.

#170
I think the CR is fine as is, feel free to expand it or add a secondary one.

> 3) I disagree, we need our own class to trigger our own processing
Yes, we need our own class but could it extend from EnforcedResponseException or does that let wrong exception handling to kick in? Logically, I'd say it should extend it's some sort of enforced response also.

I am not attached to the naming. Give me a concrete suggestion for the constant's name and string value, and I'll change it.

What about AJAX_FORM_REQUEST and "ajax_form" ?

Why do we need that constant / query parameter at all? It's only for POST requests, so why not use Accept header negotiation?

#172:

EnforcedResponseException causes EnforcedFormResponseSubscriber to trigger, and also would require us to use EnforcedResponse itself.
We'd have to encapsulate all of the logic currently in FormAjaxSubscriber either in the exception class itself, or in a response itself, both of which are less ideal.

We could completely rewrite how EnforcedResponse and friends work to fit into that paradigm, but I don't think it will give us any tangible gain, and might break even more APIs.

Renamed the constant.

#173:
Isn't that the opposite of #2481453: Implement query parameter based content negotiation as alternative to extensions?

We set the query param in \Drupal\Core\Render\Element\RenderElement::preRenderAjaxForm()
We check for it in \Drupal\Core\Form\FormBuilder::buildForm()
Not sure that we can rely on accept headers there.

I don't think it will give us any tangible gain, and might break even more APIs.

I see, thanks for the explanation.

Why do we need that constant / query parameter at all? It's only for POST requests, so why not use Accept header negotiation?

True, that seems very reasonable. The problem of #2481453: Implement query parameter based content negotiation as alternative to extensions is with caches for GET requests, but as POSTs generally are uncachable this is not an issue here.

The problem of #2481453: Implement query parameter based content negotiation as alternative to extensions is with caches for GET requests, but as POSTs generally are uncachable this is not an issue here.

Yes, but looking at the patch over there, it removes the drupal_ajax format entirely, even for POST requests. But, HEAD's ajax.js is already sending _wrapper_format=drupal_ajax so maybe we should use that?

OTOH, this patch's logic isn't about wrapping; it's about changing the code flow to return something significantly different (e.g., just one form element instead of the entire form or block).

On #177 for wrapped responses we also don't show regions etc. normally, so not sure it's a complete mis-use of that to show only a page fragment.

Two very small nits, overall I like the approach here, and while I don't love exceptions for code flow, this patches the 404/403 exceptions pretty well - so at least it's more of the same thing rather than something new.

1. +++ b/core/lib/Drupal/Core/Form/FormAjaxResponseBuilder.php
   @@ -0,0 +1,95 @@
   +    if (($triggering_element = $form_state->getTriggeringElement()) && isset($triggering_element['#ajax']['callback'])) {
   

   Nit but the extra parentheses don't do anything here.

2. +++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
   @@ -69,6 +74,14 @@ public function getForm($form_arg);
   +   *   handled by \Drupal\Core\Form\EventSubscriber\FormAjaxSubscriber.
   +   * @throws \Drupal\Core\Form\EnforcedResponseException
   +   *   Thrown when a form builder returns an exception directly, usually a
   +   *   \Symfony\Component\HttpFoundation\RedirectResponse. It will be handled by
   +   *   \Drupal\Core\EventSubscriber\EnforcedFormResponseSubscriber.
   

   RedirectResponse isn't an exception. Shouldn't this be "Thrown when a form builder returns a response directly"?

Nit but the extra parentheses don't do anything here.

I don't get why, but otherwise storm complains. This is what I know

Because we use $triggering_element later in the same line, we need the parens to protect the assignment.

Fixed my typo and rebased.

"0 fails, 0 exceptions" found Other SimpleTestBrowserTest.php 146 Drupal\simpletest\Tests\SimpleTestBrowserTest->testTestingThroughUI()
@alexpott says he's seen this before, and it seemed random

Gonna be bold and set back to RTBC.

Do we have a major / critical follow-up for the fast path, yet? Or are we gonna do this as part of the generic 'make form execution independent of the mainpage request' issue that I cannot find right now?

+++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
@@ -152,6 +153,7 @@ public static function processAjaxForm(&$element, FormStateInterface $form_state
+   *   - #ajax['cache_form']

The CR mentions this new key as well. But there's a followup issue I want to open about removing it. I'm not sure if we want to break APIs for contrib twice though (telling them they can make their forms work by using this, then later telling them they can't), so please hold off on commit until I post the other issue and a suggestion on what else we can do as an interim step.

This breaks AJAXified blocks in forms anyway, so it's not ready. I have a failing test locally, just trying to debug it right now, will post shortly either way.

A few problems here.
First, as @effulgentsia mentioned to me earlier, we can't let the AJAX request be wrapped as drupal_ajax, it must return HTML (I think?). That's fixed.

Second, in the context of rendering, \Twig_Template::displayWithErrorHandling() is catching ALL Exceptions and throwing it's own, preventing FormAjaxException from being caught by FormAjaxSubscriber. We can fix that though, mimicking \Drupal\Core\Form\EnforcedResponse::createFromException().

Finally, this only works if the AJAX block is the first block on the page.
That is unfixed, and I'll have to deal with that tomorrow.

Silly mistake.

Fixing DialogTest again. Signing off for now.

Filed #2502785: Remove support for $form_state->setCached() for GET requests. Feedback there welcome. Per #187, tomorrow I'll comment here on thoughts about how that should affect this patch.

A good fix for the twig problem is in my patch for:

#2407195: Move attachment processing to services and per-type response subclasses

See https://www.drupal.org/node/2407195#comment-9962395 12. for the code:

+++ b/core/themes/engines/twig/twig.engine
@@ -62,6 +62,17 @@ function twig_render_template($template_file, array $variables) {
+  catch (\Twig_Error_Runtime $e) {
+    // In case there is a previous exception, we just display the message and
+    // throw the original Exception so that the original function that fails is
+    // shown.
+    $previous_exception = $e->getPrevious();
+    if ($previous_exception) {
+      drupal_set_message($e->getMessage(), 'error');
+      throw $previous_exception;
+    }
+    throw $e;
+  }

We need this anyway to make our ErrorsTest pass, so adding it here with the justification of that redirect responses and other things _need_ to be passed through is a good one.

Unless it is already fixed and I just don't see how :).

#196 That would be the polite thing for our twig integration to do IMO, but my change in that interdiff handled it by always looping through $e->getPrevious():

diff --git a/core/lib/Drupal/Core/Form/EventSubscriber/FormAjaxSubscriber.php b/core/lib/Drupal/Core/Form/EventSubscriber/FormAjaxSubscriber.php
index aa033ba..054e262 100644
--- a/core/lib/Drupal/Core/Form/EventSubscriber/FormAjaxSubscriber.php
+++ b/core/lib/Drupal/Core/Form/EventSubscriber/FormAjaxSubscriber.php
@@ -42,8 +42,9 @@ public function __construct(FormAjaxResponseBuilderInterface $form_ajax_response
    *   The event to process.
    */
   public function onException(GetResponseForExceptionEvent $event) {
-    $exception = $event->getException();
-    if ($exception instanceof FormAjaxException) {
+    // Extract the form AJAX exception (it may have been passed to another
+    // exception before reaching here).
+    if ($exception = $this->getFormAjaxException($event->getException())) {
       $request = $event->getRequest();
       $form = $exception->getForm();
       $form_state = $exception->getFormState();
@@ -67,6 +68,28 @@ public function onException(GetResponseForExceptionEvent $event) {
   }
 
   /**
+   * Extracts a form AJAX exception.
+   *
+   * @param \Exception $e
+   *  A generic exception that might contain a form AJAX exception.
+   *
+   * @return \Drupal\Core\Form\FormAjaxException|null
+   *   Either the form AJAX exception, or NULL if none could be found.
+   */
+  protected function getFormAjaxException(\Exception $e) {
+    $exception = NULL;
+    while ($e) {
+      if (!$e instanceof FormAjaxException) {
+        $e = $e->getPrevious();
+      }
+
+      $exception = $e;
+      break;
+    }
+    return $exception;
+  }
+
+  /**
    * {@inheritdoc}
    */
   public static function getSubscribedEvents() {

I think we should do both. Maybe even split your twig_render_template() fix out to another issue?

Working again on the block fail.

#197 Ah, I get it now.

Yes, I think the more important part however is that we need an Interface for Exception that we _do_ not want to display an error message for.

e.g. while the unwrapping works and is the correct thing to do, it would display a message that there was an Ajax Form Exception thrown, which is not that helpful ...

But yes, follow-up.

Okay, thanks to @effulgentsia I figured out how to make this work.

#766146: Support multiple forms with same $form_id on the same page is still broken, but that's broken in HEAD too.

Per @effulgentsia's request, I'm going to see if I can revert the introduction of #ajax][cache_form.

#198, this approach will not display anything, because that exception is being caught.

Okay, this just checks for the existence of #ajax][url again. Not deleting that draft change record yet, but we probably won't need it.

Comment #200!

I think we might be done here.

Yes, indeed, RTBC.

Opening the fast-path issue now.