Page MenuHomePhabricator

PHP warnings related to footer V2 when run Minerva in desktop mode
Closed, ResolvedPublic2 Story Points

Description

When viewing Wikipedia on Minerva in desktop mode I see PHP warnings
https://en.wikipedia.org/wiki/Head_cheese?useskin=minerva&useformat=desktop

Notice: Undefined index: footer-site-heading-html in /Users/jrobson/git/core/extensions/MobileFrontend/includes/skins/MinervaTemplate.php on line 98
Notice: Undefined index: mobile-license in /Users/jrobson/git/core/extensions/MobileFrontend/includes/skins/MinervaTemplate.php on line 99

When viewing Vector mobile we do not display a license at all due to the same issue.

Event Timeline

Restricted Application added a subscriber: Aklapper. ยท View Herald TranscriptDec 16 2016, 9:16 PM
pmiazga renamed this task from PHP warnings related to footer when run Minerva in desktop mode to PHP warnings related to footer V2 when run Minerva in desktop mode.Dec 21 2016, 5:11 PM
ovasileva triaged this task as High priority.Dec 21 2016, 5:13 PM
ovasileva set the point value for this task to 2.
ovasileva moved this task from Incoming to Triaged but Future on the Readers-Web-Backlog board.

Change 335080 had a related patch set uploaded (by Jdlrobson):
Hygiene: Remove MinervaUseFooterV2

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

phuedx assigned this task to Jdlrobson.Feb 2 2017, 3:03 PM

The warning persists after applying the patch.

Change 335698 had a related patch set uploaded (by Jdlrobson):
Avoid PHP warnings in desktop Minerva

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

Jdlrobson added a subscriber: pmiazga.

@bmansurov @pmiazga I hope you'll like the new pass which tries to standardise on template data available in core - https://gerrit.wikimedia.org/r/335698

@Jdlrobson so there are a couple of problems with the minerva skin on desktop. Take a look at this screenshot:

  • The last edited information is repeated.
  • Some things are missing as is evident from the bullet points without content.

Since we officially don't support Minerva for desktop yet, do you think it's a good opportunity for us to fix these problems rather than fixing the warnings? I assume fixing the warnings is not urgent as most users don't use Minerva on Desktop and we don't see those warnings spamming our logs.

Jdlrobson added a comment.EditedFeb 9 2017, 8:11 PM

Since we officially don't support Minerva for desktop yet, do you think it's a good opportunity for us to fix these problems rather than fixing the warnings? I assume fixing the warnings is not urgent as most users don't use Minerva on Desktop and we don't see those warnings spamming our logs.

The warnings are the main problem here and if we're not addressing that I don't know why we are even doing this task. A warning can spam our logs if it gets picked up by a search engine crawler. Just because it doesn't do that now doesn't mean it might in future. This is why I went for PS1 which is the simplest way this can be solved even though it may not feel perfect (https://gerrit.wikimedia.org/r/#/c/335698/1).

@Jdlrobson so there are a couple of problems with the minerva skin on desktop. Take a look at this screenshot:

  • The last edited information is repeated.

Yes, I noted the last modified information in the patchset commit message. Given this skin is not exposed to users I think it's acceptable and can be thought about later. I see this as a problem with the limitations imposed on skin developers by core. Also if we remove this, then the mobile Vector variant doesn't get a history link which is a licensing issue.

  • Some things are missing as is evident from the bullet points without content.

The missing bullet points look like an issue with your caching setup. Those elements should be divs not list items. Change the version number in minerva.mustache (if you already pulled down the patch you'll still be using the old cached template).

Jdlrobson updated the task description. (Show Details)Feb 9 2017, 8:12 PM

Change 335698 abandoned by Jdlrobson:
Avoid PHP warnings in desktop Minerva

Reason:
Not actively working on this due to disagreements on correct approach here

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

Jdlrobson added a subscriber: ovasileva.

@ovasileva @bmansurov dealing with the warnings should be a priority here. Can we give this some attention and work out what's the minimum possible work to do that?

We don't officially support Minerva on desktop, right? Also, do we know how many users are already using the skin on desktop? Any reason why we should take the task more seriously?

The error is accessible by a public URI (in this description). "Official" support for Minerva is irrelevant here. The page can be rendered this way and causes a warning that pollutes log files which make it harder to find real errors. One solution of course would be to officially not support Minerva by not allowing it to be rendered but MediaWiki doesn't make this easy.

@Jdlrobson, @bmansurov - I agree that we should take care of this, howeer, I think there's higher priority items we should focus on first. @bmansurov - I'll leave this one up to you.

Yes, let's put this off until a later date. We've already spent too much time on the task in a previous attempt.

This is in Current Sprint in the backlog. Should it be moved?

I'd like a chance to make a case for this in the next sprint. There are still warnings in logstash that we should look to suppress.

Change 348953 had a related patch set uploaded (by Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Move logo generation code inside Minerva

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

Jdlrobson reassigned this task from Jdlrobson to bmansurov.Apr 24 2017, 7:37 PM

@bmansurov will sanity check the solution to see if it makes sense before we commit to doing work for this in the next sprint.

I've skimmed through the patch and I think it makes sense given that we don't use MobileContext inside SkinMinerva (as was mentioned by Jon).

Change 348953 had a related patch set uploaded (by Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Move logo generation code inside Minerva

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

Change 348953 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Move logo generation code inside Minerva

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

Jdlrobson moved this task from Needs Analysis to Ready for Signoff on the Reading-Web-Sprint-96 board.

Just need to verify the php errors are no longer showing in beta logstash

Jdlrobson closed this task as Resolved.May 4 2017, 3:54 PM

This is fixed.
Follow up work filed in T164502