Skip to content

Conversation

@Krinkle
Copy link

@Krinkle Krinkle commented Jun 10, 2025

Reviewers should focus on:

  • No (new) PHP warnings/errors in the logs.
  • Ensure the app still renders the same way visually in terms of CSS rules from your Less stylesheet files.

Necessity

  • Has the problem that is being solved here been clearly explained?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Commit message:

The only two relevent changes in Less.php 5.0 are:

  • Remove import_callback.
  • Change default math from "always" (strictMath: false) to "parens-division".

import_callback

The import_callback option was deprecated in 4.x, because it exposed a number of internal classes, all of which had to be used in order to work, including carefully obtaining and then returning the pathAndUri[1] value.

The SetImportDirs() method has been available for a long time already, and is instead given a simple string path (already resolved), and must return null|array{0:?string,1:?string}. Thus if it doesn't want to override, it can simply not return (implied null), which is equiv to [null,null] and the second URI value is optional, defaulting to the default.

Upstream change: https://gerrit.wikimedia.org/r/1010962
Docs: https://github.com/wikimedia/less.php/blob/v4.4.1/lib/Less/Parser.php#L592-L617

math

In Less.js 1.x-3.x the default was strictMath: false, which evaluates math eagerly, instead of only "strictly" between parenthesis. This had the downside of interpreting "/" slashes in newer CSS syntax as Less division, instead of as separator between values in those CSS features (aspect-ratio, border-radius, font).

As such, Less.js 4.x and Less.php 5.x, rename "strictMath" to "math", with a new third option "parens-division" as the default alongside "always" (aka strictMath: false) and "parens" (aka strictMath: true). This new option still eagerly evaluated math in most cases (plus, minus, multiply) but no longer for division. I don't know if or when Flarum wants to adopt this new default, but for now, to minimise disruption and unblock the Less.php upgrade, I suggest simply setting strictMath: false explicitly so that Less.php 5 behaves the same as before.

This decouples the trivial Less.php upgrade from the (potentially non-trivial, or unwanted) math migration.

Ref https://phabricator.wikimedia.org/T366445.
Ref https://github.com/wikimedia/less.php/blob/master/CHANGES.md.

Custom properties

In upstream Less.js 3.5, custom properties were natively supported which included a change to no longer evaluate math expressions there.

This is separate from the general case, which remains eagerly evaluated under the strictMath:false setting.

You can still perform calclations in custom properties with calc().

Ref less/less.js#3317

@Krinkle Krinkle requested a review from a team as a code owner June 10, 2025 00:24
@Krinkle Krinkle changed the title Less upgrade feat: update wikimedia/less.php from 4.x to 5.x Jun 10, 2025
@imorland imorland added this to the 2.0.0-beta.5 milestone Nov 24, 2025
@imorland
Copy link
Member

Hello @Krinkle and thank you for this PR.

So sorry that it's taken us so long to get to this. I'm preparing the next beta release, and would like to include this. Any chance you can please rebase and resolve the merge conflicts? 🙏

@Krinkle
Copy link
Author

Krinkle commented Dec 22, 2025

@imorland Done.

@imorland
Copy link
Member

@imorland Done.

Thank you! Would you also be able to address the two failures here please?

Copy link
Author

@Krinkle Krinkle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the test hits a corner case that was changed intentionally in upstream Less.js.

Failed asserting that "…
…
…reen}.dummy_func_test2{--x:100 * 10;--y:false}' [ASCII](length: 243377)
contains
".dummy_func_test2{--x:1000;--y:false}" [ASCII](length: 37).

This is not specific to the Less.php implementation, but rather a change in the upstream spec since Less.js 3.5. See this Less.js demo.

Less.php fully supports math=always and the legacy strictMath:false, which I enabled in this pull request to ensure no major changes in behavior. This means the following keeps behaving the same:

div {
  width: 10px * 4;
  height: 200px / 5;
}

But math inside the value of a custom property is no longer evaluated. This change is considered a bug fix, see also the discussion at less/less.js#3317 (comment) from 2018.

foo {
  a: 100 * 5;
  b: (100 * 5);
  --c: 100 * 7;
  --d: (100 * 7);
}
// Less.js 3.5+ and Less.php 5.1+
foo {
  a: 500;
  b: 500;
  --c: 100 * 7;
  --d: (100 * 7);
}

@Krinkle
Copy link
Author

Krinkle commented Dec 30, 2025

There seems to be no use of math inside a -- custom property in this repository, except in this one unrelated unit test.

I've updated it to demonstrate Less custom functions through width: fn() * 10px instead of --x: fn() * 10.

The only two major changes in Less.php 5.0 are:

* Remove `import_callback`.
* Change default math from "always" (strictMath: false) to "parens-division".

The import_callback option was deprecated in 4.x, because it exposed a number
of internal classes, all of which had to be used in order to work, including
carefully obtaining and then returning the `pathAndUri[1]` value.

Upstream change: https://gerrit.wikimedia.org/r/1010962
Docs: https://github.com/wikimedia/less.php/blob/v4.4.1/lib/Less/Parser.php#L592-L617

== import_callback ==

The SetImportDirs() method has been available for a long time already,
and is instead given a simple string path (already resolved), and must
return `null|array{0:?string,1:?string}`. Thus if it doesn't want to
override, it can simply not return (implied null), which is equiv
to `[null,null]` and the second URI value is optional, defaulting to
the default.

== math ==

In Less.js 1.x-3.x the default was `strictMath: false`, which evaluates math eagerly,
instead of only "strictly" between parenthesis. This had the downside of
interpreting "/" slashes in newer CSS syntax as Less division, instead of as
separator between values in those CSS features (aspect-ratio, border-radius, font).

As such, Less.js 4.x and Less.php 5.x, rename "strictMath" to "math", with a new
third option "parens-division" as the default alongside "always"
(aka `strictMath: false`) and "parens" (aka `strictMath: true`). This new
option still eagerly evaluated math in most cases (plus, minus, multiply) but
no longer for division. I don't know if or when Flarum wants to adopt this new
default, but for now, to minimise disruption and unblock the Less.php upgrade,
I suggest simply setting `strictMath: false` explicitly so that Less.php 5
behaves the same as before.

This decouples the trivial Less.php upgrade from the (potentially non-trivial,
or unwanted) math migration.

Ref https://phabricator.wikimedia.org/T366445.
Ref https://github.com/wikimedia/less.php/blob/master/CHANGES.md.

== Custom properties ==

In Less.js 3.5, custom properties were natively supported which
included a change to no longer evaluate math expressions there.

This is separate from the general case, which remains eagerly
evaluated under the `strictMath:false` setting.

You can still perform calclations in custom properties with `calc()`.

Ref less/less.js#3317
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants