Problem/Motivation

When an AJAX form is submitted, Drupal\Core\Form\FormBuilder::buildForm() performs the following check:

    // In case the post request exceeds the configured allowed size
    // (post_max_size), the post request is potentially broken. Add some
    // protection against that and at the same time have a nice error message.
    if ($ajax_form_request && !$request->request->has('form_id')) {
      throw new BrokenPostRequestException($this->getFileUploadMaxSize());
    }

This is intercepted by Drupal\Core\Form\EventSubscriber\FormAjaxSubscriber::onException() which has the following code:

    // Render a nice error message in case we have a file upload which exceeds
    // the configured upload limit.
    if ($exception instanceof BrokenPostRequestException && $request->query->has(FormBuilderInterface::AJAX_FORM_REQUEST)) {
      $this->messenger->addError($this->t('An unrecoverable error occurred. The uploaded file likely exceeded the maximum file size (@size) that this server supports.', ['@size' => $this->formatSize($exception->getSize())]));
      $response = new AjaxResponse(NULL, 200);
      $status_messages = ['#type' => 'status_messages'];
      $response->addCommand(new PrependCommand(NULL, $status_messages));
      $event->allowCustomResponseCode();
      $event->setResponse($response);
      return;
    }

Note that the exception is triggered by a missing form_id during an AJAX request.

There are multiple reasons I've discovered why form_id may be missing:

  1. The request file size is too large and gets silently truncated (correctly called out in the existing error message)
  2. A mistake in a form theme template causes the form_id to not get submitted.
  3. The one that bit me: The number of form fields being submitted exceeds PHP's max_input_vars setting, which defaults to 1000.

Proposed resolution

The current error message is:

An unrecoverable error occurred. The uploaded file likely exceeded the maximum file size (@size) that this server supports.

We should replace this with a more descriptive and accurate message. Here is a proposed replacement:

An unrecoverable error occured. The form's form_id key must be present. Ensure your form is submitting it's form ID, that your form submission does not exceed the number of values set in PHP's max_input_vars (currently @amount), and the total size of your submission is less than the maximum file size (@size) that this server supports.

It's probably too verbose, so would love to see better proposals..

Remaining tasks

  • Decide on a better error message.
  • Implement it.

User interface changes

API changes

Data model changes

Release notes snippet

Comments

brianV created an issue. See original summary.

brianV’s picture

Issue summary: View changes
brianV’s picture

Title: Error message presented when a BrokenPostRequestException is fire on AJAX submission can be misleading » Error message presented when a BrokenPostRequestException is fired on AJAX submission can be misleading
Issue summary: View changes
brianV’s picture

Version: 9.2.x-dev » 9.3.x-dev
Status: Active » Needs review
StatusFileSize
new1.5 KB
brianV’s picture

Updated patch with spelling issue fixed.

Status: Needs review » Needs work
brianV’s picture

Fix the broken test.

brianV’s picture

Status: Needs work » Needs review

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

ranjith_kumar_k_u’s picture

StatusFileSize
new2.74 KB

Rerolled #7

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

Issue tags: +Needs usability review

For the UX change.

heddn’s picture

I think we should make the error more generic, not more specific. But then log the exception message to watchdog. Otherwise, I'm driving in the dark here as we eat the exception/throwable without listing out what is at fault.

tr’s picture

Status: Needs review » Needs work

"its", not "it's".

Also, the code comment says "// Render a nice error message in case we have a file upload which exceeds the configured upload limit." but if that's not really what is happening here then the code comments should be changed as well.

Likewise, in core/lib/Drupal/Core/Form/FormBuilder.php, where this exception is thrown, we have this code:

    // In case the post request exceeds the configured allowed size
    // (post_max_size), the post request is potentially broken. Add some
    // protection against that and at the same time have a nice error message.
    if ($ajax_form_request && !$request->request->has('form_id')) {
      throw new BrokenPostRequestException($this->getFileUploadMaxSize());
    }

Again, this comment doesn't agree with the actual code, and doesn't agree with what the patch says the problem is. This code is explicitly checking for form_id, so why is it passing the file upload max size as an argument to the exception if the form_id is the problem? How is file upload size involved here?

If BrokenPostException is being used for all these different problems - file upload size, missing form_id, max_input_vars, post_max_size, then I would argue that there needs to be more than one type of exception defined and thrown because it's not useful to have just one generic exception here that wasn't intended to cover all these cases. And each of these error conditions ought to be checked separately.

I agree with @heddn that in general detailed error messages should not be shown to the end user, and that detailed information about what went wrong and where it went wrong should be logged for the site owner.

rishabh vishwakarma’s picture

Version: 10.1.x-dev » 9.5.x-dev
Status: Needs work » Needs review
StatusFileSize
new3.94 KB
new4.03 KB

Addressed the points mentioned in #15.

smustgrave’s picture

Version: 9.5.x-dev » 11.x-dev
Status: Needs review » Needs work

From what I can tell the message being displaced is still the detailed approach vs generic. And having the detailed message logged instead.

poker10’s picture

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.