User tests: Successful: Unsuccessful:
The file path should be shown in the error message when JForm can't load a file.
Error message:
JForm::getInstance could not load file article
Error message:
JForm::getInstance could not load file
Status | New | ⇒ | Pending |
Category | ⇒ | Libraries |
It is still better from what we have now and it happens mostly when we are developing stuff and not in production, at least not for me.
Can't we use a Language String for this error and change it too correct UX like @sovainfo suggest.
Then we can display the correct message to the Joomla! Administrators.
Something like language string GLOBAL_FILENOTFOUND = Could not load the needed file. Try again later or consult your webmaster. Filename:
@brianteeman can you comment on this?
Actually, the library class shouldn't throw exceptions with user friendly messages. They are more meant to be useful for developers. I wouldn't show the full path though as it may be an information disclosure. The current message actually is better imho.
What is much more important is that the Exception is catched in the calling code and there you can then show a translated message to the user for that case.
Any error message exposing a file name/path is a bad error message and has security implications.
In addition would expect some decent error handling around the loadfile as well.
Currently using simplexml_load_file, guess what happens when file is not well formed!
while on it, check load as well !
Currently using simplexml_load_file, guess what happens when file is not well formed!
$this->loadUser('mbabker')->createRant('Joomla error handling lacking in a lot of places');
See no problem disclosing it in DEBUG or logfile.
We really need a mentality shift in Joomla. In the past any error that was raised would basically always go to the user via the message queue. The debug console and static log files are powerful tools for administrators and we really should be using them better (even if it means we have to add something to com_admin to be able to read the logs directory and parse the individual file contents if we don't want to impose a "FTP to your site to get the files" requirement).
Luckily with 3.7 the debug console now has hook points for developers so we can encourage those callbacks to be used going forward.
Coming back to the origin of the PR. It just adds the name of the file without path and extension. I think from security point of view, this should be ok.
If you only show the filename, then it's no security issue indeed. Not sure how helpful it then is.
From security point of view, no issue at all. Still not the correct thing to do. It should be logged in a file and include al information. That means you either log here and throw a generic message or you provide the details and have all callers log it and produce a proper message or deal with it more appropriate. It should be the caller that decides on the severity. It might even completely ignore it!
Considering the nature of this service would go for logging specifics and communicating to caller something was wrong, so it can deal with it properly.
This bad error handling in the application should be corrected. It should never reach a visitor, regardless whether that is from the frontend or backend.
So, changing something but NOT dealing with the actual issue is something that shouldn't be done!
Don't change something because you can, change something because there is a need!
But I don't see the point here. If the error handling should be improved in general, then please open an issue or propose a PR. But just declining pr's which do solve something different is for me the wrong approach.
Not sure how helpful it then is.
I was implementing a new view in the back end with filters and something went wrong with one of the filter files. So either way you have two options in that situation, debug JForm or try and error which file it is.
The form name (file name) should not be part of the error message the user sees, it just doesn't matter to them at all. As is this PR is no good by me, this is information that should be made available to the developer by other means. Throwing all of the information in an Exception's message is why you have stupid crap like SQL queries being displayed on our error pages.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-01-23 18:26:14 |
Closed_By | ⇒ | laoneo |
Still very bad UX !
Never heard of a content manager understanding JForm::getInstance.
Probably a lot of administrators don't understand that, you need to be developer to possibly understand that.
Proper UX would talk to the user in the language he understands: The function is currently not available. Try again later or consult your webmaster.
Obviously, proper logging should be implemented! Exact details should be logged in order to fix the issue. That would include all details, no obfuscating. To allow proper service it would be nice if the user experiencing this was logged as well.
An added bonus would be when those logs would be available in DEBUG, when configured of course. Saves going to the logfile.