Handle DB errors in categories search plugin #6710

alikon · 2015-04-08T17:58:08Z

updated to handle DB errors like #6612, #6632 and for #6591

Testing info
simulate a db error

updated to handle DB errors like #6612, #6632 and for #6591 Testing info simulate a db error

whitespace

phproberto · 2015-04-09T15:35:15Z

plugins/search/categories/categories.php

+		catch (RuntimeException $e)
+		{
+			$rows = array();
+			JFactory::getApplication()->enqueueMessage($e->getMessage(), 'error');


What is the point in catching the error if we are going to then show it in a system message? I think it should be sent to logs or at least check that the user has core.admin rights before showing the DB error.

Well at least the page still renders :P But I agree with the point. Part of the issue is we don't want to show the database prefix to an average user. We need a more general error message and to ensure the DB failure is logged somewhere for admins

JDatabaseDriver already has logging mechanisms in place...

yes. I tested that today when @wilsonge suggested it in Glip. Is not easy to understand but you can get the debug system plugin showing that kind of errors.

Is there a better way to do this?

So for db exceptions we need to replace:

JFactory::getApplication()->enqueueMessage($e->getMessage(), 'error');

with:

if (JFactory::getUser()->authorise('core.admin')) { JFactory::getApplication()->enqueueMessage($e->getMessage(), 'error'); }

Would it not be better to do that at the JModuleHelper level?

If it's at the module rendering level I think it's not enough:

It's not something that we can use everywhere (a recommended practice that we can document)

Sometimes you want to return content even having an exception that requires admin review

It isn't JModuleHelper's job to act as a global exception handler for
modules. Same as JComponentHelper doesn't for components.

The error message displayed to the user, if one is displayed, should
always be sanitized. IMO there isn't a need to wrap the enqueueMessage
call in a core.admin permission check. Use the logs for verbose error
logging with "sensitive" data.. In the UI, it is difficult to turn one on,
I agree, but the code structure is already present to use this stuff.
Let's not make things more difficult than we need to please.

On Friday, April 10, 2015, George Wilson notifications@github.com wrote:

In plugins/search/categories/categories.php
#6710 (comment):

@@ -169,7 +169,15 @@ public function onContentSearch($text, $phrase = '', $ordering = '', $areas = nu
}

$db->setQuery($query, 0, $limit);

$rows = $db->loadObjectList();

try

{

$rows = $db->loadObjectList();

}

catch (RuntimeException $e)

{

$rows = array();

JFactory::getApplication()->enqueueMessage($e->getMessage(), 'error');

Would it not be better to do that at the JModuleHelper level?

—
Reply to this email directly or view it on GitHub
https://github.com/joomla/joomla-cms/pull/6710/files#r28108367.

I agree with Michael. Don't show the query error to the user. Just show a generic message "Oops, the database query failed. Please notifiy the administrator." Or something like that. I wouldn't add ACL checks for that kind of things.
We could add docs for the administrator how to get the full error.

sovainfo · 2015-04-10T09:18:20Z

The errors should be logged and currently that requires proper configuration of debug. Consider that not sufficient. The product should have a logger enabled for errors.

mbabker · 2015-04-10T09:30:13Z

It can be default enabled but there are conditions to keep in mind:

A hosting platform where Joomla can't write log files (due to poor config
or a user implementing extra "security" measures)
Failing to write to the log shouldn't trigger a fatal error (I think I've
seen instances where this may happen, in part due to the lack of
permissions)
Reviewing where we are actually using JLog now. We have a log callback
that enqueues messages for the 'jerror' log category (which IMO is really
not optimal; if we want to log and display a message just call both...).
We don't want to be writing logs on every request a site gets; those files
blow up pretty quickly, especially if catching deprecations or logging all
database queries.

On Friday, April 10, 2015, sovainfo notifications@github.com wrote:

The errors should be logged and currently that requires proper
configuration of debug. Consider that not sufficient. The product should
have a logger enabled for errors.

—
Reply to this email directly or view it on GitHub
#6710 (comment).

sovainfo · 2015-04-10T10:39:52Z

Guess you missed the second word: ERROR
Added this to cms.php:
JLog::addLogger(array(), JLog::ERROR, array('deprecated'), true);
Then created an error in the featured query, it was logged and embarrassingly enough also shown:

alikon · 2015-04-10T16:49:29Z

the point is how the core modules,plugins,ect handle db errors ( #6591 ) ?

showing a generic message

JFactory::getApplication()->enqueueMessage('blah blah ', 'warning');

or something different like

if (JFactory::getUser()->authorise('core.admin'))
{
 JLog::addLogger("blla bla");  
........
}

i would prefer the keep it simplest stupid "bla bla" msg

but not sure to understand what exactly is the "family feeling"

i'm missing something?

p.s
a special thanks to all #pbfnl

wilsonge · 2015-05-07T20:36:51Z

Me and @roland-d just had a chat about this. We have made an executive call that we show a generic "blah blah" message to users and then just add the exception message into the Logs. Just make sure it isn't in a category that will be shown to the frontend user.

showing a generic msg to user log the error

alikon · 2015-05-10T07:00:24Z

done
i've used a ready lang string maybe something different?

wilsonge · 2015-05-11T22:54:36Z

I think probably something different. I'll ping @brianteeman in the language string group on glip

roland-d · 2015-05-28T14:31:23Z

@wilsonge Any update on this?

wilsonge · 2015-05-28T23:26:45Z

@brianteeman recommended as it's a string to frontend users we either display a hugely simplified string of "There has been an error" or don't show a string at all.

roland-d · 2015-06-09T18:52:28Z

plugins/search/categories/categories.php

@@ -169,7 +169,16 @@ public function onContentSearch($text, $phrase = '', $ordering = '', $areas = nu
 		}

 		$db->setQuery($query, 0, $limit);
-		$rows = $db->loadObjectList();
+		try


@alikon Can you please add an empty line before try so we stick to the codestyle. Thanks.

roland-d · 2015-06-09T18:58:19Z

@test Works as described, after generating an error, I get a simple error message that the database connection is not there.

To test, I changed the plugins/search/categories/categories.php file on line 152 and changed THEN to THN. On the frontend, I used the search function to search categories only and received the error page with a large DB query. After applying the patch and making the same change, I now receive a graceful message.

alikon · 2015-06-09T19:31:19Z

@roland-d i'm sorry for my pr patchworks
look at this one please #7073

p.s
maybe this one could be closed

roland-d · 2015-06-09T19:41:25Z

@alikon There are a lot of these db PRs. Is there a comprehensive list with all of them that should be tested?

alikon · 2015-06-09T19:49:04Z

@roland-d my search string

is:open is:pr author:alikon Handle DB

;)

coolcat-creations · 2015-06-09T19:52:26Z

Did same testing procedure like @roland-d - successfull

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/6710.}

zero-24 · 2015-06-09T19:54:58Z

RTC 😄

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/6710.}

wilsonge · 2015-06-14T22:59:15Z

Closing in favour of #7073

Handle DB errors in categories search plugin

5a3328c

updated to handle DB errors like #6612, #6632 and for #6591 Testing info simulate a db error

joomla-cms-bot added the PR-staging label Apr 8, 2015

alikon mentioned this pull request Apr 8, 2015

Handle DB errors in contacts search plugin #6711

Closed

travis

b17a5ff

whitespace

This was referenced Apr 9, 2015

Handle DB errors in tags search plugin #6714

Closed

Handle DB errors in newsfeeds search plugin #6715

Closed

Handle DB errors in content search plugin #6716

Closed

Handle DB errors in tinymce editors plugin #6717

Closed

phproberto reviewed Apr 9, 2015
View reviewed changes

wilsonge added this to the Joomla! 3.4.2 milestone Apr 9, 2015

Update categories.php

9114788

showing a generic msg to user log the error

alikon mentioned this pull request May 30, 2015

Handle DB errors in categories search plugin #7073

Closed

roland-d reviewed Jun 9, 2015
View reviewed changes

zero-24 added the RTC This Pull Request is Ready To Commit label Jun 9, 2015

wilsonge closed this Jun 14, 2015

wilsonge removed the RTC This Pull Request is Ready To Commit label Jun 14, 2015

zero-24 removed this from the Joomla! 3.4.2 milestone Jun 15, 2015

alikon deleted the patch-24 branch March 30, 2017 11:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle DB errors in categories search plugin #6710

Handle DB errors in categories search plugin #6710

alikon commented Apr 8, 2015

phproberto Apr 9, 2015

wilsonge Apr 9, 2015

mbabker Apr 9, 2015

phproberto Apr 9, 2015

phproberto Apr 9, 2015

wilsonge Apr 9, 2015

phproberto Apr 9, 2015

mbabker Apr 10, 2015

Bakual Apr 10, 2015

sovainfo commented Apr 10, 2015

mbabker commented Apr 10, 2015

sovainfo commented Apr 10, 2015

alikon commented Apr 10, 2015

wilsonge commented May 7, 2015

alikon commented May 10, 2015

wilsonge commented May 11, 2015

roland-d commented May 28, 2015

wilsonge commented May 28, 2015

roland-d Jun 9, 2015

roland-d commented Jun 9, 2015

alikon commented Jun 9, 2015

roland-d commented Jun 9, 2015

alikon commented Jun 9, 2015

coolcat-creations commented Jun 9, 2015

zero-24 commented Jun 9, 2015

wilsonge commented Jun 14, 2015

Handle DB errors in categories search plugin #6710

Handle DB errors in categories search plugin #6710

Conversation

alikon commented Apr 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sovainfo commented Apr 10, 2015

mbabker commented Apr 10, 2015

sovainfo commented Apr 10, 2015

alikon commented Apr 10, 2015

wilsonge commented May 7, 2015

alikon commented May 10, 2015

wilsonge commented May 11, 2015

roland-d commented May 28, 2015

wilsonge commented May 28, 2015

Choose a reason for hiding this comment

roland-d commented Jun 9, 2015

alikon commented Jun 9, 2015

roland-d commented Jun 9, 2015

alikon commented Jun 9, 2015

coolcat-creations commented Jun 9, 2015

zero-24 commented Jun 9, 2015

wilsonge commented Jun 14, 2015