Problem/Motivation

Some of our dependencies pull in polyfill packages such as symfony/polyfill-php72, which are unnecessary as we already have a minimum dependency of PHP 7.3.

Proposed resolution

Add unnecessary polyfills to the "replace" section of composer.json as per https://github.com/symfony/polyfill#design

Remaining tasks

Decide whether to do this in composer.json or core/composer.json.

User interface changes

None

API changes

None (the polyfills do nothing as the functionality is already provided by the minimum version of PHP we use)

Data model changes

None

Release notes snippet

A couple of unneeded polyfills for versions of PHP prior to 7.3 were unnecessarily included in the core-recommended template and the lockfile. These have now been removed.

CommentFileSizeAuthor
#3 3168514-3.patch11.8 KBjungle

Comments

longwave created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Component: base system » composer
Parent issue: #3157296: Upgrade dependencies prior to 9.1.0 »
jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Status: Active » Needs review
StatusFileSize
new 3168514-3.patch11.8 KB

  1. Decide whether to do this in composer.json or core/composer.json.

    +1 to do this in DRUPAL_ROOT/composer.json.

    Steps:

    • Add unnecessary polyfills to the replace section in DRUPAL_ROOT/composer.json
    • COMPOSER_ROOT_VERSION=9.1.x-dev composer update --lock -vvv
  2. $ composer-lock-diff --no-links
+-------------------------+----------+---------+
| Production Changes      | From     | To      |
+-------------------------+----------+---------+
| paragonie/random_compat | v9.99.99 | REMOVED |
| symfony/polyfill-php70  | v1.18.1  | REMOVED |
| symfony/polyfill-php72  | v1.18.1  | REMOVED |
| symfony/polyfill-php73  | v1.18.1  | REMOVED |
+-------------------------+----------+---------+

    \Drupal::MINIMUM_PHP is 7.3.0

  3. $ composer show | grep polyfill
ralouphie/getallheaders              3.0.3     A polyfill for getallheaders.
symfony/polyfill-ctype               v1.18.1   Symfony polyfill for ctype functions
symfony/polyfill-iconv               v1.18.1   Symfony polyfill for the Iconv extension
symfony/polyfill-intl-idn            v1.18.1   Symfony polyfill for intl's idn_to_ascii and idn_to_utf8 functions
symfony/polyfill-intl-normalizer     v1.18.1   Symfony polyfill for intl's Normalizer class and related functions
symfony/polyfill-mbstring            v1.18.1   Symfony polyfill for the Mbstring extension
symfony/polyfill-php80               v1.18.1   Symfony polyfill backporting some PHP 8.0+ features to lower PHP versions
$ composer why ralouphie/getallheaders
guzzlehttp/psr7  1.6.1  requires  ralouphie/getallheaders (^2.0.5 || ^3.0.0)

    Required extensions in core/composer.json:

        "require": {
        "ext-date": "*",
        "ext-dom": "*",
        "ext-filter": "*",
        "ext-gd": "*",
        "ext-hash": "*",
        "ext-json": "*",
        "ext-pcre": "*",
        "ext-PDO": "*",
        "ext-session": "*",
        "ext-SimpleXML": "*",
        "ext-SPL": "*",
        "ext-tokenizer": "*",
        "ext-xml": "*",

    So, no more to remove?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

Looks good. We don't need random_compat nor polyfills for PHP 7.3 and below as we require PHP 7.3 minimum. We do need the remaining polyfills as the dependencies may use them, and it is better to polyfill than require an extension as some environments might not be able to provide the extensions.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

Actually the Symfony docs suggest replacing their polyfills with a version of 1.99, so I think this should become

    "replace": {
        "paragonie/random_compat": "9.99.99",
        "symfony/polyfill-php70": "1.99",
        "symfony/polyfill-php72": "1.99",
        "symfony/polyfill-php73": "1.99"
    },
jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Status: Needs work » Needs review

Re #5, from my understanding, 1.99 over there is just an example, It should be 1.18.1 in our case for symfony ones, but I think "*" is more appropriate here. As we do not want to change the version number frequently when there are new releases.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented

I tried changing them to 1.99, the following exception got:

Resolving dependencies through SAT
Looking at all rules.

Dependency resolution completed in 1.650 seconds
Analyzed 18686 packages to resolve dependencies
Analyzed 1051830 rules to resolve dependencies
Resolving dependencies through SAT
Looking at all rules.

Dependency resolution completed in 0.002 seconds
Package operations: 0 installs, 1 update, 0 removals
Updates: drupal/drupal:9.1.x-dev 803a34b

                                                                   
  [InvalidArgumentException]                                       
  Package is not installed: drupal/drupal-9.1.9999999.9999999-dev
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

The composer docs aren't quite clear but I thought 1.99 means that drupal/drupal provides (or claims to provide) that specific version - so when another package requires symfony/polyfill-php70:^1.10, 1.99 is a valid match. However, if something requires symfony/polyfill-php70:^2.0 then our version *won't* match and the polyfill would still be installed.

However, I also don't see why Symfony would release a 2.0 of any polyfill, or why we would ever need a PHP 7.0 polyfill at all, so I guess "*" is also fine as the version specifier.

  • catch committed df676dc on 9.1.x 
    Issue #3168514 by jungle, longwave: Remove unnecessary polyfills from...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed df676dc and pushed to 9.1.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +9.1.0 release notes

Nice!

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes

Adding the release note snippet.