? Success

User tests: Successful: Unsuccessful:

avatar digitalgarage
digitalgarage
13 Oct 2015

Hi Folks,

Currently the Joomla JLog class will default to using JLogLoggerMessagequeue class if a custom logger hasn't been added.

This means that by default something like: JLog::add("Some logging information here", JLog::INFO); will output to the message queue on the front end. This is done regardless of the error_reporting level defined in the Joomla main config. So for example even with error_reporting set to none in the Joomla Global Configuration->Server settings error logging done with JLog where a component hasn't defined an alternative logger will print to the message queue.

This PR adds a check to the JLogLoggerMessagequeue to ensure that the error reporting level is not none before continuing to log the error. This will help prevent logging statements appearing on the front end of a site as part of the message queue.

To Test

Ensure your error reporting in your Joomla config is set to none. Then add something like:

JLog::add("Should this be here", JLog::INFO);

to the top of your templates index.php file and reload the page.

Before this patch is applied the following will be displayed on the page refresh.

jlog-before

Now apply the patch and still with error reporting set to none refresh the page. The log message should now not be present on the site.

Comments

I have only tested for error_reporting set to none. Should this tests for other error_reporting levels? If so how should we map the JLog levels to the Joomla config levels?

Cheers,
Eric.

avatar digitalgarage digitalgarage - open - 13 Oct 2015
avatar digitalgarage digitalgarage - change - 13 Oct 2015
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 13 Oct 2015
Labels Added: ?
avatar mbabker
mbabker - comment - 14 Oct 2015

I don't think this is right at all. The error reporting level set in the
global config doesn't control the items that get queued for messages.
Also, IIRC, the default configuration is to use that logger only for
"jerror" category messages.

On Tuesday, October 13, 2015, Eric notifications@github.com wrote:

Hi Folks,

Currently the Joomla JLog class will default to using
JLogLoggerMessagequeue class if a custom logger hasn't been added.

This means that by default something like: JLog::add("Some logging
information here", JLog::INFO); will output to the message queue on the
front end. This is done regardless of the error_reporting level defined in
the Joomla main config. So for example even with error_reporting set to
none in the Joomla Global Configuration->Server settings error logging done
with JLog where a component hasn't defined an alternative logger will print
to the message queue.

This PR adds a check to the JLogLoggerMessagequeue to ensure that the
error reporting level is not none before continuing to log the error. This
will help prevent logging statements appearing on the front end of a site
as part of the message queue.
To Test

Ensure your error reporting in your Joomla config is set to none. Then add
something like:

JLog::add("Should this be here", JLog::INFO);

to the top of your templates index.php file and reload the page.

Before this patch is applied the following will be displayed on the page
refresh.

[image: jlog-before]
https://cloud.githubusercontent.com/assets/9042878/10471415/ff461ffc-7258-11e5-8c21-cdaa7e0b4dd5.png

Now apply the patch and still with error reporting set to none refresh the
page. The log message should now not be present on the site.
Comments

I have only tested for error_reporting set to none. Should this tests for
other error_reporting levels? If so how should we map the JLog levels to
the Joomla config levels?

Cheers,

Eric.

You can view, comment on, or merge this pull request online at:

#8087
Commit Summary

  • Checks J config error_reporting before logging

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#8087.

avatar digitalgarage
digitalgarage - comment - 14 Oct 2015

@mbabker at least in staging at present the JLogLoggerMessagequeue class is sending ALL the JLog error levels to the message queue.

            case JLog::EMERGENCY:
            case JLog::ALERT:
            case JLog::CRITICAL:
            case JLog::ERROR:
                JFactory::getApplication()->enqueueMessage($entry->message, 'error');
                break;
            case JLog::WARNING:
                JFactory::getApplication()->enqueueMessage($entry->message, 'warning');
                break;
            case JLog::NOTICE:
                JFactory::getApplication()->enqueueMessage($entry->message, 'notice');
                break;
            case JLog::INFO:
                JFactory::getApplication()->enqueueMessage($entry->message, 'message');
                break;

That was just taken out of the JLogLoggerMessagequeue.

The test I did was using JLog::INFO and that was being displayed in the message queue. The PR won't change adding items to the messagequeue. It will just change adding items to the messagequeue via JLog.

Eric

avatar mbabker
mbabker - comment - 14 Oct 2015

At https://github.com/joomla/joomla-cms/blob/3.4.4/libraries/cms.php#L58 the message queue is only set up for log items of category "jerror". Whether this is right or not can be debated, but essentially anything that gets logged in that category will also be pushed to the message queue (consider it a poor man's shortcut for logging an error and displaying a message to the user in one call). By design, this JLog call will hit the message queue while the log statements in JDatabaseDriverMysqli::execute() will not forward to the message queue because it is the incorrect category. Since JLog::add() does not have a default category, I'm not following what issue you're trying to address since your given examples should not be matching the message queue logger's criteria.

As for the error_reporting configuration value, its only use is to set the PHP error level reporting (see https://github.com/joomla/joomla-cms/blob/3.4.4/includes/framework.php#L51-L90 for that). That setting does not correspond to what error levels are logged or what messages are enqueued and displayed by the application.

avatar digitalgarage
digitalgarage - comment - 14 Oct 2015

Hey Michael, I understand what you are saying and according to the docs what you are saying is how it should be. (https://docs.joomla.org/Using_JLog).

But.... The issue is that without specifying a category when calling JLog::add the entry is being added to the messagequeue regardless of priority.

I've attached three screenshots where I've used:

  • JLog::add("Should this be here", JLog::INFO);
  • JLog::add("Should this be here", JLog::WARNING);
  • JLog::add("Should this be here", JLog::ERROR);

So this PR is trying to go about this the wrong. Maybe what I should have said, and where I should be looking is that calling JLog::add WITHOUT a category is still displaying the message to the user, despite the docs saying that this shouldn't happen.ie. the docs say that the message should ONLY be displayed to the user when jerror is adding to the category.

I've tested this on both a current J3.4.3 install and against the staging branch from git. For both I have added JLog::add to the template's index.php file without a category and the message is displayed to the user.

Can you maybe just do me a quick favour and just double check if the same thing happens to you? ie. if you add JLog::add("This message shouldn't show", JLog::ERROR) to your templates index.php file do you get the same thing?

Eric.

error-log
info-log
jlog-before

avatar zero-24 zero-24 - change - 14 Oct 2015
Category Libraries
avatar zero-24 zero-24 - change - 14 Oct 2015
Easy No Yes
avatar mbabker
mbabker - comment - 14 Oct 2015

It does that but your approach only places a bandaid on the solution and doesn't actually fix anything long term. When you set no category, this condition is met and the message is queued for entry to the logs. So your bug isn't specific to the message queue logger but a bug in the base JLog object which would affect all log types (the only reason you see it with the message queue logger is because of our adding the message queue logger to the bootstrap).

avatar digitalgarage
digitalgarage - comment - 14 Oct 2015

Hi Michael

Agreed. I was looking in the wrong place and you are absolutely right it is just a band aid solution. I didn't dig deep enough.

So we are both on the same page and this shouldn't be happening and i will review the JLog class for a better fix.

Does that sound like a better idea?

Eric.

avatar mbabker
mbabker - comment - 14 Oct 2015

Now that I traced it out I see where you're hitting the issue and it's indeed a bug.

avatar digitalgarage
digitalgarage - comment - 14 Oct 2015

Thanks i didn't dig deep enough when i opened this.

When i get to work i will close this and open a more descriptive issue through the issue tracker.

Eric.

avatar digitalgarage
digitalgarage - comment - 14 Oct 2015

Moved to better home on Issue #8094

avatar digitalgarage digitalgarage - change - 14 Oct 2015
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2015-10-14 23:51:38
Closed_By digitalgarage
avatar digitalgarage digitalgarage - close - 14 Oct 2015

Add a Comment

Login with GitHub to post a comment