Page MenuHomePhabricator

3rd parties shouldn't be subjected to hacks.less
Closed, ResolvedPublic2 Story Points

Description

This file only makes sense on Wikimedia wikis. There should be a config variable that turns this module on/off.

Event Timeline

Jdlrobson raised the priority of this task from to Needs Triage.
Jdlrobson updated the task description. (Show Details)
Jdlrobson added a project: Readers-Web-Backlog.
Jdlrobson added a subscriber: Jdlrobson.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 26 2015, 9:36 PM
Jdlrobson set Security to None.
Restricted Application added a project: Readers-Web-Backlog. · View Herald TranscriptAug 12 2015, 1:33 AM
Jdlrobson triaged this task as Low priority.Sep 16 2015, 6:41 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson added a project: good first bug.
Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptApr 27 2016, 4:26 PM
MaxSem moved this task from Unsorted to Cleanup on the Technical-Debt board.Jul 17 2016, 6:18 PM
Niedzielski added a subscriber: Niedzielski.

@Jdlrobson, I'm gonna try my hand on this one since I just changed hacks.less. Please unassign if you object.

Change 326192 had a related patch set uploaded (by Niedzielski):
WIP: Allow hacks.less inclusion to be configurable

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

Tgr added a subscriber: Tgr.

This does not seem to be a blocker for deploying TemplateStyles, maybe the relationship was intended the other way around.

MBinder_WMF set the point value for this task to 2.Dec 19 2016, 5:14 PM
ovasileva added a subscriber: ovasileva.

@Jdlrobson - moving to next sprint - this is still for code review?

Jdlrobson updated the task description. (Show Details)Jan 10 2017, 12:43 AM
Jdlrobson updated the task description. (Show Details)

Change 326192 merged by jenkins-bot:
New: allow hacks.less inclusion to be configurable

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

Change 331425 had a related patch set uploaded (by Jdlrobson):
Minerva should apply known template hacks in production

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

Jdlrobson updated the task description. (Show Details)Jan 10 2017, 1:20 AM

Change 331693 had a related patch set uploaded (by Jdlrobson):
3rd parties should not get Wikipedia specific styles

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

Change 331425 merged by jenkins-bot:
Minerva should apply known template hacks in production

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

Change 331693 merged by jenkins-bot:
3rd parties should not get Wikipedia specific styles

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

phuedx updated the task description. (Show Details)Jan 17 2017, 8:01 PM
phuedx closed this task as Resolved.Jan 17 2017, 9:53 PM
phuedx added a subscriber: phuedx.

As expected, you can still see the contents of hacks.less in the body of the skins.minerva.content.styles RL module.

I'd like to note that the (neato!) use of the when clause may not play nicely with RL module invalidation: when I added $wgMinervaApplyKnownTemplateHacks = 1; I didn't the module's contents change until I'd added a NOOP statement to a file inside of the module body {};. AFAICT this is because a module's hash varies by the contents of the file before compilation, i.e. before the when clause is expanded.

If we intend to use this feature of LESS more and more, we'll begin to come up against this more and more.

I'm sorry if this didn't work out as hoped. Should this ticket be reopened with an alternate approach or new tickets against RL filed?

I'm sorry if this didn't work out as hoped. Should this ticket be reopened with an alternate approach or new tickets against RL filed?

On the contrary, I think it's worked out exactly as planned and, in the context of this particular "feature", the solution works perfectly as it's highly unlikely that an admin will change the $wgMinervaApplyKnownTemplateHacks.

This change means that the styles from hacks.less are now omitted when we update our bundled stylesheets from Vagrant with make-css-assets.bash.

I'm not familiar with what's behind the various hacks in that file, but just from a cursory glance with the change applied I can see that it's affecting the layout of in-article image galleries. With T155694 in mind, it's probably better to try and get hacks.less back than to write a new set of Android-specific hacks. Is there another way we can save hacks.less from getting omitted from our downloads of the relevant module(s)?

Would just loading from a wiki on the Wikimedia production cluster (say, mediawiki.org) be an easy solution? It seems to work, but are there gotchas here that I'm not familiar enough with ResourceLoader or the Wikimedia production environment to know about?

I can split off a new ticket if this is off-topic for the current one.

@mdholloway looks like the make-css-assets just hits a localhost.. so yes you can either hit a production server instead or make sure whoever runs it has set $wgMinervaApplyKnownTemplateHacks to true.

Looks like this shouldn't impact old versions of app but you'll need to do it for next release.

Cool, as long as there's no hidden drawback to hitting a production server, I'll just change the bash script to do that.

Only drawback is the production code is likely to be outdated (as deployments are only once a week). Care would need to be taken on when to hit it. The beta cluster may be a better place to hit for more recent versions.

Given that the Beta Cluster is updated every 10 minutes, I'd say hit that first, falling back to prod if the BC is down.

For our purposes in the app, I'm not sure we need the latest and greatest beyond keeping in sync with what's deployed to production. Back in the old days, we used to load from bits.wikimedia.org, which I think reflected the production environments, but since bits was decommissioned, we've just been using a local Vagrant instance. Still, I'm fine either way.