[#20438] - Add $uploadResult variable to JFile class
- Closed
- 12 May 2019
- Medium
- Build: staging
- # 20438
- Diff
- olleharstedt:staging
- Success Hound No violations found. Woof! Details
- Success continuous-integration/drone/pr the build was successful Details
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
- Success continuous-integration/appveyor/pr AppVeyor build succeeded Details
User tests: Successful: Unsuccessful:
Pull Request for Issue #20246.
Summary of Changes
Add static variable $uploadResult to JFile class.
Testing Instructions
Run JFile::upload(). Run JFile::getUploadResult().
Expected result
$uploadResult is set to result of upload() method.
Actual result
Documentation Changes Required
Don't know.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
PhilETaylor
- 
Something that could be discussed is if we should use the convention "0 for success" instead of "truthy for success", as I've used now (the constants start at 1 and then counts down).
Should we not start throwing exceptions? I don't like to have state full helper classes. It is not wrong how you do it, but we can do better.
Hit the "Comment" button too early. Yes then we should do it against J4.
I agree that throwing exception is the correct way, but yes, it's about backwards compatibility. Alternatively, we can have a new method uploadWithException or something, that does throw exception (code duplication, though...).
Instead of trying to cater for B/C, do it once and do it right. Do a PR against 4.0 (and the 2.0 branch of the Framework's Filesystem package) breaking B/C to do it right. Yeah it means you have to live with the "busted" code for the rest of the 3.x lifetime, but at this point it's better than adding something that creates new stateful statics or APIs which are nothing more than complete hacks that are going to have to stay in the API for who knows how long.
| Labels |
Added:
?
|
||
How about changing the return value from true/false to 1 and 0/-1/-2 etc as return codes? It would break compatibility, but not "so much" (only if client code uses ===).
@olleharstedt whats the state of this Pull Request?
@franz-wohlkoenig It should be closed, and a discussion should be had about my other suggestion: to return integer instead of boolean as a result code OR throw exception in version 4 instead.
thanks @olleharstedt - can you open an issue about "return integer instead of boolean"?
closed as stated above.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-05-12 08:17:08 |
| Closed_By | ⇒ | franz-wohlkoenig | |
| Labels |
Removed:
J3 Issue
|
||
Sure, I'll have a look tomorrow.
Hm, those constants does not really reflect the error messages logged in upload().