Page MenuHomePhabricator

Title not shown for page named "0" on mobile
Closed, ResolvedPublic

Description

In the mobile site of any Wikipedia (*.m.wikipedia.org) the article title is not displayed if it is "0".

Examples: https://en.m.wikipedia.org/wiki/0, https://test2.m.wikipedia.org/wiki/0

Details

Related Gerrit Patches:
mediawiki/skins/MinervaNeue : masterShow title on the page named '0'

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 16 2017, 9:31 PM
Zppix added a subscriber: Zppix.Apr 16 2017, 9:36 PM

Here's a screenshot of what the task is talking about.
On left, there is no Article Title "Heading" shown (en.m.wikipedia.org/wiki/0). On right, there is an Article Title "Heading" shown (en.m.wikipedia.org/wiki/Human)

Jdlrobson triaged this task as Low priority.Jul 19 2017, 3:37 PM
Jdlrobson moved this task from Incoming to Triaged but Future on the Readers-Web-Backlog board.
Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptJul 19 2017, 3:37 PM
Jdlrobson moved this task from Backlog to Bugs on the MinervaNeue board.Jul 20 2017, 10:47 PM
EddieGP claimed this task.Jul 28 2017, 3:00 PM
EddieGP added a subscriber: EddieGP.

Typical php pitfall:

When converting to boolean, the following values are considered FALSE: [...] the empty string, and the string "0"

Looing at SkinMinerva::getHeadingHtml:

$pageTitle = $this->getOutput()->getPageTitle();
if ( $pageTitle ) {
	$heading = $pageTitle;
}

I haven't sent this in as a patch because I'm not sure why the if( $pageTitle ) was added in the first place - hence I don't know whether removing it might be harmful to some other edge case. Should we go with just removing the if or exclude this edge case with if( $pageTitle || $pageTitle === '0' )?

TheDJ added a subscriber: TheDJ.EditedJul 30 2017, 5:08 PM

@EddieGP
Vector also expects that title's might not be present.

// Loose comparison with '!=' is intentional, to catch null and false too, but not '0'
                        if ( $this->data['title'] != '' ) {
                        ?>
                        <h1 id="firstHeading" class="firstHeading" lang="<?php $this->text( 'pageLanguage' ); ?>"><?php
                                 $this->html( 'title' )
                        ?></h1>
                        <?php
                        } ?>

Change 368585 had a related patch set uploaded (by EddieGP; owner: EddieGP):
[mediawiki/skins/MinervaNeue@master] Show title on the page named '0'

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

Thanks @TheDJ, that snippet (especially with the comment given there) convinced me that this check exists on purpose. ;-) Hence I've used the same comparison as Vector did (and re-used the comment from there, sums it up quite nicely).

pmiazga added subscribers: ovasileva, pmiazga.

Bringing this patch as unplanned sprint work to our current sprint. @EddieGP wrote fix and it works nicely. @ovasileva could you verify and signoff this task?

Change 368585 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Show title on the page named '0'

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

Jdlrobson closed this task as Resolved.Jul 31 2017, 10:07 PM
Jdlrobson added a subscriber: Jdlrobson.

I can verify this as it's a technical task. Thanks @EddieGP you fixed it! Well done and thank you! :D