Page MenuHomePhabricator

Patchlet 1: ENotif code clean-up patch for REL1_5 - revisited
Closed, InvalidPublic

Description

Patchlet number 1 (for issues in collective bugzilla 454).

  • moved the notification calls to module RecentChange
  • fully reestablished memcache-efficient user_newtalk table,but only for the newmessage marker.
  • it works now that way:

when someone writes on another user's X talk page, Article.php creates an watchlist entry for this user_talk:X (and
the user-page) on-the-fly now (calls addwatch for it), whereas the immediately following editupdate call triggers
sending the ENotif via the hook in RecentChange. This is safe and clean now.

  • needed to move two RecentChange calls in Article just behind the editupdates calls to make that working; I did

not notice any problem with that move

  • TimestampOrNull slightly modified, but still database independent
  • updaters.inc, tables.sql and maintenance/patch-create-user_newtalk.sql supplied with the patch
  • WatchedItem:duplicatEntries now copies both pages (subject/talk) when a page is moved

Version: 1.5.x
Severity: normal

Details

Reference
bz2014

Event Timeline

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

Created attachment 476
ENotif code clean-up; reborn user_newtalk;

I forgot to mention: Module UserTalkUpdate.php dropped finally.

attachment cvs.diff ignored as obsolete

Created attachment 477
sql fixlet

attachment patch-drop-user_newtalk.sql ignored as obsolete

Brion: is the way how I contributed this okay ?

Comment on attachment 477
sql fixlet

i submitted the wrong sql fixlet

Created attachment 478
maintenance/archives/patch-create-user_newtalk.sql

The correct re-creator script

attachment patch-create-user_newtalk.sql ignored as obsolete

forgot also to mention this:

  • fixes also the problem of not deleting "You have new messages"

Created attachment 479
patchlet 1 bugfix 2014 (no changes; just the diff against todays CVS)

CVS REL1_5 as of 29.04.2005 18:50 UTC incl. this patch:

full directory /phase3 incl. CVS tag files can be downloaded from
http://www.wikinaut.de/mw/cvs15+bugfix2014.tgz

attachment cvs.diff ignored as obsolete

Comment on attachment 476
ENotif code clean-up; reborn user_newtalk;

superseded by same patch against newer CVS

This is a kind reminder to bring this into CVS HEAD, please.

It patches several bugs in current ENotif (e.g. non-deleting of "You have new
messages"). and allows further improvments in smaller steps.
On request, I can submit a fresh "diff" file against CVS.

If needed, the full directory /phase3 incl. CVS tag files can be downloaded from
http://www.wikinaut.de/mw/cvs15+bugfix2014.tgz

next patchlet (enotif for new pages is waiting)

bug: ENotifs were sent also to unconfirmed addresses :-((
bugfix: add two parenthesis in UserMailer.php :-))

Excerpt:

  • if ( ( $enotifwatchlistpage &&

$watchingUser->getOption('enotifwatchlistpages') ) ||

  • ( $enotifusertalkpage && $watchingUser->getOption('enotifusertalkpages') )

+ if ( ( ( $enotifwatchlistpage &&
$watchingUser->getOption('enotifwatchlistpages') ) ||
+ ( $enotifusertalkpage && $watchingUser->getOption('enotifusertalkpages') ) )

Whole patch: not available here.
Consult download section http://meta.wikipedia.org/wiki/Enotif

Tim, as promised during Wikimania and Brion (and Hashar, Avar, Duesentrieb),

here comes my _clean_ patch against CVS REL1_5 of today which also fixes 1876 (new message also for _User_ page
changes) and 2825 (new message not shown for anons).

Let me explain, what you can expect and how it works.

  1. UserTalkUpdate.php --> drop this file finally (it is _not_ needed any longer)
  2. updaters.inc --> drop the code of copy_newtalk_to_watchlist (it is _not_ needed any longer)
  3. The whole business for newmessage flag handling for _user_ and user_talk page is still done by the _one_(!) entry

in newtalk and memchache for efficiency reasons. Only when the owner of the user page visits his page, the watchlist
table entry is asked to distinguish the three possible cases (user, user_talk or both pages changed). This reuslts in
three different texts in the marker.

  1. When a user or user_talk page is edited by someone else, then editupdate() in Article.php calls addWatch() without

further user action and adds automatically that page to the watchlist of the user-page owner of course. This is needed
to store information, which one of the both pages has been changed. It is also used for sending out enotifs, but the
former aspect is the most essential one here.

  1. the two calls to enotif modules are moved to RecentChange.php, where they logically belong to.
  2. ++ no other things in that patch ++

I would like to ask you to have look to this clean patch now and ask to commit as soon as you can to REL1_5 and to CVS
HEAD.

Created attachment 784
CLEAN patch for enotif code clean up and user-page newmessage flag

attachment CVS-REL1_5.diff ignored as obsolete

Created attachment 785
CLEAN patch for enotif code clean-up and user-page newmessage flag plus cosmetics in UserMailer

CLEAN patch for enotif code clean-up and user-page newmessage flag plus
cosmetics in UserMailer

attachment CVS-REL1_5.diff ignored as obsolete

This doesn't seem to be a cleanup patch, but rather introduces some kind of new
feature where the function of the new messages notification is radically different.

Closing as WONTFIX for now as this doesn't seem to do as advertised; please reopen if
you have a cleanup patch that doesn't introduce large, controversial feature changes.

For future reference: try not to make variable and function names quite so long.
When they get to extremes like $wgShowNewtalkForUserOrUserTalkPage and
$wgUser->checkNotificationPendingForArticleOrTalk() they harm readability.

(In reply to comment #14)

This .. introduces some kind of new
feature where the function of the new messages notification is radically

different.
It still is linked to http://bugzilla.wikipedia.org/show_bug.cgi?id=1876 which
it solves.

My decision to do so was the following, please allow me this defensive comment:

The http://bugzilla.wikipedia.org/show_bug.cgi?id=1876 issue was discussed
during your absence in the irc last week and very positively welcomed in the irc
(i.a. Avar, Hashar, Duesentrieb) and there wasn't any objection, that I decided
not to remove this sub-issue and to publish patchlet 1 - revisited with that
feature built-in.

The reason is also, that I use *the same* memcache bit as re-introduced by Tim
Starling, which is efficient.

I agree on better having split-off that issue, if there wasn't the
wholeheartedly welcome to my "you have new message on your user page" question
in the irc.

Brion:
Would you patch, if I removed the 1876 issue from 2014 patch ?

(In reply to comment #14)

For future reference: try not to make variable and function names quite so long.
When they get to extremes like $wgShowNewtalkForUserOrUserTalkPage and
$wgUser->checkNotificationPendingForArticleOrTalk() they harm readability.

I used these ultralong names with the intention to help you and the rest of the
developers who usually substitutes them by names in conformity with the general
MediaWiki naming schemes.

I will duly consider to follow your advise, if I publish the one or other patch.

Created attachment 822
renewed clean patch _without_ introducing "newmessage flag for user_page changes"

I removed the non-enotif related extension (newmessage marker also shown for
changes on user pages) as requested; there is still one long variable name in
it. Brion, pls. rename at your discretion.

attachment CVS-REL1_5.diff ignored as obsolete

Here comes my (now) clean patch against CVS REL1_5 of today
Let me explain, what you can expect and how it works.

  1. UserTalkUpdate.php --> drop this file finally. This file is not needed any longer.
  2. updaters.inc --> drop the code of copy_newtalk_to_watchlist. This is not needed any longer.
  3. The whole business for newmessage flag handling is done by the one entry in newtalk and memchache for efficiency

reasons.

  1. When a user_talk page is edited by someone else, then editupdate() in Article.php calls addWatch() without

further user action and adds automatically that page to the watchlist of the user-page owner of course.

  1. the two calls to enotif modules are moved to RecentChange.php, where they logically belong to.

Created attachment 823
renewed clean patch _without_ introducing "newmessage flag for user_page changes"

removed a not-used subroutine in User.php

Attached:

(In reply to comment #19)

Created an attachment (id=823) [edit]renewed clean patch _without_ introducing "newmessage flag for

user_pagechanges"removed a not-used subroutine in User.php

Addendum

Brion, the instruction

$other->addWatch( $this->mTitle );

in about line 2123 in Article.php is _not_ needed, I forgot to remove that line.

[ There is no need to add user_talk page X automatically to the watchlist of the user X; this was only necessary
within my withdrawn extension. I'll upload a corrected patch without that line in a couple of hours. ]

(In reply to comment #20 correction

$other->addWatch( $this->mTitle ); in about line 2123 in Article.php _is_ needed

because it actually adds the changed user_talk page X to the watchlist of user X so that an e-notif can be sent on
that event. The code as published in the patch is correct and reestablishes "send me an e-mail if someone changes my
talk page" (if the user has checked that box in Special:Preferences i.e. opted-in).

Report any actual bugs separately so they can be fixed against current code.