Page MenuHomePhabricator

Special:Newsletter fatals if the newsletter main page is deleted
Closed, ResolvedPublic

Description

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.

Details

Related Gerrit Patches:

Event Timeline

Glaisher raised the priority of this task from to Needs Triage.
Glaisher updated the task description. (Show Details)
Glaisher added a subscriber: Glaisher.
Restricted Application added subscribers: StudiesWorld, Aklapper. · View Herald TranscriptNov 19 2015, 12:41 PM
Glaisher triaged this task as High priority.Dec 14 2015, 8:39 AM

rude way of fixing this the former way ?

'default' => $mainTitle ? Linker::link( $mainTitle, htmlspecialchars( $mainTitle->getPrefixedText() ) ) : 'None' ,

I would prefer that we store the title rather than doing that.

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}
Qgil added a subscriber: Qgil.Feb 15 2016, 10:02 AM

I would prefer that we store the title rather than doing that.

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.

I would prefer that we store the title rather than doing that.

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.

we have the 'Newsletter Name' for that ?

Tinaj1234 removed Tinaj1234 as the assignee of this task.Apr 21 2016, 7:44 AM
Tinaj1234 added a subscriber: Tinaj1234.

Change 288399 had a related patch set uploaded (by 01tonythomas):
Save a Namespace-Title pair of Newsletter, fixing fatal error later

https://gerrit.wikimedia.org/r/288399

rude way of fixing this the former way ?

'default' => $mainTitle ? Linker::link( $mainTitle, htmlspecialchars( $mainTitle->getPrefixedText() ) ) : 'None' ,

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.

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.

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)

Qgil added a comment.May 13 2016, 8:11 PM

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. :)

This would be fixed by our contenthandler approach in T138462

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/ :)

https://gerrit.wikimedia.org/r/288399

01tonythomas closed this task as Resolved.Dec 17 2016, 12:27 PM

Do not occur as of now, as we are storing things more 'wiki' way with Content-Handler = yay!

Qgil awarded a token.Dec 19 2016, 2:11 PM