r111085 MediaWiki - Code Review archive

+	 * @param Title $title

Should be in order @param $title Title. Also it is misleading since the function also expects null/false.

Doxygen understands both - unlike NetBeans which I use.

It's inconsistent. Although I kind of like that syntax more than the unreadable @param $title title title of the page.

In my testing, when it cleans up watchlist page entries, for a normal (non-raw) edit, it doesn't properly delete the bad entries for the corresponding talk page.

For example, I added some invalid stuff to my watchlist:

mysql> select wl_namespace, concat( "[", wl_title, "]" ) from watchlist;
+--------------+------------------------------+
| wl_namespace | concat( "[", wl_title, "]" ) |
+--------------+------------------------------+
|            0 | [spacing cha R2]           | 
|            0 | [spacing cha r]            | 
|            1 | [spacing cha r]            | 
|           82 | [ns 82]                      | 
|           83 | [ns 82]                      | 
+--------------+------------------------------+

After doing an edit to it, the entries looked like:

mysql> select wl_namespace, concat( "[", wl_title, "]" ) from watchlist;
+--------------+------------------------------+
| wl_namespace | concat( "[", wl_title, "]" ) |
+--------------+------------------------------+
|            0 | [ns_82]                      | 
|            0 | [spacing_cha_R2]             | 
|            0 | [spacing_cha_r]              | 
|            1 | [ns_82]                      | 
|            1 | [spacing cha r]            | 
|            1 | [spacing_cha_R2]             | 
|            1 | [spacing_cha_r]              | 
|           83 | [ns 82]                      | 
+--------------+------------------------------+

Where they should have looked like:

mysql> select wl_namespace, concat( "[", wl_title, "]" ) from watchlist;
+--------------+------------------------------+
| wl_namespace | concat( "[", wl_title, "]" ) |
+--------------+------------------------------+
|            0 | [ns_82]                      | 
|            0 | [spacing_cha_R2]             | 
|            0 | [spacing_cha_r]              | 
|            1 | [ns_82]                      | 
|            1 | [spacing_cha_R2]             | 
|            1 | [spacing_cha_r]              | 
+--------------+------------------------------+

This issue does not seem to occour when doing raw edits. (And its really not a major issue, db looks wrong, but they're not displayed to user, so should be ok).

Additionally, it seems like the entries that are removed from watchlist as invalid sometimes still end up on the edit page (but won't if you reload the page). This seems to be true for the raw view for all types of entries, and for the normal edit mode, this is true only for my article in namespace 82.

(For reference, I should say i have no namespace 82 defined)

These are rather minor issues though, and certainly much better than the old behaviour.

if ( $this->checkTitle( $title, $row->wl_namespace, $row->wl_title )
+					&& !$title->isTalkPage()
+				) {

That isTalkPage should probably be part of the checkTitle() method.

User->cleanupWatchlist() should be properly deprecated for back compatibility (it is a public function).

A new class property probably deserve a new comment :-)

+	private $badItems = array();

In cleanupWatchlist() the foreach on badTitle seems to be inserting those bad titles back in the DB:

if ( $title ) {
+				$this->getUser()->addWatch( $title );
+			}

> User->cleanupWatchlist() should be properly deprecated for back compatibility (it is a public function).

But it had no callers whatsoever save this code that was there just for one version.

> In cleanupWatchlist() the foreach on badTitle seems to be inserting those bad titles back in the DB

This code path is for updating of titles normalised by Title::secureAndSplit(), what's your definition of 'bad'?

> > User->cleanupWatchlist() should be properly deprecated for back compatibility (it is a public function). > But it had no callers whatsoever save this code that was there just for one version.

It still deserves a proper deprecation in my opinion. Just in case someone decided to use it with 1.18 :-( But maybe I am just too paranoid.

> This code path is for updating of titles normalised by Title::secureAndSplit(), what's your definition of 'bad'?

Oh the bad title array contains a safe title. Guess that is just misleading variable names. Maybe renames $title to $safeTitle.

- if ( $namespace < 0 ) {
- $haveInvalidNamespaces = true;
- continue;

You replaced this with an "if", but it no longer breaks out before the inner loop on $pages.

This is OK with the checkTitle() below.

Overall, this is OK. I don't like the object state changes in checkTitle(), it should take an argument by reference that builds the list rather than mutating some weird member variable.

From bawolf's comments, this still needs some work to finish the job, but the commit is a working improvement.