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:
- The request file size is too large and gets silently truncated (correctly called out in the existing error message)
- A mistake in a form theme template causes the
form_idto not get submitted. - The one that bit me: The number of form fields being submitted exceeds PHP's
max_input_varssetting, 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
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | interdiff_11_16.txt | 4.03 KB | rishabh vishwakarma |
| #16 | 3247064-16.patch | 3.94 KB | rishabh vishwakarma |
| #11 | 3247064-11.patch | 2.74 KB | ranjith_kumar_k_u |
| #7 | drupal-broken_post_request_exception_message-3247064-6.patch | 2.74 KB | brianV |
Comments
Comment #2
brianV commentedComment #3
brianV commentedComment #4
brianV commentedComment #5
brianV commentedUpdated patch with spelling issue fixed.
Comment #7
brianV commentedFix the broken test.
Comment #8
brianV commentedComment #11
ranjith_kumar_k_u commentedRerolled #7
Comment #13
smustgrave commentedFor the UX change.
Comment #14
heddnI 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.
Comment #15
tr commented"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:
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.
Comment #16
rishabh vishwakarma commentedAddressed the points mentioned in #15.
Comment #17
smustgrave commentedFrom what I can tell the message being displaced is still the detailed approach vs generic. And having the detailed message logged instead.
Comment #18
poker10 commentedIs this a duplicate of #1452128: Misleading error message when uploading a file?