r68613 MediaWiki - Code Review archive

Next to CountEdits, and Editcount, mw-editcount is now extension #3 that implements an edit counter as a special page. Not sure if that's a great idea. Would you be willing to have a look at all three of them, and combine all their great features into one of the two existing ones? After that, we can retire the two remaining ones, and have one excellenly maintained special page for edit counts...

Each one of them seems to fail in basic code quality, having raw sql, constructing html manually or not doing i18n properly.

Even if you managed to combine and create one super extension, you would need to get the end users to change over to using it, these are in here because its shown that people are using them, Its better to have several crappy ones that get little care, to ones that aren't in svn and lead to people using all types of random hacks to get them to work, at least in SVN they get a basic level of care, and if people desire to submit patches (for example in BZ) we can get them applied.

1. The directory name is uncommon. I would suggest to rename it to EditCount, but ...

2. We already have an extension EditCount. What is the difference?

1. That was the name of the original plugin at sourceforge and EditCount was already taken, It was imported from SF because it was shown that people were using it and wasn't being maintained which lead to people being told to enter crappy code to work around.

2. We already have many plugins that duplicate functions of other ones, But we may as well have several crappy coded ones in svn, compared to several random crappy ones that are no longer maintained, get no love interest and that guide people to re-add deprecated code back in to make them work

1. Re: perfectly good reason to report a few bugs in its bug tracker for it, and mail the maintainer to get his project closed down, or advertise usage of another similar extensions.
2. Re: That's not a reason to not change that and continue adding more duplication. Committers should take their responsibility or stop being a committer.

Suggestion removal of at least extensions/mw-editcount as soon as possible because of duplicated extension functionality.

All three extensions exist, all three are being used... Even if this extension is deleted from svn people are still going to use it, they've been using it for awhile already. If you delete it from svn people will go back to using the garbage unmaintained version on SourceForge with bad ?> bugs, bad UTF-8, and uses depreciated code that causes people to add garbage to their wiki. Moving it here to svn at least gives it a chance to be maintained in terms of code quality. And makes it possible for anyone interested to fix it up.

Someone interested is of course free to write an EditCount plugin that works better than all of them and convince everyone to move to use of it. But till then removing any of these from svn doesn't do anyone any good.

Then we should mark the unmaintained extensions as such on mw.org, and provide a link to alternative that works. Triplication (heh!) still makes no sense.

+1 for removal. If people are using it, they can use it from SF. No need to duplicate more functionality in our repo.

Since it's here, I might as well review it:

+	function EditCountPage()
+	{
+		parent::SpecialPage("EditCount", "", true);
+	}

Should be function __construct() and call parent::__construct().

+	function execute($par = null) 

You don't need the default value, it won't match the parent signature and will throw warnings on E_STRICT. Thus, this:

$this->target = ($par !== null) ? $par : $wgRequest->getVal("target");

Could just be $this->target = !$par ? $par ...

+            wfProfileOut(__METHOD__);

Indentations are wrong, here and elsewhere.

+		$nt = Title::newFromURL($this->target);

From Title::newFromURL's docs: THIS IS NOT THE FUNCTION YOU WANT. Use Title::newFromText().

+		$table .= wfElement("table", array("style" => "$this->border text-align: center; margin-left: 25%; margin-right: auto; margin-top: 7px; border-collapse: collapse"), null, null);

None of wfElement() and friends exist anymore. Should use Xml or Html static methods as appropriate.

+		$cond = ($this->user->isAnon()) ? "rev_user_text = '" . $this->user->getName() . "'" 
+			: "rev_user = " . $this->id;
+		$result = $this->db->query("SELECT page_namespace as ns, COUNT(*) as count
+			FROM {$wgDBprefix}revision JOIN {$wgDBprefix}page p ON rev_page = p.page_id
+			WHERE $cond GROUP BY ns", __METHOD__);

Gluing SQL together is error prone and not secure.

+		$index = Namespace::getCanonicalIndex(strtolower($param2));

Namespace is now called MWNamespace, and this will yell at you in 5.3+ anyway since Namespace is reserved.

Index: trunk/extensions/mw-editcount/.project

Generally we don't check in .project files.

This is an ancient extension. It contains a lot of back-compat hacks for long-since deprecated (and in some cases, removed) interfaces. Portions of it will throw fatal errors, warnings and other goodies.