Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Updated wikimedia/less.php from v3 to v4 #37842

Draft
wants to merge 1 commit into
base: 2.4-develop
Choose a base branch
from

Conversation

hostep
Copy link
Contributor

@hostep hostep commented Aug 3, 2023

Description (*)

Will update description later

TODO's:

  • describe PR
  • benchmark speed differences (with PHP 8.1 is around ~1 second faster, with PHP 8.2 is ~0.3 seconds faster, sidenote: the difference between 8.1 & 8.2 is insane, never expected this)
  • show diff of compiled css and try to explain it (still figuring part of this out)
  • complain about Magento's silly coding standards (again)
  • B2B Edition contains more differences than Magento Open Source - need to double check:
# both in Magento/blank & Magento/luma frontend themes:

 .negotiable-quote-quote-print .quote-block-content {
-  width: calc(44%);
+  width: calc(100% * 0.5 - 6rem);
 }

# both in Magento/backend & Magento/spectrum backend themes:

 .accordion .config input[name*='expiration_period'] + label {
   position: static;
   top: 5.5rem;
-  width: calc(96%);
+  width: calc(100% - 4rem);
 }
 .quotes-quote-print .quote-negotiated-price {
   float: right;
-  width: calc(44%);
+  width: calc((100%) * 0.5 - 6rem);
 }
 .quotes-quote-print .quote-catalog-price {
   float: left;
-  width: calc(44%);
+  width: calc((100%) * 0.5 - 6rem);
 }
  • PageBuilder also has one change - need to double check:
# in Magento/backend backend theme

 .focus-options .pagebuilder-options .pagebuilder-options-wrapper:after {
   background: #ffffff;
   border: 1px solid rgba(153, 153, 153, 0.7);
   bottom: -6px;
   content: '';
   display: block;
   height: 12px;
-  left: calc(49%);
+  left: calc(50% - 1px);
   position: absolute;
   transform: rotate(45deg) translateX(-50%);
   width: 12px;
   z-index: 1;
 }

Update: This looks like a fix, because 50% - 1px is not equal to 49%, so it seems like this is a bug that gets fixed

Related Pull Requests

Fixed Issues (if relevant)

  1. Fixes Difference in less compilation between php & nodejs library (grunt) with complicated calc expressions #37841

Manual testing scenarios (*)

  1. TODO !!!

Questions or comments

I've chosen to keep the constraints open so that version 3 is also still available to install, the changes in the calc compilation might be problematic for some shops, so this way we give an opportunity to let people stay at version 3. However, the 2 fixes in core magento themes will then cause issues for them...

In case of any protest about potential backwards compatibility breakage: we'll have to move to v4 anyways one day in the future (v3 will probably get abandoned and probably won't support newer PHP versions).

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)

@m2-assistant
Copy link

m2-assistant bot commented Aug 3, 2023

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
Copy link
Contributor Author

hostep commented Aug 3, 2023

Just for funsies:

@magento run all tests

@magento-automated-testing
Copy link

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
Copy link
Contributor

mrtuvn commented Aug 3, 2023

Safer way is fix as you mentioned
Second way is

width: ~"calc(100% - (@{checkout-tooltip-icon__font-size} + @{indent__s} + @{indent__xs}))";

=> this will output consistent results no matter how do you used node/grunt or php compilation
always output width: calc(100% - (express));

I think it's clean fix problem without broken anything.
Upgrade to v4 is worthy (of course php8 compat) but i'm not sure it's may come with breaking changes
In case update package you may need to update in this file too
https://github.com/magento/magento2/blob/2.4-develop/lib/internal/Magento/Framework/composer.json#L54
for consistent

@mrtuvn
Copy link
Contributor

mrtuvn commented Aug 13, 2023

also i have already created PR for exactly your case here
https://github.com/magento/magento2/pull/33643/files

@hostep
Copy link
Contributor Author

hostep commented Aug 21, 2023

Sorry, I was away on holiday. Also there are some changes in the wikimedia/less.php repository that haven't been published yet in a new release that I want to test first, because there are too much changes in the resulting compiled css which might be fixed by those unreleased changes. But I need to check.
I'll probably need 1 or 2 more weeks before I find some time to continue on this.

Thanks for pointing out #33643, I'll take a look at it as well in a couple of weeks.

@hostep
Copy link
Contributor Author

hostep commented Aug 31, 2023

Just tried again but with dev-master 06ab7ce of wikimedia/less.php as a test, and I still get maybe (?) unexpected changes. See small extract of diff between v3.2.0 and dev-master 06ab7ce of the library:

...
diff -urN /tmp/magento-scd-comparison/adminhtml/Magento/backend/en_US/css/styles.css pub/static/adminhtml/Magento/backend/en_US/css/styles.css
--- /tmp/magento-scd-comparison/adminhtml/Magento/backend/en_US/css/styles.css  2023-08-31 17:48:28
+++ pub/static/adminhtml/Magento/backend/en_US/css/styles.css   2023-08-31 17:50:55
@@ -3051,7 +3051,11 @@
 .action-select-wrap .abs-action-menu .action-submenu,
 .action-select-wrap .action-menu .action-submenu,
 .action-select-wrap .actions-split .dropdown-menu .action-submenu,
-.action-select-wrap .actions-split .action-menu .action-submenu {
+.action-select-wrap .actions-split .action-menu .action-submenu,
+.action-select-wrap .abs-action-menu .action-submenu .action-submenu,
+.action-select-wrap .actions-split .dropdown-menu .action-submenu .action-submenu,
+.action-select-wrap .actions-split .action-menu .action-submenu .action-submenu,
+.action-select-wrap .actions-split .action-menu .action-submenu .action-submenu {
   max-height: 45rem;
   overflow-y: auto;
 }
@@ -3059,14 +3063,22 @@
 .action-select-wrap .abs-action-menu .action-submenu ._disabled:hover,
 .action-select-wrap .action-menu .action-submenu ._disabled:hover,
 .action-select-wrap .actions-split .dropdown-menu .action-submenu ._disabled:hover,
-.action-select-wrap .actions-split .action-menu .action-submenu ._disabled:hover {
+.action-select-wrap .actions-split .action-menu .action-submenu ._disabled:hover,
+.action-select-wrap .abs-action-menu .action-submenu .action-submenu ._disabled:hover,
+.action-select-wrap .actions-split .dropdown-menu .action-submenu .action-submenu ._disabled:hover,
+.action-select-wrap .actions-split .action-menu .action-submenu .action-submenu ._disabled:hover,
+.action-select-wrap .actions-split .action-menu .action-submenu .action-submenu ._disabled:hover {
   background: #ffffff;
 }
 .action-select-wrap .action-menu ._disabled .action-menu-item,
 .action-select-wrap .abs-action-menu .action-submenu ._disabled .action-menu-item,
 .action-select-wrap .action-menu .action-submenu ._disabled .action-menu-item,
 .action-select-wrap .actions-split .dropdown-menu .action-submenu ._disabled .action-menu-item,
-.action-select-wrap .actions-split .action-menu .action-submenu ._disabled .action-menu-item {
+.action-select-wrap .actions-split .action-menu .action-submenu ._disabled .action-menu-item,
+.action-select-wrap .abs-action-menu .action-submenu .action-submenu ._disabled .action-menu-item,
+.action-select-wrap .actions-split .dropdown-menu .action-submenu .action-submenu ._disabled .action-menu-item,
+.action-select-wrap .actions-split .action-menu .action-submenu .action-submenu ._disabled .action-menu-item,
+.action-select-wrap .actions-split .action-menu .action-submenu .action-submenu ._disabled .action-menu-item {
   cursor: default;
   opacity: .5;
 }
...

Need to figure out if this is expected or not, as it makes the css files a bit bigger in filesize which is maybe not something we want.

No idea when I'll find time to continue with this investigation, I'm a bit busy in my afterwork life at the moment.

@m2-community-project m2-community-project bot added the Priority: P2 A defect with this priority could have functionality issues which are not to expectations. label Oct 12, 2023
@ihor-sviziev
Copy link
Contributor

Hi @hostep,
I think we should close this PR in vafor to #38335. Could you please confirm that?

@hostep
Copy link
Contributor Author

hostep commented Jan 18, 2024

No, not at all, these are 2 completely different things.

#38335 deals with compilation using less.js which is used for local development using grunt

This PR deals with compilation using less.php which is used by the setup:static-content:deploy command.

I'll try to find some more time to try to move this forwards.

@hostep
Copy link
Contributor Author

hostep commented Jan 23, 2024

Still WIP, this whole less compilation is a giant mess (no wonder that the guy who worked for Adobe some years ago to try to fix the frontend stack quit his job).

More observations:

  • the changes found in [WIP] Updated wikimedia/less.php from v3 to v4 #37842 (comment) match with the less.js compilation, so those are good changes I think (but haven't verified what they do exactly), see minimal test case at the bottom of this comment
  • there's a lot of activity the last few weeks in https://github.com/wikimedia/less.php/commits/master/ in where the less.php library's compilation is starting to become closer and closer to the original less.js compilation, none of this is released in a stable version yet, so I suggest we wait a few weeks longer
  • for example wikimedia/less.php@79ae252 significantly reduces the filesize of compiled css, as it compresses colors in a smaller form (just like less.js compilation does)
  • my note in the description at the top around the page-builder module has been updated, it seems like this is a good change that doesn't require intervention. Also the changes in the B2B modules look to be similar to that, but I don't have the source code of B2B at hand, so not 100% sure.
  • there were more things I discovered this evening, but it's hard to remember them all since it's such a complex thing this whole less compilation and I'm starting to doubt myself if I'm actually checking for the right things or not
  • ...

Minimal test case for the first bullet point, compile this with less.php v3 vs less.php v4 vs less.js and observe the differences or similarities:

.admin__scope-old {
    //
    //    Data table
    //--------------------------------------
    .data-table {
        thead,
        tfoot,
        th {
            background: red;
        }

        td,
        tbody tr th,
        tbody tr td {
            background: blue;
        }

        tfoot  {
            tr {
                &:last-child th,
                &:last-child td {
                    border: 0;
                }
            }
        }
    }

    .accordion .config .data-table {
        thead th,
        tfoot td {
            &:extend(.data-table thead all);
        }

        td {
            &:extend(.data-table td all);
        }

        tbody tr:nth-child(odd) td {
            &:extend(.data-table tbody tr:nth-child(odd) td all);
        }

        tfoot tr:last-child td {
            &:extend(tfoot tr:last-child td all);
        }
    }
}

If somebody else wants to take a stab at this, feel free to do so, you are free in taking over the changes I send here.

@hostep
Copy link
Contributor Author

hostep commented Feb 21, 2024

Tried it out with the recently released v4.2.0 of wikimedia/less.php today, but ran into a bug: wikimedia/less.php#107

And while working on this, I ran into another bug in Magento as well:

Error happened during deploy process: Magento\Framework\Exception\LocalizedException::__construct(): Argument #1 ($phrase) must be of type Magento\Framework\Phrase, string given, called in app/code/Magento/Deploy/Service/DeployPackage.php on line 142

This is because the $errorMessage (here) first gets assigned a Phrase but due to the concatenation with a bunch of strings, ends up as a string. And LocalizedException expects a Phrase, so this gives another error.

I'm not exactly sure yet how to deal with this other bug:

  • Replace LocalizedException with \Exception?
  • Additionally remove the translation for Compilation from source: ?
  • Change __('Compilation from source: ') to __('Compilation from source: %1') and put all the concatenated strings as param of the __ method?
  • But if we keep translating Compilation from source, I'm wondering if this code won't trigger in case it's a translated message since it references a non-translatable constant...

Anyway, still a lot of work to be done here.

@hostep
Copy link
Contributor Author

hostep commented May 1, 2024

The problem discovered in my last comment is being fixed over here: #38683

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Design/Frontend Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: review
Projects
Pull Requests Dashboard
  
Review in Progress
Development

Successfully merging this pull request may close these issues.

Difference in less compilation between php & nodejs library (grunt) with complicated calc expressions
4 participants