-
Notifications
You must be signed in to change notification settings - Fork 165
Removing dependency on symfony/polyfill, is no longer required now th... #468
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
Removing dependency on symfony/polyfill, is no longer required now th... #468
Conversation
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.
Thanks for putting this together @hostep. I agree this can be safely removed. The coding standard should be installed as a development dependency though, so shouldn't cause any performance impact on production.
@fredden: that's not exactly true, dependencies of dev-dependencies can influence production dependencies being installed.
See the output in composer/composer#11641, by upgrading to v32 of coding-standards, it replaces the individual polyfill packages with the full one, and that one gets stored in the composer.lock file, so it gets installed in production as well.
I'm not a super fan of how dev dependencies work in composer, I've been experimenting with https://github.com/bamarni/composer-bin-plugin for over a year in some modules I've built (example), for better seperation of dev & prod composer dependencies so they don't influence each other as much, and I quite like it. It doesn't work with everything, I believe phpunit and composer-normalize can't be used without problems this way, but most dev dependencies (phpcs, php-cs-fixer, phpstan, ...) can be used without issues like this.
...at we dropped support for PHP 7.4
4753eeb to
8f01c09
Compare
@magento import pr to magento-commerce/magento-coding-standard repository
@sidolov the Pull Request is successfully imported.
a0b6fc9
into
magento:develop
@sidolov please can we get a new release which includes this change?
Got released in v33 today 🎉
...at we dropped support for PHP 7.4
Fixes #467
As explained in the issue, the symfony/polyfill dependency got added in 2ea3c39 for the use of the php function
str_starts_withwhich was added in PHP 8.0, at that time, the coding-standards repository still supported PHP 7.4, so it made sense at the time to add a polyfill.However, since 8d37ab7 support for PHP 7.4 was dropped. So this polyfill package is no longer needed.
I'd say, merge this and tag a new version 33 of coding-standards, so people using Magento aren't being forced to install the entire
symfony/polyfillpackage which comes with a whole lot of extra code that's not needed, maybe it will even cause a slight performance impact if all that code gets potentially loaded inside a Magento application.