Contrib.social

Error message presented when a BrokenPostRequestException is fired on AJAX submission can be misleading

Open on Drupal.org →
Created on 1 November 2021, about 3 years ago
Updated 8 February 2024, 10 months ago

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

🐛 Bug report
Status

Needs work

Version

11.0 🔥

Component
Ajax  →

Last updated 6 days ago

Created by

🇨🇦Canada brianV

Live updates comments and jobs are added and updated live.
  • Needs usability review

    Used to alert the usability topic maintainer(s) that an issue significantly affects (or has the potential to affect) the usability of Drupal, and their signoff is needed. When adding this tag, make it easy to review the issue. Make sure the issue summary describes the problem and the proposed solution. Screenshots usually help a lot! To get sign-off on issues with the "Needs usability review" tag, post about them in the #ux channel on Drupal Slack, and/or attend a UX meeting to demo the patch and get direct feedback from designers/UX folks/product management on next steps. If an issue represents a significant new feature, UI change, or change to the general "user experience" of Drupal, use Needs product manager review instead. See the scope of responsibilities for product managers.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment almost 2 years ago →
    🇺🇸United States smustgrave

    For the UX change.

  • Comment over 1 year ago →
    heddn Nicaragua

    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.

  • Status changed to Needs work over 1 year ago
  • Comment over 1 year ago →
    🇺🇸United States tr Cascadia

    "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, 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.

    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.

  • Status changed to Needs review over 1 year ago
  • Comment over 1 year ago →
    RISHABH VISHWAKARMA

    Addressed the points mentioned in #15.

  • Status changed to Needs work over 1 year ago
  • Comment over 1 year ago →
    🇺🇸United States smustgrave

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

  • Comment 10 months ago →
    🇸🇰Slovakia poker10

    Is this a duplicate of ✨ Misleading error message when uploading a file Needs work ?

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024