Page MenuHomePhabricator

Remove $wgResourceLoaderLESSVars and ResourceLoaderGetLessVars hook
Closed, ResolvedPublic

Description

It's almost entirely unused. Instead we have two other approaches:

  • Explicit import from core-provided modules (e.g. @import "mediawiki.ui/variables";).
  • Implicit import from currently active skin or theme (not yet done, but under consideration for T112747).

Problems with $wgResourceLoaderLESSVars:

  • Inherently lead to unmaintainable code. There is no explicit link between code stuffing variables in there (e.g. site configuration, extensions) and code consuming those variables. This makes it very fragile to change or migrate anything.
  • Changing anything in there also results in immediate global invalidation of all Less-related caches because anything could be using it without knowing.

The last known use of it was to introduce a global variable deviceWidthTablet (T93675; 48263c3). Once that is moved to variables.less file and explicitly imported by callers, we can remove $wgResourceLoaderLESSVars from core.

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added subscribers: Zppix, Aklapper. · View Herald TranscriptJul 19 2016, 6:56 PM

We also have the ResourceLoaderGetLessVars hook, which implements the exact same functionality.

We also allow subclassing ResourceLoaderModule to override getLessVars(), which can be used to achieve the same, too (requires more code, but produces less unmaintainable results).

Krinkle added a comment.EditedJul 19 2016, 11:08 PM

We also have the ResourceLoaderGetLessVars hook, which implements the exact same functionality.
We also allow subclassing ResourceLoaderModule to override getLessVars(), which can be used to achieve the same, too (requires more code, but produces less unmaintainable results).

Yeah, the ResourceLoaderModule method is fine. We should remove the hook though. Forgot about that one!

Krinkle changed the task status from Open to Stalled.Apr 11 2017, 12:18 AM
Krinkle triaged this task as Normal priority.
Krinkle renamed this task from Remove $wgResourceLoaderLESSVars to Remove $wgResourceLoaderLESSVars and ResourceLoaderGetLessVars hook.Jul 19 2017, 11:36 PM

When is a good time to update the documentation to discourage users from using the hook?

Krinkle changed the task status from Stalled to Open.Jul 21 2017, 11:17 PM
Krinkle moved this task from Backlog to Accepted Enhancement on the MediaWiki-ResourceLoader board.

Change 366990 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] resourceloader: Soft-deprecate use of global LESS variables

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

Change 366992 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/XenForoAuth@master] Avoid deprecated ResourceLoaderGetLessVars hook

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

Change 366992 merged by jenkins-bot:
[mediawiki/extensions/XenForoAuth@master] Avoid deprecated ResourceLoaderGetLessVars hook

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

Change 366990 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Soft-deprecate use of global LESS variables

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

Restricted Application added a project: Performance-Team. · View Herald TranscriptDec 6 2017, 12:29 PM
Imarlier moved this task from Inbox to Radar on the Performance-Team board.Dec 11 2017, 8:57 PM
Imarlier edited projects, added Performance-Team (Radar); removed Performance-Team.
Krinkle changed the task status from Open to Stalled.Dec 13 2017, 6:19 PM
Krinkle edited projects, added Performance-Team; removed Performance-Team (Radar).
Krinkle moved this task from Inbox to Blocked or Needs-CR on the Performance-Team board.

Change 425273 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] resourceloader: Remove deprecated ResourceLoaderGetLessVars hook

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

I've tested and merged the above patch. Removing $wgResourceLoaderLESSVars will need a little more work however.

Change 425273 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Remove deprecated ResourceLoaderGetLessVars hook

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

Krinkle changed the task status from Open to Stalled.Apr 10 2018, 7:27 PM
Krinkle added a comment.EditedApr 11 2018, 12:40 AM

I've tested and merged the above patch. Removing $wgResourceLoaderLESSVars will need a little more work however.

The only remaining use of wgResourceLoaderLESSVars is MobileFrontend reading core's deviceWidthTablet key, which is a deprecated copy of @width-breakpoint-tablet from mediawiki.ui/variables.less. As much as it seems odd, I think it may be best to hardcode that number in MobileFrontend.hooks.php for the moment.

It will remain a duplicate, but in a different place. E.g. moving being a copy in core/DefaultSettings.php to being a copy in MobileFrontend.

I wouldn't mind prolonging this further, but only if we have a plan.

As it stands, I don't think there's a way to solve this. I assume we don't want MobileFrontend PHP to parse LESS-syntaxed files from MediaWiki core internals to extract a variable. Currently MF uses it via mobile.startup/browser.js#isWideScreen, which is accessed by:

  1. mobile.special.mobileoptions.scripts/mobileoptions.js: Hides part of an interface. Could this use CSS display:none instead, with the original Less variable directly (and e.g. a media query).
  2. mobile.startup/PageList.js: Uses it as a proxy to decide "Delay an unnecessary load of images on mobile connections". Seems obsolete with the skin separation as MF is/should be mobile-only. Could this be replaced with a browser.isMobile() method of some sort that isn't related to tablet-width, but is instead based on browser features, or user-agent, or wgMFMode?
  3. mobile.toggle/toggle.js: Decides whether to remember a decision to expand a section. Based on comments from mobileoptions.js, it seems unexpected that a section could be toggled at this point, given its interface was meant to be hidden. Either way, both ways can be solved in CSS. The toggle button can be hidden with CSS if needed. And if there are other ways to toggle a section that need to be no-ops for some reason, then the toggling itself can use a class (instead of inline display:none styles), and that class can toggle display:none from a media query. Should not affect performance.

Change 451648 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] resourceloader: Remove $wgResourceLoaderLESSVars support

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

Change 451661 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] search: Use @width-breakpoint-tablet instead of @deviceWidthTablet

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

Change 451662 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/skins/Vector@master] Use @width-breakpoint-tablet instead of @deviceWidthTablet

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

Change 451662 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Use @width-breakpoint-tablet instead of @deviceWidthTablet

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

Change 451661 merged by jenkins-bot:
[mediawiki/core@master] search: Use @width-breakpoint-tablet instead of @deviceWidthTablet

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

Change 451801 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Use @width-breakpoint-tablet

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

matmarex removed a subscriber: matmarex.Aug 9 2018, 10:38 PM

Change 451801 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Use @width-breakpoint-tablet

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

Krinkle claimed this task.Aug 12 2018, 12:51 PM
Krinkle moved this task from Next In This Quarter to Doing on the Performance-Team board.

Change 451648 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Remove $wgResourceLoaderLESSVars support

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

Krinkle closed this task as Resolved.Aug 15 2018, 2:08 AM
Krinkle removed a project: Patch-For-Review.

This has caused a serious regression -https://phabricator.wikimedia.org/T202021 :/
Why was this not picked up in CI?
Is there an issue with mobile skin CI being run in core?

Why was this not picked up in CI?

It seems that the shared extension "gate" job does not install MinervaNeue. It installs MobileFrontend and a handful of other common extensions, but not MinervaNeue. Seems worth adding!

Noloader added a subscriber: Noloader.EditedJun 7 2019, 8:40 PM

This option is still needed for Mobile FrontEnd.

In fact, we needed to add it to our LocalSettings.php after a 1.31.1 -> 1.31.2 upgrade. Also see undefined option: 'ResourceLoaderLESSVars'. This post was by another user who needed to add it after a recent migration.

Change 540093 had a related patch set uploaded (by Krinkle; owner: Fomafix):
[mediawiki/core@master] resourceloader: Drop deprecated ResourceLoader::getLessVars

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

Change 540093 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Drop deprecated ResourceLoader::getLessVars

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