[#4209] - Non-descriptive error messages when a package upload fails in com_installer
- Closed
- 4 Sep 2014
- Medium
- Build: staging
- # 4209
- Diff
- nikosdion:fix/upload-errors-in-com-installer
- Failure The Travis CI build failed Details
User tests: Successful: Unsuccessful:
Executive summary
When you try installing an extension package file with Manage Extensions but your PHP upload temporary directory (upload_tmp_dir) is not set in pph.ini or the package size is larger than the maximum upload size (max_upload_size) you get a generic error which doesn't let you understand why the upload failed. This PR adds descriptive error messages using the same language strings we are already using in the Warnings page.
Backwards compatibility
No backwards compatibility issues are introduced.
Translation impact
No translation strings are added, removed or modified.
Testing instructions
Test 1: Make sure that your server's php.ini has a max_upload_size of 1M. Try installing an extension bigger than that, e.g. Admin Tools Core, K2, Community Builder etc. You get a generic upload error. Apply the PR and retry. You now get an error telling you that the upload failed and that your maximum upload limit is too small.
Test 2 (the problem may not appear on all servers): Make sure that your server's php.ini doesn't have an upload_tmp_dir set. Try installing any extension. You get a generic upload error. Apply the PR and retry. You now get an error telling you that the upload failed and the PHP temporary directory is not set.
jissues-bot
- | Labels |
Added:
?
|
||
Regarding returns: I am following the same codestyle as the rest of the code in this method. It would be extremely ridiculous to have line breaks before returns when the rest of the code in this method doesn't. The idea behind code style is enforcing uniformity, not breaking it.
Regarding the break, here you go.
This is a really useful error-message bug-fix! Thank you @nikosdion 
@test, @nikosdion Tested great for me following test instructions. This is great! I'm amazed how low the upload size is on some servers!
I must have one of the servers that test 2 may not appear on.
| Labels |
Added:
?
|
||
| Labels |
Added:
?
|
||
Regarding returns: I am following the same codestyle as the rest of the code in this method. It would be extremely ridiculous to have line breaks before returns when the rest of the code in this method doesn't. The idea behind code style is enforcing uniformity, not breaking it.
Imho JM is right that our codestyle requires an empty line before a return statement, however Travis seems to be fine.
I completely agree with you that codestyle is about enforcing uniformity. The thing is that we only require it for new submissions, existing code isn't tested. So their is a period which can be annoying long when there is part of it folling the codestyle and other parts dont.
There are some community members which do codestyle PRs from time to time. So eventually it will be fixed.
That being said, as long as Travis is fine with codestyle, Jenkins hopefully is fine as well. If not, I can fix it after merging myself.
I fully agree, Thomas. Do you want me to go through the entire file and
add the missing line breaks everywhere?
I fully agree, Thomas. Do you want me to go through the entire file and
add the missing line breaks everywhere?
Personally I prefer codestyle changes in a separate PR. It's easier to review and less likely to produce conflicts. But I also don't mind if you do it, it's not a big file in this case and shouldn't generate problems 
OK, better put code style changes in a new PR once this one is merged.
zero-24
- | Status | New | ⇒ | Ready to Commit |
dbhurley
- | Status | Ready to Commit | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2014-09-04 19:11:27 |
@nikosdion
Please add a line before the returns.
Also, as you are using existing lang strings, I suggest, to cope with all languages, to replace the space between the 2 strings by a