Sounds exactly like bug 60368.

(In reply to Bartosz Dziewoński from comment #1)

Sounds exactly like bug 60368.

Maybe, if using "./variables" behaves like "variables" and doesn't bypass the include path.

(In reply to Daniel Friesen from comment #2)

(In reply to Bartosz Dziewoński from comment #1)

Sounds exactly like bug 60368.

Yes, this is exactly the same issue, though with a better summary. I'm not sure which one should be marked as a duplicate.

This happens for two reasons:

a) The skins/vector directory, which contains such a generic filename as "variables.less", is in $wgResourceLoaderLESSImportPaths. If it is really necessary to include this file from other directories, the filename ought to be prefixed somehow (e.g. @import "skins/vector/variables.less").

b) lessc adds the including file's directory to its importDir array *after* the entries from $wgResourceLoaderLESSImportPaths.

Maybe, if using "./variables" behaves like "variables" and doesn't bypass
the include path.

I don't see any special handling of "./" in the lessc source code.

(In reply to Kevin Israel (PleaseStand) from comment #3)

(In reply to Daniel Friesen from comment #2)

(In reply to Bartosz Dziewoński from comment #1)

Sounds exactly like bug 60368.

Yes, this is exactly the same issue, though with a better summary. I'm not
sure which one should be marked as a duplicate.

How about we make this bug a blocker for 1.23, solved by anything that immediately fixes the bug.
And that bug can remain open till we add long term fixes to the issue, ie: Make doing things sane.

Since the reason vector is in the include path is for VectorBeta and since $wgResourceLoaderLESSImportPaths is global config to temporarily fix the issue how about we remove vector from $wgResourceLoaderLESSImportPaths in core and have the VectorBeta extension add it itself.

That way the issue will only show up when VectorBeta is installed.

Long-term we'll probably want to fix the issue in multiple ways:

• Maybe let resource loader module definitions define custom import paths.
• Fix lessc or our use of it so that the current directory is searched first instead of last.

I think there is value in making the LESS variables of Vector available to extensions using that skin. Most extensions don't use LESS but we should be encouraging them to do and to share variables and themes with standard names.

Let's rethink this and get to something like this:
@import 'variables.less'
@import 'vector/variables.less'
@import 'monobook/variables.less'

(In reply to Jon from comment #5)

I think there is value in making the LESS variables of Vector available to
extensions using that skin. Most extensions don't use LESS but we should be
encouraging them to do and to share variables and themes with standard names.

Being able to define custom per-module include paths could help that.

While digging into this I found that part of this issues actually lies in a poor choice of LESS parsing library:
https://www.mediawiki.org/wiki/Requests_for_comment/Change_LESS_compilation_library

We used lessphp/lessc while Less.php/Less_Parser is actually better maintained.

The latter has a better default import path (this regression would have never popped up as the local variables file would always be used.

What Jon wants to do would even work better in Less.php/Less_Parser than it would in lessphp/lessc.

Though we should probably fix both these bugs before we get around to considering a different less parser.

Change 132440 had a related patch set uploaded by Daniel Friesen:
Add Vector to the LESS import path from within VectorBeta

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

Change 132441 had a related patch set uploaded by Daniel Friesen:
Remove Vector from the default LESS import path

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

Change 132447 had a related patch set uploaded by Daniel Friesen:
Backport Iee47bdc23630e02ccfcbd28496ec5268892eb629 to 1.23.

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

Vector has no business being in the import path. It was added there as a hack for the VectorBeta extension.

https://github.com/wikimedia/mediawiki-extensions-VectorBeta/blob/92ea3a6ce67ef/resources/typography/styles.less#L2

Now that the typography stuff is merged into core, I don't think they need it as much anymore. They should probably just duplicate the individual properties on a individual need base (they're only temporary for the duration and iteration of each experiment, worst case scenario someone makes a change to them in core and people'd have to update them manually in VectorBeta).

The local path should always be used in preference to the global import path. So while Vector should not be in MediaWiki's global import path, your extensions module's directory should always be consulted first.

So removing it is imho not a logical fix and won't solve the actual problem (if you'd have a file that has the same name as one of the other import paths it would cause the same bug).

I think there is a bug in how we interpret less that causes the path to be appended instead of prepended (like /my:$PATH vs $PATH:/my)

At krinkle's suggestion, I'm leaving this as is for now. I'll approve your patch tomorrow for 1.23 only if nothing else comes up.

(In reply to Mark A. Hershberger from comment #13)

...if nothing else comes up.

I should clarify that I mean if there aren't any other issues raised.

So yeah, this is a bug in the leafo/lessphp implementation of LESS. It looks up import path first, and if nothing is found it tries to resolve it as a relative file name.

Both the official js implementation and the other PHP port (oyejorge/less.php), do it correctly (try resolving as relative file name first and fallback to the import path).

Ori and I are repository collaborators of leafo/lessphp and can fix it there. Let's see if we can fix that, and then apply that update to mediawiki. Should be a trivial path.

Hoping Ori and I can get up to 24 hours tops to fix this "the right way". If not, let's go ahead and remove Vector from the import path in the 1.23 branch. Note that this will not entirely resolve the issue as the bug could just as easily happen again with another file name. Perhaps people are already hitting this bug, so don't mark this bug as fixed (in the release notes be sure that it's fixed for paths clashing with Vector, it would not have resolved it for imported paths in general).

(In reply to Krinkle from comment #15)

Hoping Ori and I can get up to 24 hours tops to fix this "the right way".

Granted. You have 24 hours. The clock starts now.

((Edit conflicted, but I'll post it anyways))

(In reply to Krinkle from comment #12)

So removing it is imho not a logical fix and won't solve the actual problem
(if you'd have a file that has the same name as one of the other import
paths it would cause the same bug).

MediaWiki has worked fine for a few releases with LESS in place. There is an underlying bug, but it has existed ever since we introduced LESS support. It only turned into a regression in 1.23 when Vector was added to the default path (previously being dedicated solely to mixins that always had .mixins in their filename). So removing the path won't fix the bug, but it will return us to the status quo (the same behavior as 1.22) allowing us to release 1.23 (which is already branched for release as AFAIK under a feature freeze) without a regression and work on adding proper long-term fixes to the underlying bugs to 1.24.

I mentioned my long-term ideas earlier:
(Daniel Friesen said in comment #4)

Long-term we'll probably want to fix the issue in multiple ways:

• Maybe let resource loader module definitions define custom import paths.
• Fix lessc or our use of it so that the current directory is searched first

instead of last.

I'm working on the first right now. I also peaked at the second but there are multiple ways of fixing that, each with tradeoffs that should be evaluated.

I think there is a bug in how we interpret less that causes the path to be
appended instead of prepended (like /my:$PATH vs $PATH:/my)

Actually it's not our fault at all, it's lessc's (leafo/lessphp) fault.

Instead of prepending the current directory to the import dir when compiling, lessc appends it:
https://github.com/leafo/lessphp/blob/2cc77e3c7b9e84cf30ca64fccf2a45a84fbcf0d3/lessc.inc.php#L1918

lessc has only made two changes between the the copy we have in master and now, and the behavior is still the same.

Also on our end in core we set our include path in a static ResourceLoader::getLessCompiler() that doesn't know what the path being compiled is, so there is no one simple workaround.

We can:
A) Fix lessc upstream by making that line use array_unshift. (Assuming they are responsive enough and don't consider this existing behavior a feature or something they want to wait before fixing)
B) Patch lessc locally in the same way (this would end up forking and making it harder to maintain)
C) Make getLessCompiler return a subclass of lessc that messes with its import dir handling on compile to work around the issue.
D) Explicitly work around the bug in every piece of code that does less compilation by manually messing with the include path before compile. (doesn't scale well)
E) Just drop lessc (leafo/lessphp) and switch to oyejorge/less.php, since this is just one of the many issues in lessc that less.php lacks.

Change 132498 had a related patch set uploaded by Daniel Friesen:
ResourceLoader: Allow individual modules to define their own additional less import paths.

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

I've implemented per-module import paths that – among other things – allow VectorBeta to import stuff from Vector without requiring extra co-operation from core, pollution of global imports, or even requiring VectorBeta modules that don't need stuff from Vector to have it in the import path. I hope to have this in 1.24.

We can either have VectorBeta add Vector to $wgResourceLoaderLESSImportPaths so it works with 1.23 until we have 1.24 released and can use the feature.
Or we can have VectorBeta use the per-module import paths now and make VectorBeta 1.24/master/wmf-1.24 only.

It's up to you guys what we do.

But either way, I do NOT want to see 1.23 shipped with Vector in the default LESS import path.

Change 132447 abandoned by Krinkle:
Backport Iee47bdc23630e02ccfcbd28496ec5268892eb629 to 1.23.

Reason:
Please maintain a useful summary and a matching Change-Id for the backport commit. The easiest way is to create this commit using cherry-pick. If it doesn't apply cleanly, manually re-use the same commit message.

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

Change 132812 had a related patch set uploaded by MarkAHershberger:
Remove Vector from the default LESS import path

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

Change 132812 merged by jenkins-bot:
Remove Vector from the default LESS import path

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

Was lessc updated to fix the underlying issue?

What is the status of Daniel's patches https://gerrit.wikimedia.org/r/#/c/132440/ and https://gerrit.wikimedia.org/r/#/c/132441/ to undo VectorBeta hacks? They both have a -1 from Krinkle.

Upstream bug report is:
https://github.com/leafo/lessphp/issues/555

That one has not been fixed.

See also bug 60368 which is for the general issue with @import. This bug, bug 64595, is specifically for the conflict with a file called "variables.less" and the fact that Vector has its path in the less import path list, which shouldn't be.

@Bartosz: However Ori and myself have been given push access to the leafo/lessphp repo, so consider it open for patches just like MediaWiki itself. Either through pull request directly (which we can review) or in Gerrit and I'll apply accordingly.

Should be a matter of carefully swapping the two pieces of code in lessc:: tryImport()

Change 132440 abandoned by Bartosz Dziewoński:
Add Vector to the LESS import path from within VectorBeta

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

Change 132441 merged by jenkins-bot:
Remove Vector from the default LESS import path

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

This is no longer an issue with Vector removed from the import path.
Bug 60368 still needs a proper fix, but that's a bit less urgent.

Change 132498 abandoned by Krinkle:
ResourceLoader: Allow individual modules to define their own additional less import paths.

Reason:
Closing for now. bug 64595 was fixed and bug 60368 is an upstream bug we mitigate by not adding any more import paths.

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