-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Improve template minifier #33016
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
base: 2.4-develop
Are you sure you want to change the base?
Improve template minifier #33016
Conversation
Hi @blmage. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
You can find more information about the builds here ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review. For more details, please, review the Magento Contributor Guide documentation. 🕙 You can find the schedule on the Magento Community Calendar page. 📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket. 🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel ✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel |
Still need to :
but this looks promising so far, if adding a dependency on nikic/php-parser is not a problem. |
@magento run all tests |
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. |
@magento run all tests |
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. |
@magento run Functional Tests CE, Functional Tests B2B, Functional Tests EE, WebAPI Tests |
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. |
Fail tests may not related with changes from this pull request |
@magento run WebAPI Test, Functional Tests B2B |
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. |
@magento run WebAPI Test, Functional Tests B2B |
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. |
@magento run WebAPI Test |
Failed to run the builds. Please try to re-run them later. |
@magento run WebAPI Test |
The template minifier should not fix PHP code and hide errors from the user
For example, prevent "/\/path\/to\//" from being replaced with "/\/path\/to\"
- Do not attempt to parse empty contents - Prevent nesting problems with Xdebug - Catch and silence errors
efcb392
to
4e50eff
Compare
@magento run all tests |
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. |
Let's wait test results |
@magento run Database Compare, Functional Tests CE, Functional Tests EE, Integration Tests, Sample Data Tests CE, |
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. |
@magento run all tests |
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. |
I'm surprised that Magento wants to minify source code (PHP) instead of resulting html files. Generally the current approach is not how it should work. At least view_preprocessed files should ideally not be minified. At least the current setup leads to many issues and the new proposed solution here still does some black magic / voodoo with regex. The catch statement probably throws away too complex code, PHP 8 support might have to be added, the regex does not cover the optional trailing semicolon and so on (see #32153 (comment)). Please see also my comments at #32153 Truely, (complex) regular expressions are a safe way to shoot yourself into the foot, with a double-barrel shotgun. |
Generally I would also propose to support some ignore-comment syntax (like eslint-ignore, ts-ignore, htmlmin:ignore by html-minifier and more) to tell the minifier, to not touch code blocks if needed and have some control, so we don't have to patch core files in the future, if some minified code leads to such issues (there are way more cases where newlines and other elements are crucial). |
$content = $prettyPrinter->prettyPrintFile($ast); | ||
$heredocs = $prettyPrinter->getDelayedHeredocs(); | ||
} catch (\Error $error) { | ||
// Some PHP code is seemingly invalid, or too complex. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case doesn't it make more sense to just log a warning and return the unminified code instead of guessing that you could do a better job than the PHP parser library at minifying some mangled script using regex?
Description (*)
This PR aims to improve the template minifier by :
Related Pull Requests
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
It is very likely that there remains cases that are handled incorrectly, mostly in JS code now, but it also is probably impossible to make the process entirely safe as long as the minification happens on the templates, given the mix of PHP and HTML / CSS / JS code.
Contribution checklist (*)