Show the php.ini setting for max upload or alterantively the in phabricator configured one. Fixes T6663
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T6663: Friendly message when exceeding file upload size limit
- Commits
- Restricted Diffusion Commit
rP6132d8012b02: show the current size limit when a file upload fails
changed php.ini and alternatively phabricator file upload size settings to minimal values and try to upload a larger file
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- fileupload
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 3318 Build 3325: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
Comment Actions
I'm wondering if we should have a friendly error and an admin error. If you are just an average user, the message is a bit confusing and meaningless (and perhaps the admin is not even known).
Comment Actions
Whatever this error message ends up being, it should continue or improve the ratio of users who bug their administrator to fix configuration : bug the upstream with issues. Or in other words, today when a user encounters this error they bother the administrator or go on a "whose the administrator" hunt; I think whatever change we make should retain this property and certainly not increase the amount of people who come to the upstream with questions.
Comment Actions
I think the main difference here was to show the user the actual size limit. Otherwise he'd have to guess, maybe compress the pdf/txt whatever and reupload to get the same error.
But it may be a generally good idea to display the configured administrators information (name/email) or maybe even a link to directly create a maniphest task with the exceptions info whenever an exception is shown?
Comment Actions
I'm hesitant to do admin vs user messages because I worry they weaken the property @btrahan discusses: particularly, if a user gets the "dumb/friendly" message and tells an admin, the admin is less likely to be able to resolve the error unassisted than if they had all the information.
I think adding the size information while retaining the configuration options in the error messages balances concerns well.
On "mail an admin" / "file a task", we haven't seen much demand for that from installs. We have a very old task (T302) about making traces actionable. After Nuance gets built, it might give us easier / more natural / more flexible options.
| src/applications/files/exception/PhabricatorFileUploadException.php | ||
|---|---|---|
| 6–18 | This construction can be a bit misleading, because it's (theoretically, and probably practically) possible to exceed upload_max_filesize even when storage.upload-size-limit is (incorrectly) configured. If storage.upload-size-limit is set to 100MB, and upload_max_filesize is set to 1MB, and the user tried to upload a 5MB file, the user would previously receive an exception pointing them at upload_max_filesize, which is correct and specific. They will now receive a message pointing them at storage.upload-size-limit, which is less specific and less useful. The setting may really need to be adjusted (it has not been configured correctly if it is set to a higher value than any of the other limits) but it's less useful to send users there, and it's probably not the setting they want to adjust since it tends to represent intent better than other limits. Overall, I think we can be most clear and specific by providing two separate error messages. | |
| 16 | For consistency, capitalize "Phabricator". Maybe "To adjust this limit..." instead of "Consider changing...", although this is super nitpicky. | |
Comment Actions
I initially didn't get the '-1000' being the phabricator limit exception and wondered why there were two identical error cases ;)
But to differentiate here totally makes sense.
As i'm not a native speaker, feedback about my wording is always appreciated even when it might seem nitpicky.