? Pending

User tests: Successful: Unsuccessful:

avatar alikon
alikon
28 Jun 2016

Pull Request for Issue #7428 .
see #7428 (comment)

Summary of Changes

partial reverted #8130 following the new class JDatabaseExceptionExecuting
and for #10801

Testing Instructions

simulate a db error
for example change this line
https://github.com/joomla/joomla-cms/blob/staging/components/com_content/models/articles.php#L253
with this
->join('LEFT', '#__ZZZcategories as parent ON parent.id = c.parent_id');
in fronted click on :

  • article-category-blog menu item
  • article-category-list menu item
  • most read, latest etc modules
before patch

error1
error2

after patch

error3

avatar alikon alikon - open - 28 Jun 2016
avatar alikon alikon - change - 28 Jun 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 28 Jun 2016
Labels Added: ?
avatar sovainfo
sovainfo - comment - 28 Jun 2016

Consider the error to generic. It should read something like:
An error has occurred while trying to get the information from the database.

It is up to the application to catch this and turn it into something like:
No content available

avatar brianteeman brianteeman - change - 28 Jun 2016
Category Components
avatar mbabker
mbabker - comment - 28 Jun 2016

I'd suggest keeping the error message & code that the database layer sets versus using a generic error just as @sovainfo suggested. Otherwise it makes the entire Exception thrown here unusable for debugging. The main issue that needs addressing is the arbitrary appending of the SQL query to the error string which was done inline previously then changed to be done in the getErrorMessage() method of each driver after the stripping of table prefixes. Removing the query from the displayed error message 100% negates the need for said getErrorMessage() methods.

avatar alikon
alikon - comment - 28 Jun 2016

not so sure to fully understand, maybe i have been too radical/generic

so let's go less generic....
something like this
dberror

@sovainfo , @mbabker ?

avatar alikon
alikon - comment - 28 Jun 2016

...maybe we can undiscolse the dbname ...

avatar mbabker
mbabker - comment - 28 Jun 2016

It's a step forward.

JDatabaseExceptionExecuting (or its parent RuntimeException for 3.5 compatibility) should be caught by whatever's making the query and handling the error. In a lot of Joomla code those errors are going uncaught and bubbling up to the top level error handler (JErrorPage::render()). There isn't a simple fix to address that instead of going through each class/method, deciding what should catch Exceptions and what should allow Exceptions to bubble up, and handling that.

The thing about the getErrorMessage() methods is that they're really just a big bandage and a lazy way to deal with a partial issue. In well designed code no database error should get caught all the way at the top of the stack, some intermediary should be catching it even if it's just to give a more contextually appropriate error message/code.

For me the first priority is getting the SQL query out of the Exception message, your change in the MySQLi driver accomplishes just that. That same change should be applied to all the drivers and that change merged to core ASAP.

A longer project, or something you could dedicate resources for a 2-3 day code sprint with a handful of folks to to make a dent in the work, is reviewing all the database interactions in core. The MVC layer interactions (components) should all probably be catching errors no matter what. Sorting out what "helper" APIs should be catching and should be bubbling comes next. For example, JInstaller::abort() should probably be catching database exceptions, especially since that's an error handling routine that's trying to gracefully back out a partial extension installation. On the other hand, JPluginHelper::load() should probably catch the database exception and throw a new exception with a more localized error message (Failed to load plugin data) and the database exception chained to the new object so as to "mask" the database error with a contextually appropriate error string but still make the source error available for debugging. Lastly, you've got something like JHtmlAccess::level() which as a pure HTML helper might be better off just letting the Exception bubble and an error handler in whatever is using that JHtml helper (presumably we're talking about a JForm or JFormField object or something in the MVC layer with this specific one) handle catching errors at a higher level and "translate" them as need be.

One last idea (not related to this RFC but should be implemented), in our error.php templates with debugging enabled we're only showing the error details of the "top" exception object. It might be a good idea to add handling to loop while Throwable::getPrevious() returns an object so that each object in the trace is displayed to help with debugging.

avatar sovainfo
sovainfo - comment - 29 Jun 2016

Dislike very much that it says it can't find the page. Don't expect this page in this situation. Assuming it is a component problem expect the template and modules and a message the content is not available. In the case it is a problem with a module it should produce the page. It should be configurable whether to report anything on the page, for both situations. Probably at the menu item level for the component and at the module level for each module.

avatar alikon
alikon - comment - 29 Jun 2016

so let's go step by step
first step :

getting the SQL query out of the Exception message

please some test...

avatar mbabker
mbabker - comment - 29 Jun 2016

? from me. Let's get this merged and move on with things.

avatar alikon alikon - change - 30 Jun 2016
Title
[rfc] - Database error handling
Database error handling
avatar alikon alikon - change - 30 Jun 2016
Title
[rfc] - Database error handling
Database error handling
avatar hardiktailored hardiktailored - test_item - 11 Jul 2016 - Tested successfully
avatar hardiktailored
hardiktailored - comment - 11 Jul 2016

I have tested this item successfully on 8bde217


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

avatar hardiktailored
hardiktailored - comment - 11 Jul 2016

Check the test result below.

Before applying patch:
screen shot 2016-07-11 at 07 31 09

After applying patch:
screen shot 2016-07-11 at 07 31 57


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

avatar hardiktailored
hardiktailored - comment - 11 Jul 2016

Note that the error message replaced but it is showing two times.


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

avatar gunjanpatel
gunjanpatel - comment - 14 Jul 2016

@hardiktailored then test should be unsuccessful as if error message comes two times.


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

avatar hardiktailored hardiktailored - test_item - 14 Jul 2016 - Tested unsuccessfully
avatar hardiktailored
hardiktailored - comment - 14 Jul 2016

I have tested this item ? unsuccessfully on 8bde217

Changed to unsuccessful as suggested due to error message showing two times.


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

avatar alikon
alikon - comment - 30 Dec 2016

closed in favour of #13356

avatar alikon alikon - close - 30 Dec 2016
avatar alikon alikon - change - 30 Dec 2016
The description was changed
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2016-12-30 16:59:53
Closed_By alikon
avatar joomla-cms-bot joomla-cms-bot - change - 30 Dec 2016
Category Components Libraries Postgresql MS SQL Components

Add a Comment

Login with GitHub to post a comment