Removes less code that uses frontend definitions in an adminhtml file…

[hostep]

…. Fixes #775

Description (*)

This removes references to less classes in an adminhtml less file that are only defined in the frontend of Magento's Blank and/or Luma themes.
So you aren't able to use these references in an adminhtml file and really shouldn't as it won't work.
See #775 (comment) for an example (I did investigate all the other problems by now and they are all the same problem).

When comparing the resulting css file before and after this change, zero changes were found, so the PR shouldn't cause any issues.

Story

?

Bug

?

Task

?

Fixed Issues (if relevant)

1. Less code isn't 100% valid #775

Builds

?

Related Pull Requests

?

Manual testing scenarios (*)

1. Setup a Magento installation (version 2.4.3 for example) and have the page builder module installed and enabled
2. Install the official less.js compiler: npm install less@3.13.1
3. Run rm -R var/view_preprocessed/* pub/static/*; bin/magento setup:static-content:deploy -f --theme=Magento/backend en_US
4. Validate the generated less code by linting the outputted files. Especially the styles.less file is important in scope of this PR: node_modules/.bin/lessc -l var/view_preprocessed/pub/static/adminhtml/Magento/backend/en_US/css/styles.less, notice that you get a bunch of warnings.
5. Make a backup of the pub/static/adminhtml directory
6. Apply the changes from this PR
7. Run step 3 and 4 again, notice that the warnings are gone now.
8. Compare the contents of the pub/static/adminhtml directory with the backup you created in step 5. There will be no differences.

End result: cleaner code, no warnings anymore, reduced code to maintain, reduced confusion when people try to understand what your code is doing.

Questions or comments

It would be nice if all Magento less code in all kinds of various repo's could get tested and validated by the official less.js compiler in some kind of static test so problematic code like this doesn't get added to the codebase.

(Very much unrelated, but your template for contributions isn't really friendly to the open source community since it asks for a lot of references to your private Jira board, which we can't provide ...)

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)

[hostep]

@sidolov: any chance the priority can get increased here? It's not only a simple cleanup like you think it would.

If you use a module to change the less compiler to the official one, these mistakes in the less code cause the backend to look broken.

[sidolov]

@hostep I got your point, thanks!

[paras89]

@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 re-request them if they don't show in a reasonable amount of time.

[paras89]

@hostep - can you merge develop into your fork branch and push so we can go ahead with processing this PR? Thanks!

[hostep]

@paras89: done! (I've rebased my changes on top of develop instead of having merged develop into my branch, which results in a cleaner git history, but the end result is the same).

@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 re-request them if they don't show in a reasonable amount of time.

[hostep]

Most failures in the static tests are probably going to be fixed by upgrading the coding-standard library to version 25 (ref magento/magento-coding-standard#405 & magento/magento-coding-standard#395), and the failures that will remain (about .admin__) probably shouldn't be fixed in scope of this PR as this usage is just all over the place.

[paras89]

@hostep can you add ignore directives so the static tests don't fail for the points you mentioned.

[hostep]

@paras89: I don't really think this is the right move here, shouldn't the coding standards be changed so it allows underscores? It's not like this will ever go away in the next few years in the Magento codebase. Also, why is "php code sniffer" even checking coding standards of css/less files, isn't that thing only supposed to check php code?

Also, can't we just ignore the static test failures in this PR? They really aren't important here in my opinion.

If nothing of my remarks make sense to you, and you really really really want me to add ignore directives, can you let me know how I can do that? Is it with @codingStandardsIgnoreStart and @codingStandardsIgnoreEnd directives, or are there other/better ways?

/cc @sivaschenko

[hostep]

@paras89, @sivaschenko: any input on my remarks? Especially around:

shouldn't the coding standards be changed so it allows underscores? It's not like this will ever go away in the next few years in the Magento codebase.

Also: less code is not the same as php code, so coding standards for writing less code should be very different than the ones for php code. Less code is being compiled sure, but the class names that are being used are outputted directly to the browser as is. PHP code on the other hand is being interpreted and the output that it gives is something that the coding standard is not checking (obviously), so the way you write php code isn't affecting the output, so checking for strict coding standards on php code can make sense. But you should treat less code and php code very differently if it comes to coding standards, especially regarding the way of writing class names, id's, actual css properties, ... because those get send to the browser as is.
So I would vote for not complaining anymore on underscores being used in css class names, because they are used all over the place and will not go away in the near (and probably long) future in the Magento code base. Right? And no, adding a bunch of ignore directives around all those underscore usages really isn't the solution, it will make the code a lot bigger and more harder to read.

[hostep]

@paras89, @sivaschenko: any update from you regarding my question(s) would be appreciated. Thanks! 🙂

[paras89]

@hostep - codingStandardsIgnoreStart is the right way to ignore.

As for css class name, the thinking is that best practice is to not have an underscore in them, the sniff is trying to prevent this problem in new code. Since the sniff was introduced at a later stage, the approach is to prevent new code from following bad practice, refactoring old code where possible and ignore where it is big scope to change.

Page Builder project is also not the appropriate project to discuss Magento wide coding standards. Feel free to open an issue in the coding standards repo if you'd like to take this discussion further.

[hostep]

Just to move this PR forwards, I've added some coding standard ignore directives.

I really don't like it because:

• the code looks incredibly ugly, makes it hard to read, which is actually the opposite of what coding standards should achieve
• if the coding standards ignore directive is applied around the entire css block (to make it more readable), it might ignore other violations as well, because there is no granularity in what it ignores
• underscores should be allowed in class names, because of the argumentation I already brought up earlier

@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 re-request them if they don't show in a reasonable amount of time.

[hostep]

It looks like an issue for this has already been created in the coding-standards repo: magento/magento-coding-standard#409, in typical Adobe fashion, it has been ignored for 2 months already

[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 re-request them if they don't show in a reasonable amount of time.

[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 re-request them if they don't show in a reasonable amount of time.

[hostep]

The two static test failures that remain (see below) will be fixed when coding-standard v25 will be used in the automation here like explained earlier, no idea why version 25 is not being used yet at the moment...

FILE: /var/www/html/app/code/Magento/PageBuilder/view/adminhtml/web/css/source/content-type/products/_default.less
------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------
 91 | WARNING | Expected 1 space after colon in style definition; newline found
 91 | WARNING | Expected 1 space after colon in style definition; 0 found
------------------------------------------------------------------------------------------------------------------

All other test failures, don't seem to have anything to do with the changes here proposed at first sight (but correct me when I'm wrong)

[hostep]

Rebased on latest develop branch (or should we now use future-develop??)

And removed my silly workaround for static test failures, now that magento/magento-coding-standard#409 is resolved finally (the tests will probably still fail, we'll probably need to wait until a new version of coding standards gets released, but maybe someone over here would be willing to just ignore these failures and just test and approve and merge, that would be really appreciated).

@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 re-request them if they don't show in a reasonable amount of time.

[hostep]

Rebased on latest develop branch, let's run the tests again, now that coding-standard 32 got released some weeks ago with a fix for a false positive that kinda blocked this PR.
There are more false positives in coding standard that as a workaround got fixed in the second commit in this PR here, that I believe shouldn't be necessary, but let us not stop this PR from being processed by this please. It's been already more than 2 years that this PR got opened.

I just retested everything locally, the generated css file still is no different then without these fixes, it just removes all invalid less code that breaks the official less.js compiler.

Would appreciate a quick merge this time, let's not keep this hanging for another 2 years please 🙂

@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]

@paras89: are you still around? Any chance you (or one of your colleagues) could pick this up soon-ish so we can get this included in Magento 2.4.7? Thanks!

[hostep]

@engcom-Hotel: what needs to happen to move this PR further? It has been open for 2 years and fixes an obvious problem.

[hostep]

Hey @ihor-sviziev, while we're dealing with less code fixes and cleanup, could you somehow try to move this one forward, it's been stuck for way too long. Thanks!

[ihor-sviziev]

@engcom-Hotel @sinhaparul can you please move it forward?

[hostep]

Any news here? Would be great if this could be included in Magento 2.4.7 ...

[engcom-Hotel]

@magento run Unit Tests

[engcom-Hotel]

@magento run all tests

[engcom-Hotel]

✔️ QA Passed

Preconditions:

This removes references to less classes in an adminhtml less file that are only defined in the frontend of Magento's Blank and/or Luma themes.

Manual testing scenario:

1. Setup a Magento installation (version 2.4.3 for example) and have the page builder module installed and enabled
2. Install the official less.js compiler: npm install less@3.13.1
3. Run rm -R var/view_preprocessed/* pub/static/*; bin/magento setup:static-content:deploy -f --theme=Magento/backend en_US
4. Validate the generated less code by linting the outputted files. Especially the styles.less file is important in scope of this PR: node_modules/.bin/lessc -l var/view_preprocessed/pub/static/adminhtml/Magento/backend/en_US/css/styles.less, notice that you get a bunch of warnings.
5. Make a backup of the pub/static/adminhtml directory
6. Apply the changes from this PR
7. Run step 3 and 4 again, notice that the warnings are gone now.
8. Compare the contents of the pub/static/adminhtml directory with the backup you created in step 5. There will be no differences.

Actual Result: ✔️
cleaner code, no warnings anymore, reduced code to maintain, reduced confusion when people try to understand what your code is doing.

After: ✔️

Before: ✖️

For Failed tests
The test seem flaky, hence moving forward with this PR.

Thanks

[engcom-Hotel]

@magento run all tests