API page: http://api.drupal.org/api/drupal/modules%21file%21file.module/function/f...
Enter a descriptive title (above) relating to file_ajax_upload, then describe the problem you have found:
In the following validation:
if (empty($_POST['form_build_id']) || $form_build_id != $_POST['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');
$commands = array();
$commands[] = ajax_command_replace(NULL, theme('status_messages'));
return array('#type' => 'ajax', '#commands' => $commands);
}
The error message has absolutly nothing related to the actual error, this greatly mislead a member of our team while debugging one of our feature. I understand the "security implications" of giving an explanation of whats going on to the user but maybe the error message could be something like "An error occured while authorizing the upload of your file." Or something more vague...
| Comment | File | Size | Author |
|---|---|---|---|
| #40 | interdiff_35-40.txt | 2.25 KB | lucassc |
| #40 | 1452128-40.patch | 2.38 KB | lucassc |
Comments
Comment #1
jhodgdonSounds like a code, not documentation issue report.
Comment #2
jyee commentedHow about, "An unrecoverable error occurred. The submitted information does not match the generated form. Try reloading the page and submitting again."?
That should provide a hint for developers that there's a mismatch between the submission and generation (i.e. form build id submitted does not match the actual form). It's also not overwhelming or scary for end users. Assuming that there's nothing intentionally resetting the build id, reloading the page would correct the build id and be an appropriate next-step for end users.
Comment #3
matthieu.symetris commentedI think it's great,
will it be implemented in the next release of drupal 7.x and 8.x? I'm not familiar with the proccess of suggesting code ajustement to drupal. Should I change the status of this super-minor issue? :)
Thank you very much jyee.
Comment #4
jyee commented@matthieu.symetris: The general process is for this to be reviewed and for other Community members to comment on it. Then, assuming there's consensus to accept this solution, a Drupal project maintainer can commit it.
Comment #6
jyee commentedSo, I'm not sure why that first patch had a couple unrelated code changes. Here's a clean patch against D8 (hash: 467a825239bdd34a84e18064cb6c8ac8ecb7bb1f). I also generated it from Drupal root.
Comment #7
adamdicarlo commentedThe patch looks good to me. Though it passing does reveal that there is no test for this failure case, right? Does this need a test case?
Comment #8
jyee commentedDo you mean a test to corrupt the form build id in order to verify that this error message appears as expected?
Comment #9
liam morlandThe original error message is a correct, at least in some circumstances. I am seeing this error when trying to upload files that are larger than the maximum configured in PHP.
Comment #10
alansaviolobo commentedis it not possible to catch the large file error separately ?
Comment #11
jhedstromPatch no longer applies. Needs to be bumped to 8.1 since beta freezes strings.
Comment #12
liam morlandReroll.
Comment #13
jhedstromI was incorrect in #11. Beta did not freeze strings. Back to 8.0.
Comment #16
liam morlandReroll.
Comment #27
mitthukumawat commentedThe patch applied cleanly on the Drupal 8.1.x-dev version but not in Drupal 9.2.x-dev version. So adding patch for 9.2.x-dev version.
Comment #30
nikitagupta commentedComment #34
smustgrave commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
At this time we will need a D10 version of the patch.
Tagging for IS update for proposed solution and remaining tasks.
Comment #35
lucasscRerolled for 10.1.x
Comment #36
smustgrave commentedThank you for working on this.
When uploading patches please include an interdiff file also
Moving back to NW for the issue summary update.
Comment #37
lucasscMy bad, I couldn't attach an interdiff duo to the error below:
But the only difference was that I kept the new D10
statusMessageContainsAfterWait(message, 'error')over the patch'spageTextContains.Comment #38
smustgrave commentedNo worries!
I occasionally get that myself. For those what I do is
diff old.patch new.patch > diff-[old-patch-number]-[new-patch-number].txtComment #39
liam morlandIn the first hunk, the
@sizetoken is removed from the text, so it should not be set in the arguments.Comment #40
lucasscThat's right! @size's arguments removed. I took the opportunity to add a final point that was missing.
And what about these comments?
Should we change them too?
Comment #42
pameeela commentedComment #44
baikho commented