Page MenuHomePhabricator

Improve SpecialUserStats.php - namespace selector
Closed, ResolvedPublic

Description

Author: alfred.maghi

Description:
statistics per namespace

Add the possibility to plot the statistics concerning Main namespace.


Version: unspecified
Severity: enhancement
URL: http://www.mediawiki.org/wiki/Extension:Usage_Statistics#Add-on:_statistics_per_namespace

attachment Add-on - statistics per namespace.patch ignored as obsolete

Details

Reference
bz19621

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 10:43 PM
bzimport set Reference to bz19621.
bzimport added a subscriber: Unknown Object (MLST).

alfred.maghi wrote:

Add to query (line159) " AND page_is_redirect=0 " to get rid of statistics on redirects.

alfred.maghi wrote:

(In reply to comment #1)

Add to query (line159) " AND page_is_redirect=0 " to get rid of statistics on
redirects.

Better adding: " AND page_is_redirect=0" in function DisplayForm line 387:

<option value='=0 AND page_is_redirect=0' selected>". wfMsg('usagestatisticsnsmain') ."

(In reply to comment #2)

(In reply to comment #1)

Add to query (line159) " AND page_is_redirect=0 " to get rid of statistics on
redirects.

Better adding: " AND page_is_redirect=0" in function DisplayForm line 387:

<option value='=0 AND page_is_redirect=0' selected>".
wfMsg('usagestatisticsnsmain') ."

That shouldn't work. If it does, that's an SQL injection vulnerability.

alfred.maghi wrote:

(In reply to comment #3)

That shouldn't work. If it does, that's an SQL injection vulnerability.

Is it not rather a selection than an injection; indeed that query does not change the DB:

'SELECT ... FROM ... WHERE rev_page=page_id AND page_namespace=0 AND page_is_redirect=0'

(In reply to comment #4)

(In reply to comment #3)

That shouldn't work. If it does, that's an SQL injection vulnerability.

Is it not rather a selection than an injection; indeed that query does not
change the DB:

'SELECT ... FROM ... WHERE rev_page=page_id AND page_namespace=0 AND
page_is_redirect=0'

I don't think you understand. It should not be possible to inject SQL through form fields EVER, or someone will find a way to exploit it. To fix this, you should use "AND page_namespace " . $db->addQuotes( $ns )

alfred.maghi wrote:

Patch corrected with no SQL through form fields

Correction thanks to Roan.

attachment Add-on - statistics per namespace.patch ignored as obsolete

alfred.maghi wrote:

alternative with addition of a message to inform users

attachment Alternative with message.patch ignored as obsolete

(In reply to comment #7)

Created an attachment (id=6330) [details]
alternative with addition of a message to inform users

These patches are still ugly as they pass >= or = as part of the namespace parameter. Instead, you should make the namespace parameter default to null and not set the page_namespace part of the query if it's null. If a namespace was selected, you should just pass the number, without the = sign.

alfred.maghi wrote:

Patch extension and extension messages. Correction thks to Roan

attachment Add-on - statistics per namespace.patch ignored as obsolete

IMHO, rather than hard-coding this to be "all NS or NS-0 only", why not use the namespace selector in the XML class? It's already designed for this and handles all namespaces.

alfred.maghi wrote:

(In reply to comment #10)

why not use the namespace selector in the XML class?

Because I didn't need statistics on other namespaces.

This could be an improvement, could you submit a patch or detail how namespace-selector looks like?

To patch, we should keep in mind that we need a "all NS" value in selection.

(In reply to comment #11)

(In reply to comment #10)

why not use the namespace selector in the XML class?

Because I didn't need statistics on other namespaces.

You may not, but while you're patching you might as well go for the better implementation and it'll be more helpful to everybody :)

This could be an improvement, could you submit a patch or detail how
namespace-selector looks like?

Check out the Xml::namespaceSelector() method.

To patch, we should keep in mind that we need a "all NS" value in selection.

namespaceSelector() could use a generic way to do this, which it doesn't currently do. Might be worth patching that in too.

alfred.maghi wrote:

Alternative patch: further improvement with namespaceSelector

(In reply to comment #12)

(In reply to comment #11)

(In reply to comment #10)

why not use the namespace selector in the XML class?

Because I didn't need statistics on other namespaces.

You may not, but while you're patching you might as well go for the better
implementation and it'll be more helpful to everybody :)

There is a patch using nsSelector... That alternative patch can be cleaned up.

attachment further improvement with namespaceSelector.patch ignored as obsolete

alfred.maghi wrote:

use addQuotes instead of strencode, plus other improvements thks to Roan.

attachment further improvement with namespaceSelector.patch ignored as obsolete

alfred.maghi wrote:

parent class in constructor

attachment Add-on - statistics per namespace.patch ignored as obsolete

  • JS file should be as an external file, use addScriptFile().
  • Use consistent function casing (addHTML vs AddHTML)
  • Use XML functions rather than building all HTML by hand

alfred.maghi wrote:

+casing correction

Attached:

alfred.maghi wrote:

is that use of XML functions wanted?

attachment Add-on - statistics per namespace.patch ignored as obsolete

alfred.maghi wrote:

further improvement with NikeRabbit

use class=mw-label , mw-input. instead of align=left..

Further use of XML::element instead of html.

colon put inside the message.

Attached:

alfred.maghi wrote:

FIXME: JS as an external file with $wgJSAutoloadClasses

CalendarPopup() is working. showNavigationDropdowns() is working.

FIXME: the onClick event is *not* working. [<a onClick=CalendarPopup.select()]

Attached: