Page MenuHomePhabricator

CSSMin library calls global function wfExpandUrl()
Open, LowPublic


Since r82457 (rMW97a945864ad217c4), CSSMin::remapOne() expects the application to have defined a global function called wfExpandUrl(). Otherwise, it will not expand local URLs with absolute paths. This is not a good API, and it should not have passed code review.

It should be possible for the caller to specify a callback, preferably without using global state. This may require adding additional parameters (e.g. an array of options, or an options object) or making the methods non-static (in which case the caller would specify the callback using a setter method).

Event Timeline

PleaseStand raised the priority of this task from to Needs Triage.
PleaseStand updated the task description. (Show Details)
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 8 2015, 12:53 AM
matmarex removed a subscriber: matmarex.Feb 8 2015, 12:58 PM
Aklapper triaged this task as Normal priority.Mar 17 2015, 10:36 AM

Note that we use two types of expansion when handling stylesheets:

  • Remapping: Path references in stylesheets, unlike in JavaScript, are relative to where the stylesheet is hosted. (Not relative to the current browsing context.) Kind of like how relative urls in an iframe are relative to the location of the iframe, not the parent document. In order to safely combine stylesheets from different directories and serve them from ResourceLoader, paths are remapped from local to remote paths.
  • Absolute expansion: This targets urls that are already absolute (e.g. /foo/bar, not foo/bar` or ./foo/bar) to include a protocol and host. This is needed to support serving ResourceLoader from a different host (e.g., in case of Wikimedia). As otherwise those references would no longer point to the correct domain.

Use of wfExpandUrl in CSSMin applies to absolute expansion.

Krinkle set Security to None.
Krinkle lowered the priority of this task from Normal to Low.Nov 18 2016, 7:38 PM

Seems fixable by making the class non-static and providing the callback via a constructor option instead. Similar to what we've done with objectcache and rdbms libs. Eg.. BagOStuff and WANObjectCache's asyncHandler option.

Restricted Application added a project: Performance-Team. · View Herald TranscriptAug 18 2018, 6:38 AM