-
Notifications
You must be signed in to change notification settings - Fork 9.4k
matthiasmullie/minify - replacing JS & CSS libraries with a better one #40049
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?
Conversation
|
Hi @jakwinkler. Thank you for your contribution!
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
|
@magento run all tests |
…e, removing tubalmartin/cssmin
…2 into matthiasmullie_minify
|
@magento run all tests |
|
@jakwinkler, please review the comments from Copilot and let us know if you would like anything to be fixed based on them. If no, I'll review the pull request |
|
@magento run all tests |
|
@magento run all tests |
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.
Hi @jakwinkler
Could you please fix failed static and unit tests
|
@engcom-Hotel, this is more about platform health - replacing old not supported tools that currently Magento using with the ones that supported. I do agree that the selected tool has to be reviewed, but I don't think it can be treated as a feature request. |
|
@engcom-Hotel I agree with @ihor-sviziev, this is not a feature but a require step in maintaining platform health. Tests fail on none-related part of the code to that PR... |
|
I've updated |
|
@magento run all tests |
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.
Pull Request Overview
This PR replaces the existing JShrink and CSSmin libraries with the maintained matthiasmullie/minify package for JS and CSS minification in Magento.
- Adds matthiasmullie/minify and removes tedivm/jshrink and tubalmartin/cssmin from dependencies
- Updates JShrink and CSSmin adapter implementations to use MatthiasMullie\Minify
- Adjusts return types and cleans up old library-specific code
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lib/internal/Magento/Framework/composer.json | Added matthiasmullie/minify; removed tedivm/jshrink |
| lib/internal/Magento/Framework/Code/Minifier/Adapter/Js/JShrink.php | Switched adapter to use MatthiasMullie\Minify\JS |
| lib/internal/Magento/Framework/Code/Minifier/Adapter/Css/CSSmin.php | Switched adapter to use MatthiasMullie\Minify\CSS |
| composer.json | Added matthiasmullie/minify; removed old CSS/JS minifiers |
Comments suppressed due to low confidence (7)
lib/internal/Magento/Framework/Code/Minifier/Adapter/Css/CSSmin.php:12
- Update the class docblock to reflect the use of
MatthiasMullie\Minify\CSSinstead of the old CSSmin library.
* Adapter for CSSmin library
composer.json:44
- [nitpick] Dependencies in
composer.jsonare not consistently sorted; consider ordering them alphabetically to improve readability and future merges.
"duosecurity/duo_api_php": "^1.1",
lib/internal/Magento/Framework/Code/Minifier/Adapter/Js/JShrink.php:15
- [nitpick] The class name
JShrinkis now misleading since it no longer uses the JShrink library; consider renaming it to something likeMatthiasMullieJsMinifierfor clarity.
class JShrink implements AdapterInterface
lib/internal/Magento/Framework/Code/Minifier/Adapter/Css/CSSmin.php:14
- [nitpick] The class name
CSSminno longer reflects the underlying library; consider renaming it to something likeMatthiasMullieCssMinifierfor better maintainability.
class CSSmin implements AdapterInterface
lib/internal/Magento/Framework/Code/Minifier/Adapter/Js/JShrink.php:27
- Add or update unit tests for this adapter to verify that the new
MatthiasMullie\Minify\JSintegration produces equivalent output to the previous implementation.
$minifier = new Minify\JS();
lib/internal/Magento/Framework/Code/Minifier/Adapter/Js/JShrink.php:25
- Add a parameter type hint for
$contentto improve type safety (e.g.,public function minify(string $content): string).
public function minify($content): string
lib/internal/Magento/Framework/Code/Minifier/Adapter/Css/CSSmin.php:22
- Add a parameter type hint for
$content(e.g.,public function minify(string $content): string) to align with strict typing.
public function minify($content): string
|
Thank you @ihor-sviziev and @jakwinkler for your valuable feedback on this PR. I completely agree with your assessment that this is a platform health improvement rather than a feature request. Replacing outdated and unsupported minification libraries with maintained alternatives like matthiasmullie/minify is indeed a necessary maintenance step to ensure the platform's stability, security, and performance. I've already sent an email to the PO explaining the importance of this change for platform health and requesting approval. I've emphasized that:
I appreciate your patience and contributions to maintaining Magento's platform health. I'll update this thread once I receive feedback from the PO. Thank you for your continued support. |
|
@engcom-Hotel, |
|
@magento run all tests |
|
Hi @jakwinkler, Thank you for your contribution! The Magento core engineering team is working on the issue which you have addressed in this PR. Team will cherry pick the commits from your PR and may do further implementation to cover few more scenarios as needed. We will reach out to you if we need more information. For now, you can pause work on this PR. Thank you once again! |
|
@magento create issue |
|
Hi Everyone, As part of our investigation, we compared JShrink and MatthiasMullie Minify libraries to evaluate frontend performance and build output differences. 🔍 Summary of Findings:
⚙️ Additional Performance Validation:
jshrink_jmeter_report_20251013_161328.zip ✅ Conclusion: However, we welcome any additional opinions or insights from the team before closing this evaluation. |
|
|
@ihor-sviziev I agree with your statement — we have already evaluated this from all possible aspects and shared the details. Regarding your point about choosing a well-maintained library with better code coverage, based on my analysis, I didn’t find any evidence indicating that the current package is poorly maintained or lacks sufficient test coverage. Please refer to the following details I reviewed — both packages are being released frequently, and the code coverage appears to be good in both cases. Kindly let me know if there’s anything I might have missed or if you have any additional insights to share. |
|
@jakwinkler, can you please clarify? |
|
Please let us know if anyone has a different opinion. Otherwise, we are planning to close this PR and the respective GitHub issue, as we’ve agreed that no action is required at this time. |



Description (*)
I've replaced old CSS / JS minification libraries with https://github.com/matthiasmullie/minify - which is maintained, up to date and covered with tests.
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)
Resolved issues: