Page MenuHomePhabricator

MassMessage: Bugs related to contenthandler branch (tracking)
Closed, ResolvedPublic

Description

Tracking bug for bug 61255, and things in the "contenthandler" branch.


Version: unspecified
Severity: normal
URL: http://mm-ch.wmflabs.org/wiki/Main_Page

Details

Reference
bz68599

Event Timeline

bzimport raised the priority of this task from to Needs Triage.Nov 22 2014, 3:35 AM
bzimport set Reference to bz68599.
bzimport added a subscriber: Unknown Object (MLST).
Legoktm created this task.Jul 26 2014, 3:17 AM

Just dumping notes here 'cause I'm lazy and don't want to lose them.


  • Fix creation edit summary
  • Categorise all delivery lists (addTrackingCategory?)
  • Kill add pages header
  • Link back to list
  • Fix instructions on special page and don't show technical details for single wiki
  • Add copyright disclaimer (see Special:MassMessage)
  • Mention protected status on the special page
  • Block interwiki titles (Title::isExternal) and special pages
  • Escape wikitext in invalid targets error (wfEscapeWikitext) ---

Additional thoughts I had:

  • edit notices? blergh
  • icons instead of words such as "(remove)" might be cleaner; dunno
  • still think the "Add pages" header should be killed; would like to discuss further
  • it's a bit weird to see "Pages in this list" as an h2 without a section-edit link
    • if the "Add pages" header is kept, it should maybe have a section-edit type link that reads "[bulk edit]" or similar
  • make "(remove)" less dangerous

I'm largely done fixing the things in MZMcBride's comment. Some comments about the ones that I have not implemented as described:

  • Add pages form / header: Form moved to before the page list per IRC discussion (prevents people having to scroll down to get to it on long lists)
  • Icons instead of words such as "(remove)": I've generally avoided adding custom design elements in order to ensure compatibility with skins other than Vector; if others also feel that an icon here would be better, this can probably be done.
  • Section edit links: Personally I don't think they make sense in this context...
Reedy added a comment.Sep 6 2014, 12:06 AM

https://github.com/wikimedia/mediawiki-extensions-MassMessage/compare/contenthandler

So looking at MassMessagehooks.php

$out->addModuleStyles( 'ext.MassMessage.content' );
$out->addModules( 'ext.MassMessage.content.js' );

A module ending in .js just feels icky. That's alright though, as the 2 should be merged, and then we can just have:

$out->addModules( 'ext.MassMessage.content' );

Api Modules:
getPossibleErrors is no more.

Other:
There are usages of "else if" in php code. Don't do that. Use "elseif"

(In reply to Sam Reed (reedy) from comment #3)

https://github.com/wikimedia/mediawiki-extensions-MassMessage/compare/
contenthandler
So looking at MassMessagehooks.php
$out->addModuleStyles( 'ext.MassMessage.content' );
$out->addModules( 'ext.MassMessage.content.js' );
A module ending in .js just feels icky. That's alright though, as the 2
should be merged, and then we can just have:
$out->addModules( 'ext.MassMessage.content' );

We have to do that so the CSS styles are still served to users with JS disabled. Modules loaded via addModules require JS. Also there would be a FOUC iirc.

Reedy added a comment.Sep 6 2014, 12:15 AM

SpecialCreateMassMessageList and SpecialEditMassMessageList doesn't return something on all code paths in their onSubmit()

MassMessageHooks.php - Line 192
$revId = ( $next ) ? $next : $revId;

$revId on the right hand side is undefined

ApiEditMassMessageList::getEditSummary() - $title and $count used at the bottom may have not been defined (I note there's quite a few code paths through that block...)

MassMessageListContentHandler::unserializeContent() should have @throws MWContentSerializationException

(In reply to Sam Reed (reedy) from comment #3)

Api Modules:
getPossibleErrors is no more.

Id74b59113cf0211e83c25364a94c597cb579b8f0

Other:
There are usages of "else if" in php code. Don't do that. Use "elseif"

I0f637f466b7c41053c9ba0a8d9399292c843104b

(In reply to Sam Reed (reedy) from comment #5)

MassMessageListContentHandler::unserializeContent() should have @throws
MWContentSerializationException

I62c73abd5f9e3fd4c2e9358e484101b962cdc6c0

(In reply to Sam Reed (reedy) from comment #5)

SpecialCreateMassMessageList and SpecialEditMassMessageList doesn't return
something on all code paths in their onSubmit()

I3a06dd743ff8b18aaba73649cbd53d4db02018ce

ApiEditMassMessageList::getEditSummary() - $title and $count used at the
bottom may have not been defined (I note there's quite a few code paths
through that block...)

AFAIS everything is defined properly, but explicitly done in I3393a7a5531c272cf879bad0da11426714a0e633.

(In reply to Sam Reed (reedy) from comment #5)

MassMessageHooks.php - Line 192
$revId = ( $next ) ? $next : $revId;
$revId on the right hand side is undefined

I think this should be $oldid, but I'm not totally sure...wctaiwan?

(In reply to Kunal Mehta (Legoktm) from comment #8)

(In reply to Sam Reed (reedy) from comment #5)

MassMessageHooks.php - Line 192
$revId = ( $next ) ? $next : $revId;
$revId on the right hand side is undefined

I think this should be $oldid, but I'm not totally sure...wctaiwan?

$oldid is correct; sorry. Changed in I75cb898dcd20db8052688152d55a8f3d63e9e241

I7e01e63717b99fe6937fa8598fdaffe8fdfe4527 closes a couple more loopholes for invalid titles, with thanks to MZMcBride for raising them.

Marking as fixed as mm-ch is now in master.