Page MenuHomePhabricator

ResourceLoader: wfDebugLog should bypass wfIsDebugRawPage() filter
Closed, ResolvedPublic

Description

e.g. wgDebugLog( 'resourceloader', .. );

Details

Reference
bz47960

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 1:29 AM
bzimport set Reference to bz47960.
bzimport added a subscriber: Unknown Object (MLST).

It seems that it already does bypass wfIsDebugRawPage(), if the relevant key in $wgDebugLogGroups is set. That's all I thought you were asking for: you don't want it to put debug lines from RL into $wgDebugLogFile when $wgDebugRawPage is false, do you?

Well..

Previously ResourceLoader was using almost no "wgDebugLog( 'resourceloader', .. );" (it had 0 calls other than 1 single generic wfDebug call), I fixed that a few weeks ago by adding calls to wgDebugLog( 'resourceloader', .. ); and changing others to use the same pattern.

However that means I still only get those log entries if I explicitly set up $wgDebugLogGroups in local settings for 'resourceloader'. I can do that locally but I don't like having to tell others do that as well all-the-time whenever they run into a problem that turns out to be caused by update a file path somewhere in a module definition. As far as I know all errors in the 'resourceloader' group are major errors of the type "that must never reach master". Meaning, it doesn't makes sense for them to be optional (e.g. only when setting wgDebugRawPage=true or setting up a custom log group file for it).

I don't see why they shouldn't go to the general log like everything else does by default.

The only reason they're excluded is because wfIsDebugRawPage() returns true for load.php instead of just for action=raw. I suspect the original rationale for that was to avoid clogging up the log with duplicate errors that would emit on both the main request and the request for action=raw (gadgets, site scripts, user scripts).

However I don't think it makes sense for load.php since the type of errors emitted from there are almost entirely from a completely separate code path (the only common thing is WebStart).

I originally proposed to remove the load.php condition from wfIsDebugRawPage() but Tim Starling suggested that we instead have wgDebugLog() bypass wfIsDebugRawPage() (thus keeping only wfDebug() behind that conditional).

[Assignee was removed, hence also resetting ASSIGNED status]

Right now [resourceloader] entries get logged to the $wgDebugLog file but not on the MWDebug console, $wgShowDebug HTML body or $wgDebugComments HTML comment.

I think this is okay, given that I can turn off [resourceloader] entries with

$wgDebugLogGroups = array(

'resourceloader' => false,

);

Not sure I understand this bug report but I think it is working now as it should.

Krinkle claimed this task.

Indeed. This behaviour was changed by 28a7ce420bd9. Which changed wfDebugLog to use MWLogger instead of wfDebug. And the wfIsDebugRawPage() conditional was in wfDebug(), but is not in MWLogger.

Fixed :)

Krinkle set Security to None.
Krinkle edited subscribers, added: tstarling, bd808; removed: Unknown Object (MLST).