Regression in v4.2.0

[hostep]

Hi there

I'm currently working on trying to upgrade this library from v3 to v4 in the Magento2 codebase. Because when working on local and when using grunt, Magento uses less.js. But in production deploys, Magento uses less.php (it's been like this for years, probably to avoid node.js requirements for the people/servers only having php available).

Magento had this problem for many years that less compilation in developer mode resulted in slightly different css output than in production mode, which is sometimes kind of annoying (example), but it's not as bad as it sounds.
So I really appreciate all the recent efforts in trying to port functionalities from less.js to less.php!

However, when trying out version 4.2.0, I'm getting compilation problems that didn't happen in 4.1.1, so I have the feeling there is a regression here somehow.
Error in Magento:

$ bin/magento setup:static-content:deploy -f en_US
Deploy using quick strategy
frontend/Magento/blank/en_US            61/2069             >--------------------------- 2%     1 sec
Compilation from source: app/design/frontend/Magento/blank/web/css/styles-m.less
variable @fontValue is undefined in file var/view_preprocessed/pub/static/frontend/Magento/blank/en_US/css/source/lib/_typography.less in _typography.less on line 235, column 34
233|         & {
234|             .lib-font-size-value(@_p-margin-top);
235|             .lib-css(margin-top, @fontValue);
236|         }
237|
238|         & { in _typography.less in _resets.less in _reset.less

I've performed a git bisect on this package and found out that the problem got introduced in 6d652ed

The less compilation in Magento is quite complex and involves a whole bunch of different less files, so sorry for not (yet) providing a minimal reproducible example of how to trigger the problem, I simply lack the time at the moment.

[hostep]

Found a simple case to reproduce, have a less file with this content:

@root__font-size: 62.5%;
@font-size-unit: rem;
@font-size-unit-ratio: unit((@root__font-size * 16/100));
@font-size-unit-convert: true;

.lib-font-size-value(
    @_value
) when not (@_value = false) and not (@_value = '') and (@font-size-unit-convert) {
    @fontValue: (unit(((@_value) * 1), @font-size-unit) / @font-size-unit-ratio);
}
.lib-font-size-value(
    @_value
) when (@font-size-unit-convert = false) {
    @fontValue: @_value;
}

.lib-font-size-value(10);
body {
    margin-top: @fontValue;
}

Compilation with v4.1.1 works and outputs: body { margin-top: 1rem; }
Compilation with v4.2.0 fails.

It has something to do with this expression that doesn't match not (@_value = false) and not (@_value = '')

[Krinkle]

Thanks @hostep, I love the quick feedback from Magento, and the test case helps a lot.

I've filed this downstream at https://phabricator.wikimedia.org/T358159 where you can follow further investigation and hopefully a quick fix!

[hostep]

Thank you for the quick fix @Krinkle, I can confirm that dcb1ba5 fixes this issue.

---

However, I now just ran into another problem with the magento less files, it works on v4.2.0, but not on the latest master branch and it seems to be caused by this change: a34ce2a

Here's how to reproduce it (it's quite big, but it can probably be narrowed down much further):

@message-error-icon: '\e61f';
@message-error-icon__color-inner: red;
@message-error-icon__color-lateral: white;
@message-error-icon__background: red;
@message-error-icon__top: 10;
@message-error-icon__right: 10;
@message-error-icon__bottom: 10;
@message-error-icon__left: 10;
@message-icon__font-size: 24px;
@message-icon__font-line-height: 1;
@icon-font: 'luma-icons';
@icon-font__vertical-align: middle;

._lib-icon-font(
    @_icon-font-content,
    @_icon-font,
    @_icon-font-size,
    @_icon-font-line-height,
    @_icon-font-color,
    @_icon-font-margin,
    @_icon-font-vertical-align
) {
    -webkit-font-smoothing: antialiased;
    -moz-osx-font-smoothing: grayscale;
    display: inline-block;
    font-weight: normal;
    overflow: hidden;
    speak: none;
    text-align: center;
}

.lib-message-icon-lateral(@_message-type: info, @_message-position: right) {
    @_message-icon-position: @_message-position;
    @_message-icon: "message-@{_message-type}-icon";
    @_message-icon-color: "message-@{_message-type}-icon__color-lateral";
    @_message-icon-background: "message-@{_message-type}-icon__background";
    @_message-icon-top: "message-@{_message-type}-icon__top";
    @_message-icon-right: "message-@{_message-type}-icon__right";
    @_message-icon-bottom: "message-@{_message-type}-icon__bottom";
    @_message-icon-left: "message-@{_message-type}-icon__left";
    ._lib-message-icon-lateral(
        @_message-icon-position,
        @@_message-icon,
        @@_message-icon-color,
        @@_message-icon-background,
        @@_message-icon-top,
        @@_message-icon-left,
        @@_message-icon-bottom,
        @@_message-icon-right
    );
}

._lib-message-icon-lateral(
    @_message-icon-position,
    @_message-icon,
    @_message-icon-color,
    @_message-icon-background,
    @_message-icon-top,
    @_message-icon-left,
    @_message-icon-bottom,
    @_message-icon-right
) {
    > *:first-child {
        &:after {
            ._lib-icon-font(
                @_icon-font-content: @_message-icon,
                @_icon-font: @icon-font,
                @_icon-font-size: @message-icon__font-size,
                @_icon-font-line-height: @message-icon__font-line-height,
                @_icon-font-color: @_message-icon-color,
                @_icon-font-margin: (-@message-icon__font-size/2) 0 0,
                @_icon-font-vertical-align: @icon-font__vertical-align
            );
        }
    }
}

.example-message-2 {
    .lib-message-icon-lateral(error, right);
}

Error output:

PHP Fatal error:  Uncaught Less_Exception_Chunk: ParseError: Unexpected input in test3.less on line 73, column 14
71|                 @_icon-font-margin: (-@message-icon__font-size/2) 0 0,
72|                 @_icon-font-vertical-align: @icon-font__vertical-align
73|             );
74|         }
75|     }
76| } in vendor/wikimedia/less.php/lib/Less/Parser.php:632
Stack trace:
#0 vendor/wikimedia/less.php/lib/Less/Parser.php(588): Less_Parser->GetRules('/Volumes/Projec...')
#1 vendor/wikimedia/less.php/lib/Less/Parser.php(453): Less_Parser->_parse('/Volumes/Projec...')
#2 test.php(12): Less_Parser->parseFile('/Volumes/Projec...', '')
#3 {main}
  thrown in vendor/wikimedia/less.php/lib/Less/Parser.php on line 632

Expected output:

.example-message-2 > *:first-child:after {
  -webkit-font-smoothing: antialiased;
  -moz-osx-font-smoothing: grayscale;
  display: inline-block;
  font-weight: normal;
  overflow: hidden;
  speak: none;
  text-align: center;
}

[Krinkle]

@hostep Thanks again. Filed at https://phabricator.wikimedia.org/T358256 with a further reduced test case, and fixed just now by Hannah in master with a113853.

[hostep]

Super, thanks very much!

I can confirm this fixes the issue.

Compiling less code using commit a113853 in Magento now works again without failures and the output seems to be correct at first sight!

Would be appreciated if you could tag a new version somewhere in the next few days.

---

There are still a ton of differences between less.js & less.php output in Magento, but I guess that's expected if you guys have to port over some changes still. Also, Magento is already using less.js v4.2.0, and I know you guys at the moment want to stick to v2.x compatibility for now, so it's expected for now that there are still many differences so far.
I'm just going leave the diff from Magento here, between less.js & less.php css output (left side being less.js), in case you are curious, but I don't expect you to fix all differences in the short term, so don't worry too much about this at the moment.

less-diff.patch

---

Thanks you very much for the quick fixes!

[Krinkle]

@hostep We won't stay on the Less.js v2.5.3 spec much longer. The porting of Less.js v3.13 was just green-lit. In preparing for that, we noticed that the previous maintainers never fully checked in all parts of the v2.5.3 spec. In addressing that, we noticed various divergences in the output and internal code structure. Hence, we're fixing a few of those up at the same time.

The next spec target is Less.js v3.13.