Wikimedia Bugzilla is closed!

Created attachment 1282 [details]
Add green U marker to updated watchlist pages in recent changes/watchlist views.

A couple of clean-ups are included - removal of newFromCurRow and
loadFromCurRow functions from RecentChange.php as they are now no longer used
in any core code (nor code of any extensions within the CVS repository), and
update of the SQL of the query whose results were redundantly being routed
through these functions within SpecialWatchlist.php.

Comment on attachment 1282 [details]
Add green U marker to updated watchlist pages in recent changes/watchlist views.

Do not use this patch; it contains a SQL injection vulnerability on first
inspection.

Removing patch and needs-review keywords; failed security review. Haven't checked other aspects 
of patch at this time.

Created attachment 1285 [details]
Patch fixed to quote the title within the SQL insert query to prevent injection.

The clean-up part of the patch can be separated from the new functionality if
that makes it easier to review.

(In reply to comment #4)
 > The clean-up part of the patch can be separated from the new functionality if
> that makes it easier to review.

Please do, cleanup patches should be submitted seperately

Created attachment 1291 [details]
The update marker patch with cleanups separated out.

Created attachment 1292 [details]
The code cleanups - apply over previous patch.

Created attachment 1293 [details]
Make global notification logic consistent.

This patch makes the logic of $wgShowUpdateMarker and $wgEnotifWatchlist
consistent with that of UserMailer.php, line 274.

Created attachment 1294 [details]
A missing conversion to NULL from the previous zero for wl_notificationtimestamp.

Arguably this should be a new bug, but I don't see the point in creating one as
it's related to the previous patches.  Btw, why was the change from 0 to NULL
made in the first place?

Tom Gries incorrectly used a non-NULL value, assuming that timestamp columns were 
always VARCHAR. For whatever reason he still hadn't fixed this by the time his 
patches got applied to the 1.5 development branch, so it remained broken until I 
fixed up this code recently.

We use proper datetime columns for non-MySQL databases (PostgreSQL and Oracle) and 
will likely fix this on MySQL at some point as well.

> We use proper datetime columns for non-MySQL databases (PostgreSQL and Oracle)
> will likely fix this on MySQL at some point as well.

OK - presumably (I haven't checked) for the non-MySQL databases, zero can't be
stored into or selected from datetime columns (it can in MySQL).  Is MW policy
for columns to be non-nullable where possible?

Created attachment 1295 [details]
Deals with talk-page-notify missing watchlist entry and $wgEnotifWatchlist, $wgEnotifUserTalk simultaneously true

Since (at least) the fixes Brion mentions in his comment above, the logic of
the mailer has:
a) assumed that a watchlist entry already exists for users monitoring their own
talk page through the 'enotifusertalkpages' option - this assumption needn't be
true.  This patch removes that assumption by adding the watchlist entry if it
doesn't already exist - the other place that could be patched to fix this is
when saving user preferences, but it seems safer here
b) assumed that either $wgEnotifWatchlist or $wgEnotifUserTalk may be true, but
not both simultaneously.  There's no comment in e.g. DefaultSettings.php that
this restriction holds, and it's possible to deal gracefully with both of them
being true, which this patch attempts to do.

The patch applies on top of the previous patches and doesn't apply cleanly to
CVS code; let me know if a patch not depending on the previous patches would be
useful.

Created attachment 1298 [details]
Supercedes previous patch which had a minor logic error.

Are you sure you're looking at current CVS HEAD code? a) and b) both sound like you're 
referring to the code as of about October/November before I changed it.

I'm looking at revision 1.38, from the sourceforge anonymous-access cvs repository.

The logic (simplified and ignoring error-checking) is:

if ($wgEnotifWatchlist) then
   query for all users bar the editing user
else if ($wgEnotifUserTalk and this is a talk page) then 
   query for the talk-page user only

So if only $wgEnotifWatchlist is true and the user does not have a watch-list
entry, they won't get a notification.

Also if both conditions are true and the user does not have a watch-list entry,
they won't get a notification - which is what lead me to (b), but it's not
really an independent problem so let's drop that one (the patch isn't affected
either way, just the description of why it's useful).

In the above comment I meant to write "if only $wgEnotifUserTalk is true" rather
than "if only $wgEnotifWatchlist is true".

Created attachment 1299 [details]
Minimal-change patch to deal with missing talk page watches; supercedes both previous patches.

OK, after some thought, here's a minimal and less intimidating patch than the
first (the second patch was rushed and had a syntax error).  It does the
following:

* when $enotifusertalkpage is true and the user has set the
'enotifusertalkpages' option, make sure a watch exists by calling
$targetUser->addWatch( $title ); - this function results in an sql query using
"insert ignore" so it doesn't matter if there's already a watch
* we want to perform this regardless of whether $wgEnotifWatchlist is also
true, so change the elseif to an if, and to compensate, further in the second
if check for $wgEnotifWatchlist not true before setting $userCondition (a
cosmetic change is to use $enotifusertalkpage as the test condition rather than
the more complicated expression that it represents)
* when testing whether to send an email, for the precedence to be correct there
needs to be parentheses around the or condition (I've also removed redundant
parentheses around the isEmailConfirmed() test)
* we also need to set the notification timestamp when this is a user talk page
so $enotifusertalkpage has been added to the test

Brion, coming back to this after some time on other things, I find a line that I
missed the first time around in User::setNewtalk():
$this->addWatch( $this->getTalkPage() );
So yes, your changes did largely fix the issue after all, with one exception:
User::setNewtalk() is called within Article::editUpdates(), however the email
notification occurs within the call to RecentChange::notifyEdit() or
RecentChange::notifyNew() in Article::updateArticle() and
Article::insertNewArticle() respectively, both of which are called /prior/ to
Article::editUpdates() within those functions.

So on (at least) the very first occasion, the new talk flag will be set but the
email won't be sent.  This is what my testing showed up and why I reported the
bug, unfortunately I missed that you had dealt with subsequent occasions already.

The patch I submitted on 2006-01-12 13:53 UTC is still valid - it deals with the
problem by moving the precautionary adding of the talk page watch to the email
sending function (it doesn't remove the line I missed which could be removed if
the patch or similar were accepted); there no doubt are other solutions.  Also
as noted the patch depends on previous patches attached to this bug but it would
be simple to remove those dependencies.

Can you submit one unified diff? It is hard to tell what is going on there.

I'm really sorry Aaron, but MediaWiki feels like another world to me - it's been so long since I played with it.  I barely even remember submitting these patches, let alone having the energy and motivation to clarify them for you.  I'm involved in paid work right now, so let alone that I don't have the energy and motivation to chase them up, I can't *afford* to do it either.  I'm afraid that you're either going to have to sort through this on your own, or to dismiss it as unresolvable.  I do recommend that you take the former option, though, because - even though I don't remember the essence of this patch - I don't talk crap and I'm sure that the patch is worthwhile, if it will still apply to the current code.  Just the title of it makes sense to me - it would be great to see through a green "U" icon in your watchlist when a page has changed.  I expect that the code that I submitted achieves that aim, if you could be bothered to analyse it, even though I don't remember the specifics of it a couple of years later.  If not, then I don't imagine that it would be too hard to generate your own diff to achieve that aim.  I expect that you're a bright enough chap.  :-)

Changed component to "Special pages"

Note from the future (from the bug's creation perspective):

On the history-page of a page we can, anno 2011, see which edits were made after the user last looked at it. This is a feature existing atleast since MediaWiki 1.16.

Seems related to this bug, just saying :)

Same is in Watchlist as well ("Mark as read").