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 [email protected]
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]

@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 [email protected]
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

[hostep]

Almost 3 years, but finally, thank you guys! :)