Put division calculations in less files in parentheses if they were not there alr…

[hostep]

…eady, this fixes generating correct css again after the upgrade to less.js v4 in AC-8098 & AC-9713.

Description (*)

This PR comes from discussion in #38109 (comment), just some notes:

• v4 of the less.js library is not backwards compatible with v3, all division calculations should be put in parentheses now.

• I've only tested with Magento Open Source, it's likely that more changes will be needed for Magento Commerce and B2B (and whatever other products are out there)

• I currently forgot to look into all the other modules that aren't included in this core Magento one, like pagebuilder, MSI, security-package, ... Those might need work as well...
  Update: only pagebuilder was affected, I created PR 860 on the magento2-page-builder repo to fix it there
  Update 2: it turns out this wasn't need, so all is good here

• I've tested this with the 3 themes included in Magento Opensource: Magento/blank, Magento/luma & Magento/backend, I know that Magento Commerce comes with a 4th theme as well and maybe there are more themes in the other products, I have no idea, so all should be tested

• Update: I've also tested bin/magento setup:static-content:deploy (which uses a different less compiler) and compared the compiled css before these changes and after these changes and could see no differences, which is good 👍

• About the flag strictMath that was changed in dev/tools/grunt/configs/less.js, my idea was that this would highlight problematic code faster to custom theme developers, but from working on this PR, it only detects a very little amount of problems, so not sure if this is a good idea or not
  Update: this wasn't a good idea, this has been reverted now

• The change in lib/web/css/source/lib/_utilities.less to @{_property}: (@_value); was a quick way to fix a bunch of different issues at the same time, not sure if that's the best way, also not sure if still needed, this was one of my first fixes, and I didn't want to re-test after all the hours I already spent on this
  Update: I've removed this fix, and fixed the root causes instead

• The majority of failing static tests are bugs in the coding standards (example 1, example 2, the other ones haven't been reported yet I think) and in my opinion we shouldn't fix those in this scope of this PR. And the PR should be approved even with those failing.

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

1. Setup 2 copies of Magento code base next to each other
2. In one copy, keep using less.js in the package.json.sample file, in the other copy, downgrade it back to version v3, like this:

@@ -18,7 +18,7 @@
     "grunt-contrib-cssmin": "~5.0.0",
     "grunt-contrib-imagemin": "~4.0.0",
     "grunt-contrib-jasmine": "~4.0.0",
-    "grunt-contrib-less": "~3.0.0",
+    "grunt-contrib-less": "~2.1.0",
     "grunt-contrib-watch": "~1.1.0",
     "grunt-eslint": "~24.3.0",
     "grunt-exec": "~3.0.0",
@@ -27,7 +27,7 @@
     "grunt-template-jasmine-requirejs": "~0.2.3",
     "grunt-text-replace": "~0.4.0",
     "imagemin-svgo": "~9.0.0",
-    "less": "4.2.0",
+    "less": "~3.13.1",
     "load-grunt-config": "~4.0.1",
     "morgan": "~1.10.0",
     "node-minify": "~3.6.0",

3. Note, only in the copy of Magento where we use less.js v4, you should test the changes from this PR.
4. Execute following steps in both magento copies:

$ cp package.json.sample package.json
$ cp Gruntfile.js.sample Gruntfile.js
$ rm -Rf node_modules/ package-lock.json
$ npm install
$ grunt clean:backend && grunt exec:backend && grunt less:backend && grunt clean:blank && grunt exec:blank && grunt less:blank && grunt clean:luma && grunt exec:luma && grunt less:luma

5. Now compare the generated *.css files in pub/static from both copies. The output should be identical.
6. We should also test the php less compilation with bin/magento setup:static-content:deploy and compare the compiled css before these changes and after these changes and see that there are no differences

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
• All automated tests passed successfully (all builds are green)

Resolved issues:

1. resolves [Issue] Put division calculations in less files in parentheses if they were not there alr… #38351: Put division calculations in less files in parentheses if they were not there alr…

[m2-assistant]

Hi @hostep. Thank you for your contribution!
Here are some useful tips on how you can test your changes using Magento test environment.

Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

• @magento run all tests - run or re-run all required tests against the PR changes
• @magento run <test-build(s)> - run or re-run specific test build(s)
  For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests
13. Semantic Version Checker

You can find more information about the builds here
ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.

---

For more details, review the Code Contributions documentation.
Join Magento Community Engineering Slack and ask your questions in #github channel.

[hostep]

@magento run all tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[hostep]

I spend a little bit more effort in checking security-package, inventory & magento2-page-builder code and only page-builder contained one mistake, here's the fix for that one: magento/magento2-page-builder#860

I've updated the description above so it's also clear there

[hostep]

I just realised that I may have interpreted the strictMath option incorrectly and that this change might be responsible for finding a bunch more code that needed to change which might not have been needed if we left this option off. I'll try to investigate later this week, it could be that this PR will be reduced significantly in amount of changes if it turns out this flag isn't needed to be turned on.

[hostep]

I've reverted my change to strictMath and turned it of again, just like it was before. That allowed me to reduce the number of changes here almost in half, only 52 files changed now instead of 110 before.

The only thing we actually needed to fix for less.js v4 is to put the division calculations in parentheses, not all calculation, that's only needed when strictMath is enabled.

I've force pushed my changes now. Still need to deal with static test failures.

@magento run Static Tests

[hostep]

@magento run Static Tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[hostep]

In the second commit of this PR, I've fixed some static test failures, only the ones I kind of agree that need fixing. All the other - in my opinion - are bugs in the coding-standards code and should be fixed there. I would very much appreciate it if somebody could double check these and if they don't make sense, just agree to ignore all failures and proceed with the PR as-is.
Thanks!

@magento run all tests

[hostep]

@magento run all tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[hostep]

sigh, I'm now seeing that compiling less files (both with less.js and less.php compiler) containing these static test fixes outputs all comments that are in the form of /* & */ to the generated css files, it doesn't output those comments when using the form //. But that latter form triggers another bug in coding standards. So I guess I should revert all my static test fixes again and consider every single one the static test failures it reports here as a bug in the coding standards codebase? Opinions?

Adobe really should throw away all coding standards checks they do on less files, and start from scratch with another tool then phpcs, so that they actually make sense and don't annoy contributors so extremely.

If I don't get an agreement, I'll just put a coding standard ignore statement at the top of every single less file that gives an issue so they don't get checked anymore all together. That would also solve the problem, it's not the right fix, but it's the fix that would allow this PR to move forward the quickest I guess...

[ihor-sviziev]

Regarding code styles - looks like it works incorrectly for less files. The comments should be // here is a comment, otherwise they will be added to resulting CSS file.
Please add to ignore all the related failures

[hostep]

@magento run Static Tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[hostep]

This will be very controversial, but the only reasonable way to fix static test failures for a coding-standard that's full of bugs with those less files, is just to ignore all those files. I spend an hour trying to figure out a better way, but there just isn't one.
The syntax // @codingStandardsIgnoreStart doesn't work, the syntax /* @codingStandardsIgnoreStart */ does work, but then gets outputted to the css files.
Fun fact, the syntax Magento core devs use to end such a block: //@codingStandardsIgnoreEnd isn't working at all and after this line the ignoring just keeps on happening, but I already discovered that in #38012.
Fun fact nr 2: the static tests here on github are not checking the less files in lib/ they only look for less files in app/.

Anyways, if the static tests now no longer fail, I'm going to get this PR out of WIP.
I'm also not going to put any more work into this PR (especially if somebody requires other static test fixes, in that case, please do it yourself and then also join the club and get very frustrated by the current situation of how bad the coding-standards works on less files)

[hostep]

@magento run all tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[mrtuvn]

looks good to me . Approved

[engcom-Bravo]

@magento create issue

[andrewbess]

Hello @hostep
Thank you for your collaboration
Looks like the test failures are not related from this PR
Also, I think it would be great to check less files in Magento2 EE and Magento2 B2B
I try to do it.

[andrewbess]

Looks good for me
Approved

[andrewbess]

I found 3 files in theme Magento/spectrum that should be fixed.
Waiting for answer about next actions from Adobe team

cc @hostep @ihor-sviziev @mrtuvn @sidolov

[andrewbess]

Hello @hostep
What do you think about this part of code?
Should we put division calculation in parentheses here?
border-width: ((@_field-error-triangle__width / 2) + 1) ((@_field-error-triangle__width / 2) + 2) 0 ((@_field-error-triangle__width / 2) + 2);
margin-left: -((@_field-error-triangle__width / 2) + 2);
I think it will no unnecessary.

[andrewbess]

The necessary fixes for Magento EE repository have been provided to @engcom-Hotel in the patch

[hostep]

@andrewbess: it's not needed to add extra parentheses in those lines you mentioned. Mathemetics in less follows the same principles as normal mathematics, in that divisions have a higher priority than additions.

[andrewbess]

@andrewbess: it's not needed to add extra parentheses in those lines you mentioned. Mathemetics in less follows the same principles as normal mathematics, in that divisions have a higher priority than additions.

Ok.
Thank you @hostep for explanation

[andrewbess]

@magento run WebAPI Tests, Integration Tests, Functional Tests B2B

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.