Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle DB errors in categories search plugin #6710

Closed
wants to merge 3 commits into from
Closed

Handle DB errors in categories search plugin #6710

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5a3328c
Handle DB errors in categories search plugin
alikon Apr 8, 2015
b17a5ff
travis
alikon Apr 9, 2015
9114788
Update categories.php
alikon May 10, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion plugins/search/categories/categories.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,15 @@ public function onContentSearch($text, $phrase = '', $ordering = '', $areas = nu
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
$rows = $db->loadObjectList();
}
catch (RuntimeException $e)
{
$rows = array();
JFactory::getApplication()->enqueueMessage($e->getMessage(), 'error');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JDatabaseDriver already has logging mechanisms in place...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

log_databasequery

Is there a better way to do this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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');
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

$return = array();

Expand Down