? Success

User tests: Successful: Unsuccessful:

avatar laoneo
laoneo
30 Nov 2016

Summary of Changes

The file path should be shown in the error message when JForm can't load a file.

Testing Instructions

  • Rename the file administrator/components/com_content/models/forms/article.xml to administrator/components/com_content/models/forms/article.xml.tmp
  • In the back end go to Content -> Articles -> Add new article

Expected result

Error message:
JForm::getInstance could not load file article

Actual result

Error message:
JForm::getInstance could not load file

avatar laoneo laoneo - open - 30 Nov 2016
avatar laoneo laoneo - change - 30 Nov 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 30 Nov 2016
Category Libraries
avatar sovainfo
sovainfo - comment - 1 Dec 2016

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.

avatar laoneo
laoneo - comment - 1 Dec 2016

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.

avatar conconnl
conconnl - comment - 2 Dec 2016

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?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13050.

avatar Bakual
Bakual - comment - 2 Dec 2016

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.

avatar mbabker
mbabker - comment - 2 Dec 2016

Any error message exposing a file name/path is a bad error message and has security implications.

avatar sovainfo
sovainfo - comment - 2 Dec 2016

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 !

avatar mbabker
mbabker - comment - 2 Dec 2016

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');
avatar brianteeman
brianteeman - comment - 3 Dec 2016

I tend to agree with @mbabker on this - disclosing the full path is never a good idea

avatar sovainfo
sovainfo - comment - 3 Dec 2016

See no problem disclosing it in DEBUG or logfile.

avatar mbabker
mbabker - comment - 3 Dec 2016

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.

avatar laoneo
laoneo - comment - 7 Dec 2016

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.

avatar Bakual
Bakual - comment - 7 Dec 2016

If you only show the filename, then it's no security issue indeed. Not sure how helpful it then is.

avatar sovainfo
sovainfo - comment - 7 Dec 2016

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!

avatar laoneo
laoneo - comment - 21 Dec 2016

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.

avatar mbabker
mbabker - comment - 21 Dec 2016

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.

avatar laoneo laoneo - change - 23 Jan 2017
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2017-01-23 18:26:14
Closed_By laoneo
avatar laoneo laoneo - close - 23 Jan 2017

Add a Comment

Login with GitHub to post a comment