Title::newFromID() returns null if the main page has been deleted so this page would start throwing fatals if the main page is deleted. Either we could show nothing if the page does not exist or maybe we should store the main page as a namespace-title pair in the table? Personally, I think the latter is better.
Description
Details
Subject | Repo | Branch | Lines +/- | |
---|---|---|---|---|
Save a Namespace-Title pair of Newsletter, fixing fatal error later | mediawiki/extensions/Newsletter | master | +43 -10 |
Status | Subtype | Assigned | Task | ||
---|---|---|---|---|---|
Duplicate | Qgil | T125545 Phabricator Q&A session for Community Liaisons | |||
Resolved | Qgil | T116025 Goal: Align Community Liaison and Developer Relations project management practices | |||
Resolved | Qgil | T119387 Community Liaison and Developer Relation quarterly goals for January - March 2016 | |||
Declined | None | T104131 Exporting existing newsletter to the Newsletter extension | |||
Resolved | Addshore | T110170 Goal: Deploy Newsletter extension in Wikimedia | |||
Resolved | Qgil | T110366 Newsletter extension: Technical review | |||
Duplicate | None | T115098 Deploy Newsletter extension in beta cluster | |||
Resolved | ori | T127297 Add the Newsletter extension to the Beta Cluster | |||
Resolved | Bawolff | T115095 Security review of Newsletter extension | |||
Resolved | 01tonythomas | T119058 Special:Newsletter fatals if the newsletter main page is deleted |
Event Timeline
rude way of fixing this the former way ?
'default' => $mainTitle ? Linker::link( $mainTitle, htmlspecialchars( $mainTitle->getPrefixedText() ) ) : 'None' ,
Still exists today at http://newsletter-test.wmflabs.org/wiki/Special:Newsletter/14:
Exception encountered, of type "BadMethodCallException" [06a1fff2] /wiki/Special:Newsletter/14 BadMethodCallException from line 137 of /vagrant/mediawiki/extensions/Newsletter/includes/specials/SpecialNewsletter.php: Call to a member function getPrefixedText() on a non-object (NULL) Backtrace: #0 /vagrant/mediawiki/extensions/Newsletter/includes/specials/SpecialNewsletter.php(71): SpecialNewsletter->doViewExecute() #1 /vagrant/mediawiki/includes/specialpage/SpecialPage.php(407): SpecialNewsletter->execute(string) #2 /vagrant/mediawiki/includes/specialpage/SpecialPageFactory.php(565): SpecialPage->run(string) #3 /vagrant/mediawiki/includes/MediaWiki.php(280): SpecialPageFactory::executePath(Title, RequestContext) #4 /vagrant/mediawiki/includes/MediaWiki.php(736): MediaWiki->performRequest() #5 /vagrant/mediawiki/includes/MediaWiki.php(517): MediaWiki->main() #6 /vagrant/mediawiki/index.php(43): MediaWiki->run() #7 /var/www/w/index.php(5): include(string) #8 {main}
I agree. There are many reasons to have the title of the newsletter as an own value, detached from the title of the wiki page marked as main page for a newsletter.
Change 288399 had a related patch set uploaded (by 01tonythomas):
Save a Namespace-Title pair of Newsletter, fixing fatal error later
I actually think this idea is okay, but in this case on deletion of the main page of a newsletter a newsletter log entry should be added saying that the mainpage which was called XXXX has been deleted.
If we realllly do want to have to store the title, we also need to think about page moves, and we also should maybe store the namespace and title seperatyl. I guess this could even just be in a blob field as some JSON as we never want to query by it..?
The patch above proposes to add a field which stores the full title of the main page as a string (in addition to the currently existing id field). This doesn't seem right to me because it adds redundant data to the DB and would make how we handle it in code a little unconventional (which could also lead to other issues due to how we work with Title). I don't think we do that anywhere else in MW? IMO, the proper way to do this would be to store it as a namespace/title_key pair as stated in the description and to remove the id field. That would mean non-existent main pages can exist since it's no longer tied to an id.
Adding a newsletter log entry for that action doesn't seem like a great idea because it isn't related to the newsletter itself. I'll note that the fatal itself can be fixed pretty easily on this page but if we go forward, I'd like us to implement it in a non-hackish way since we don't know how (and where) it would be used in the future and also who knows, we might even have different usages for it.
If we realllly do want to have to store the title, we also need to think about page moves, and we also should maybe store the namespace and title seperatyl.
Handling page moves won't be that hard, right? We'd just need to add a subscriber to the hook executed after a page move is done (f we want it done automatically; maybe it shouldn't be automatic). We could also maybe add a little message to page move and page deletion interface saying that it's a newletter main page. Having a redlink after a page move also wouldn't be that bad because it'd show the log entry leading to the new page if no redirect was left in place.
I guess this could even just be in a blob field as some JSON as we never want to query by it..?
At least, we do a query when we check for duplicate main page existence when creating and updating main page through Special:CreateNewsletter and Special:Newsletter.
I think this is also a good opportunity to think more about this field (and figure out whether main page field is really needed). There may be an issue with our current implementation because both design feedback tasks (T124527/T116353) mentioned that this is a bit confusing. (T124527#1961041 proposes to remove it completely). I don't know what the issue is (and so, how to solve it). CC @MZMcBride and @violetto for further discussion. If we do remove it, we definitely should allow parsing links (at the very least) from the description (similar to edit summary and Flow topic headers) since there are several newsletter with "main pages" in use on Wikimedia. (eg. 1)
What about making the main page field optional? I expect most active newsletter to have a main page, but it is possible that users come up with more casual uses where a main page is beyond the point. Think of Tumblr vs Wordpress. :)
Change 288399 abandoned by 01tonythomas:
Save a Namespace-Title pair of Newsletter, fixing fatal error later
Reason:
Will get fixed after https://gerrit.wikimedia.org/r/#/c/295670/ :)
Do not occur as of now, as we are storing things more 'wiki' way with Content-Handler = yay!