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

Update usage of abandoned package #8094

Closed
7 tasks done
aaemnnosttv opened this issue Jan 16, 2024 · 3 comments
Closed
7 tasks done

Update usage of abandoned package #8094

aaemnnosttv opened this issue Jan 16, 2024 · 3 comments
Labels
P1 Medium priority PHP QA: Eng Requires specialized QA by an engineer Type: Infrastructure Engineering infrastructure & tooling

Comments

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Jan 16, 2024

Feature Description

The true/punycode package is no longer maintained and should be replaced.

Its repository suggests using symfony/polyfill-intl-idn as the recommended maintained replacement which is already installed as a dependency of guzzlehttp/guzzle. We should update our usage of the old package to use the proper PHP internals where safely polyfilled by the Symfony package.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The true/punycode package should be removed as a dependency
    • Its use internally should be replaced by core PHP functions idn_to_ascii and idn_to_utf8
  • The symfony/polyfill-intl-idn package should be required

Implementation Brief

  • Add symfony/polyfill-intl-idn v1.19.0 as a direct PHP dependency (require) in composer.json.
    • The version is important, as later version requires PHP7.
  • in includes/Core/Util/URL.php, refactor the class to replace the usage of Google\Site_Kit_Dependencies\TrueBV\Punycode with the native alternatives. which will be safely polyfilled by the aforementioned package.
    • Save the value of Punycode::PREFIX, xn-- as a const field Punycode_PREFIX in the class.
    • Replace the usage of $punycode->decode with idn_to_utf8.
    • Replace the usage of $punycode->decode with idn_to_ascii.
  • Remove the true/punycode package from composer.json

Test Coverage

  • All tests should continue to pass.

QA Brief

  • QA:Eng: Confirm the permute_site_hosts behaviour has not changed.

Changelog entry

  • Replace use of the abandoned true/punycode package with symfony/polyfill-intl-idn.
@aaemnnosttv aaemnnosttv added P1 Medium priority Type: Infrastructure Engineering infrastructure & tooling PHP labels Jan 16, 2024
@kuasha420 kuasha420 self-assigned this Jan 17, 2024
@kuasha420 kuasha420 removed their assignment Feb 4, 2024
@tofumatt tofumatt self-assigned this Feb 8, 2024
@tofumatt
Copy link
Collaborator

tofumatt commented Feb 8, 2024

IB ✅

@tofumatt tofumatt removed their assignment Feb 8, 2024
@benbowler benbowler self-assigned this Feb 9, 2024
@benbowler benbowler added the QA: Eng Requires specialized QA by an engineer label Feb 9, 2024
@benbowler benbowler removed their assignment Feb 9, 2024
@techanvil techanvil assigned techanvil and benbowler and unassigned techanvil Feb 9, 2024
@benbowler benbowler assigned techanvil and unassigned benbowler Feb 9, 2024
@techanvil techanvil assigned benbowler and unassigned techanvil Feb 12, 2024
@benbowler benbowler assigned techanvil and unassigned benbowler Feb 12, 2024
@techanvil techanvil removed their assignment Feb 13, 2024
@zutigrm zutigrm self-assigned this Feb 14, 2024
@zutigrm
Copy link
Collaborator

zutigrm commented Feb 14, 2024

QA:Eng verified ✅

  • idn_to_utf8 and idn_to_ascii are tested in URLTest::test_permute_site_hosts
  • Also did smoke test by connecting AdSense module

@zutigrm zutigrm removed their assignment Feb 14, 2024
@aaemnnosttv
Copy link
Collaborator Author

I did a bit of extra testing here just because this has bitten us before when used in an environment without the intl extension just to make sure the polyfill is working as expected.

Confirmed that idn_to_ascii didn't exist from PHP

server:~/example.com$ php -r 'var_dump(function_exists("idn_to_ascii"));'
bool(false)

Confirmed idn_to_ascii was defined by the SK-loaded polyfill 👍

server:~/example.com$ wp shell
wp> $rf = new ReflectionFunction('idn_to_ascii')
=> object(ReflectionFunction)#2939 (1) {
  ["name"]=>
  string(12) "idn_to_ascii"
}
wp> $rf->isUserDefined()
=> bool(true)
wp> $rf->getFileName()
=> string(127) "/.../public/wp-content/plugins/google-site-kit/third-party/symfony/polyfill-intl-idn/bootstrap.php"
wp> 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority PHP QA: Eng Requires specialized QA by an engineer Type: Infrastructure Engineering infrastructure & tooling
Projects
None yet
Development

No branches or pull requests

6 participants