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

Critical per #2465053-65: Drupal 8 only allows one user every 6 hours to register when page caching is enabled — caused by entity UUID in form state, unless we decide on a different solution for that issue.

As a side note, FileWidgetAjaxController is actually fairly misnamed, since it's not related to file field widgets, but rather to the lower-level ManagedFile form element type.

Talked with @effulgentsia about that issue

Currently \Drupal\file\Controller\FileWidgetAjaxController::upload has a couple of additional properties on top of submitting the form:

• Given that the file upload limiit might exceed the $_POST limit or be near it, the actual rest of the form submitted data might be skipped so we need some protected against non valid submissions:
  

      if (empty($request_form_build_id) || $form_build_id !== $request_form_build_id) {
        // Invalid request.
        drupal_set_message(t('An unrecoverable error occurred. The uploaded file likely exceeded the maximum file size (@size) that this server supports.', array('@size' => format_size(file_upload_max_size()))), 'error');
        $response = new AjaxResponse();
        $status_messages = array('#type' => 'status_messages');
        return $response->addCommand(new ReplaceCommand(NULL, $this->renderer->renderRoot($status_messages)));
      }

• Figure out the amount of files, so we can show some information that a new file got uploaded. This could be solved by ManagedFile tracking the amount of files before and after

Applied for a grant to try to get this issue running

Figure out the amount of files, so we can show some information that a new file got uploaded. This could be solved by ManagedFile tracking the amount of files before and after

I'm very possibly wrong, or simply irrelevant, but : last time I looked, I had the impression that ManagedFile was about single file uploads, while FileWidget added the extra logic for handling multiple-valued fields ?

I'm very possibly wrong, or simply irrelevant, but : last time I looked, I had the impression that ManagedFile was about single file uploads, while FileWidget added the extra logic for handling multiple-valued fields ?

Ehm yes, I was just trying to make the point what its doing on top of processing the form

• Converted the custom controller to a #ajax][callback
• Kept the existing logic regarding if (empty($request_form_build_id) || $form_build_id !== $request_form_build_id) {
  I think if we want to change the way how this works, its not critical, compared to the rest of the issue
• Ensured that ajax-new-content is still set.

Note: This patch has #2263569: Bypass form caching by default for forms using #ajax. applied.

The failure in UserPasswordTest is easy, we should respect current query parameters when we construct the #ajax URL options, so that user_pass_reset can be carried over.

The second fail is more tricky.

So we have a couple of upload/remove buttons and so ajax settings attached to it.
The test first adds a couple of new files and then tries to remove them.
During that time ideally all the ajax settings for all the buttons should always point to the newly generated build IDs.
With that patch they though don't because we just render a particular subset, and so we just get the updates for the fields which are re-renderer. In the debugger its looks like that

(blue are the changed ones)

Just rerolling

Love the test coverage this patch adds - swoon.
A few comments:

1. +++ b/core/misc/ajax.js
   @@ -421,7 +407,8 @@
   +    // @todo Passing both always is kinda redicilous!
   

   Ridculous? :-)

2. +++ b/core/modules/file/src/Element/ManagedFile.php
   @@ -124,6 +128,63 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
   +   * This ajax callback takes care about ensuring that there is no broken
   +   * request due to too big files, as well as adding a class to the response
   +   * that a new file got uploaded.
   +   *
   

   The language here is a bit disjointed

3. +++ b/core/modules/file/src/Element/ManagedFile.php
   @@ -124,6 +128,63 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
   +  public static function uploadAjaxCallback(&$form, FormStateInterface &$form_state, Request $request) {
   
   @@ -145,7 +206,7 @@ public static function processManagedFile(&$element, FormStateInterface $form_st
   +      'callback' => [get_called_class(), 'uploadAjaxCallback'],
   

   Do ajax callbacks support service id notation like we do in other places? Would mean we could use DI instead of a static method

Ridculous? :-)

I'll leave this out :) Its not a problem of this patch ...

The language here is a bit disjointed

Tried to improve it

Do ajax callbacks support service id notation like we do in other places? Would mean we could use DI instead of a static method

There is the ::foo synatx but it relates to the form class itself, which is not the case for generic form elements.

For now this is just a reroll with a lot of debugging code in there.

Here is a report of my current state of debugging, days of fun:

Steps to reproduce the test failure with the patch:

• Add two file fields with infinite elements to a node type
• Upload 3 txt fields to the first field
• Upload 1 txt field to the second field
• Upload another one. Here the result is no longer what you expect it to be. See this screenshot: https://www.drupal.org/files/issues/Screen%20Shot%202015-06-16%20at%2015...
• Following the form submission process the next time, the $form_state just contains the previously uploaded file but no longer the new additional fileM
  and you end up with this odd behaviour. Why this happens is not clear yet.

PS: Other problems which exists and might become more problematic in the future:

• Duplicate ID on the page, ... we have twice the same ajax wrapper on the page, see first screenshot: https://www.drupal.org/files/issues/Screen%20Shot%202015-06-16%20at%2015..., see edit-field-field1--4-ajax-wrapper. This is also the case in HEAD so its just "random" that the browser selects the first one
• Ajax behaviours are bound to IDs, so once we generate random once, this though will be still an issue, because we use a manual #prefix, #suffix wrapper
• The query parameters for the ajax URL potential points to outdated form build IDs, so you need to ideally replace all existing ajax behaviours and use the proper form build IDs ...

The query parameters for the ajax URL potential points to outdated form build IDs, so you need to ideally replace all existing ajax behaviours and use the proper form build IDs ...

This was the actual problem ... @effulgentsia suggested to remove the checking for the build ID, and this fixed the problem.

First follow up:

One bit which is patch is doing is to remove some checking for truncated POST requests ... this adds it bad, but I'm not sure how to test it already,
because it basically requires a specific PHP configuration, with post_max_size === max_upload_size

Not sure whether a pure unit test with a faked up request object is a good protection ...

I think we should remove #22 from this issue's scope and do it in #2507019: Determine how much POST truncation protection is needed and add tests for it. I think for this issue, we can allow the regression of not getting that friendly message, and then restore that functionality properly in #2507019: Determine how much POST truncation protection is needed and add tests for it.

I think for this issue, we can allow the regression of not getting that friendly message

After some more digging, I changed my mind about that. With JS enabled, in HEAD, if you try uploading a file > post_max_size, you get a friendly validation error. With both #20 and #22, you get a silent failure. In the case of #20, because what ends up getting returned is HTML rather than JSON (because due to the empty $_POST, there's no form for which $form_state->isProcessingInput() returns true, so the code never flows to AJAX processing). In the case of #22, because nothing handles the BadRequestHttpException, so an empty {} is returned.

Here's a patch (interdiff relative to #20) that works, but it could use some cleanup. Testing this should just be a matter of enhancing FileManagedFileElementTest with a drupalPostAjaxForm() submission of a file larger than post_max_size.

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -264,6 +265,11 @@ public function buildForm($form_id, FormStateInterface &$form_state) {
+    if ($ajax_form_request && !isset($form_state->getUserInput()['form_id'])) {
+      throw new BadRequestHttpException(t('An unrecoverable error occurred. The uploaded file likely exceeded the maximum file size (@size) that this server supports.', array('@size' => format_size(file_upload_max_size()))));
+    }
...
+++ b/core/modules/file/src/Controller/FileWidgetAjaxController.php
@@ -21,74 +16,6 @@
-    if (empty($request_form_build_id) || $form_build_id !== $request_form_build_id) {
-      // Invalid request.
-      drupal_set_message(t('An unrecoverable error occurred. The uploaded file likely exceeded the maximum file size (@size) that this server supports.', array('@size' => format_size(file_upload_max_size()))), 'error');

A subtle difference here is that the code being removed from HEAD only runs when you use a file widget, whereas the code being added by patch is run whenever you have an $ajax_form_request with an empty $_POST. So the error message is an assumption. But not an entirely unreasonable one: in the real world, when do you ever exceed post_max_size without a file upload? Not sure how much we want to worry about the edge case of that not being true here vs. in #2507019: Determine how much POST truncation protection is needed and add tests for it.

It feels a little bit bad to use translatable exceptions but yeah I guess this this user facing text.

@catch, @effulgentsia, @webchick, @xjm and I discussed this issue and decided to keep it critical since it is part the work to stop setting form cache on get requests. See #2502785: Remove support for $form_state->setCached() for GET requests

ehem sorry this was a cross-upload of a patch ... so better ignore #28

We should at least ensure that we don't take over all BadHttpRequestExceptions

Here is some first start of a test coverage, ... the question is, what should we do with the non JS upload case ...

Let's fix the test coverage ... This is almost never a bad idea.

Let's use a better API function and at the same time, get some more debugging output in here ...

Patch looks awesome

--- a/core/modules/file/src/Element/ManagedFile.php
+++ b/core/modules/file/src/Element/ManagedFile.php

+ $current_file_count = $form_state->get('file_upload_delta_initial');
+ if (isset($form['#file_upload_delta']) && $current_file_count < $form['#file_upload_delta']) {
+ $form[$current_file_count]['#attributes']['class'][] = 'ajax-new-content';
+ }

I might very well be wrong (away from my coding env atm, sorry for the noise if that's the case), but isn't this adding "multiple value" logic in ManagedFile, which otherwise only caters for single uploads ? (i.e we end up with logic in the generic, lower-level ManagedFile that's coupled to the form structure of the higher-level and more specific FileWidget ?)

I might very well be wrong (away from my coding env atm, sorry for the noise if that's the case), but isn't this adding "multiple value" logic in ManagedFile, which otherwise only caters for single uploads ? (i.e we end up with logic in the generic, lower-level ManagedFile that's coupled to the form structure of the higher-level and more specific FileWidget ?)

Mh you are right about that, but it doesn't make the situation any worse than before, if I understand it correctly.
The previous controller also contained such a logic, and at the same time, was also intended to be used for a single file. Do you have an obvious way to fix that?

Alright, so the upload_max_size is 4x less than the post_max_size ..

Let's try something like this: Adapt the ini settings to be the same, so we can trigger the post case.

Mh, this test seems not to work on the testbot :(

Yeah no idea, also setting up those values 8MB 2MB locally doesn't help to reproduce the problem

#39 needs a reroll. But I applied it successfully on yesterday's HEAD, and ran the failing test locally, and it passes on my machine as well. Both on PHP 5.5 an 5.4. So, yes, something interesting happening on bot. Probably makes sense to open a tester issue and start throwing some debugging code at bot to see why it's behaving differently than local.

Just a reroll.

It's a pre-existing condition of HEAD that we don't have test coverage for uploading a file that exceeds post_max_size. Given that the added test is working locally but failing on bot, I don't think we should hold up a critical on figuring out what's different about the bot. So, removing it from here, but let's open a separate issue to add it. That separate issue can proceed independently of this one (i.e., land either before or after).

RTBC + 1, some nit picks and questions, but overall looks really great!

1. +++ b/core/lib/Drupal/Core/Form/EventSubscriber/FormAjaxSubscriber.php
   @@ -64,9 +78,24 @@ public function onView(GetResponseForControllerResultEvent $event) {
   +    // Render a nice error message in case you have a file upload which exceeds
   +    // the configured upload limit.
   

   nit nit nit - I thought we usually omitted 'you' or used 'we' instead.

2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -30,6 +33,8 @@
   +  use StringTranslationTrait;
   +
   
   @@ -138,6 +145,7 @@ public function __construct(FormValidatorInterface $form_validator, FormSubmitte
   +    $this->stringTranslation = $string_translation;
   

   not a bug - This is more question for myself:

   So even if I use the StringTranslation trait I still inject the string_translation service?

   /me wonders ...

3. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -1152,6 +1167,17 @@ protected function buttonWasClicked($element, FormStateInterface &$form_state) {
   +   * Wraps format_size() and file_upload_max_size().
   +   *
   

   Nice :)

It's a pre-existing condition of HEAD that we don't have test coverage for uploading a file that exceeds post_max_size. Given that the added test is working locally but failing on bot, I don't think we should hold up a critical on figuring out what's different about the bot. So, removing it from here, but let's open a separate issue to add it. That separate issue can proceed independently of this one (i.e., land either before or after).

Fair, its still a bit sad that this is not something obvious to check for.
Added a follow up for that #2510436: Test uploading a file > ini_get(post_max_size)

So even if I use the StringTranslation trait I still inject the string_translation service?

My personal rule of thumb is: For services inject the dependencies, for other code like controllers / plugins don't.
OH yeah that is exactly what tim wrote.

1. +++ b/core/lib/Drupal/Core/Form/EventSubscriber/FormAjaxSubscriber.php
   @@ -29,13 +33,23 @@ class FormAjaxSubscriber implements EventSubscriberInterface {
   +   *   The renderering service.
   

   Nit: s/renderering/renderer/

2. +++ b/core/lib/Drupal/Core/Form/EventSubscriber/FormAjaxSubscriber.php
   @@ -64,9 +78,24 @@ public function onView(GetResponseForControllerResultEvent $event) {
   +      $response->headers->set('X-Status-Code', 200);
   

   :O

   I totally did not know that X-Status-Code was a thing.

   Still, why not use setStatusCode() directly?

3. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -264,6 +272,13 @@ public function buildForm($form_id, FormStateInterface &$form_state) {
   +    // In case the post request exceeds the configured allowed size
   +    // (post_max_size), the post request is potentially broken. Add some
   

   Nit: s/post/POST/

4. +++ b/core/modules/file/src/Element/ManagedFile.php
   @@ -125,6 +129,53 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
   +   *   - Adds a class to the response to be able to highlight in the UI, that
   +   *     a new file got uploaded.
   

   Nit: 80 cols.

5. +++ b/core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php
   @@ -451,6 +448,19 @@ public static function processMultiple($element, FormStateInterface $form_state,
   +    // Count the amount of already uploaded files, in order to display new
   

   Nit: s/amount/number/

#51.2: Me, neither, but its only allowed for Exceptions handled by HttpKernel:

core/vendor/symfony/http-kernel/HttpKernel.php: if ($response->headers->has('X-Status-Code')) {
core/vendor/symfony/http-kernel/HttpKernel.php: $response->setStatusCode($response->headers->get('X-Status-Code'));
core/vendor/symfony/http-kernel/HttpKernel.php: $response->headers->remove('X-Status-Code');

Thank you tim!!

I totally did not know that X-Status-Code was a thing.

Still, why not use setStatusCode() directly?

Sadly I can't point to the code nor handbook on symfony.com, but symfony converts the exception to the corresponding http status code, so 500 for this example by default.
You need to use that header in order to bypass that and do exactly what you want.

#53: http://symfony.com/doc/current/reference/events.html#kernel-exception explains it. Thanks for the hint on the handbook :). (With "X-Status-Code site:symfony.com" I was able to find this easily.)

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -264,6 +272,13 @@ public function buildForm($form_id, FormStateInterface &$form_state) {
   +      throw new BadRequestHttpException($this->t('An unrecoverable error occurred. The uploaded file likely exceeded the maximum file size (@size) that this server supports.', ['@size' => $this->getFormattedFileUploadMaxSize()]));
   

   To t() or not to t()? We seem to be split as to whether we translate or not. I'm not sure what is best here.

2. +++ b/core/tests/Drupal/Tests/Core/Form/EventSubscriber/FormAjaxSubscriberTest.php
   @@ -7,13 +7,16 @@
   +use Drupal\Core\Ajax\AjaxResponse;
   

   Not used

Re #55.1: We t() that string in HEAD, so I don't think this issue should be the one to change that. But, do we need a followup for not t()'ing it inside an exception?

I think we do t(), by no means am I a translation expert but that seems like it's a user interface error message that an editor in a different language would likely see.

Yeah its an error which potentially could happen by a normal action of an user, so ...

+1 to #58 & #59.

+1 that this string needs to be t()'d. What I'm less clear on is whether it's okay to be t()'ing it at the point of throwing the exception or if we need a followup to move the t() to the place that catches the exception, except I also don't know if we have or want to introduce the pattern of adding placeholder arguments into an exception for deferred invocation of t(). Assigning to @alexpott for feedback about that.

@effulgentsia yep I'm concerned about the use of t() in the exception. It feels very wrong to have translated exception messages but this is hardly the only place in core. But two wrongs don't make a right. https://www.drupal.org/node/608166 says we should never translate exception messages.

+++ b/core/lib/Drupal/Core/Form/EventSubscriber/FormAjaxSubscriber.php
@@ -64,9 +78,24 @@ public function onView(GetResponseForControllerResultEvent $event) {
+    // Render a nice error message in case we have a file upload which exceeds
+    // the configured upload limit.
+    if ($exception instanceof BadRequestHttpException && $request->query->has(FormBuilderInterface::AJAX_FORM_REQUEST)) {
+      $this->drupalSetMessage($exception->getMessage(), 'error');
+      $response = new AjaxResponse();

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -264,6 +272,13 @@ public function buildForm($form_id, FormStateInterface &$form_state) {
+      throw new BadRequestHttpException($this->t('An unrecoverable error occurred. The uploaded file likely exceeded the maximum file size (@size) that this server supports.', ['@size' => $this->getFormattedFileUploadMaxSize()]));

I think we should have an exception that extends BadRequestHttpException and has a method for storing the size. And then the translated message should be prepared where this exception is consumed.

Good idea alex! More semantic exceptions!

While doing that I think it would be a great to not store the formatted output.

Much better. Thanks Alexes :)

Looks great and like the exception changes in #63.

Committed/pushed to 8.0.x, thanks!

Yeah!!!

This caused a regression: #2540850: (regression) EditorImageDialog alignment & captioning are not working.