search

ampproject / amp-toolbox-php

AMP Optimizer PHP library
Apache License 2.0
74 stars 23 forks source link

PHP 8.4 Compatibility #549

Open swissspidy opened 3 months ago

swissspidy commented 3 months ago

I wanted to test PHP 8.4 compatibility in the Web Stories plugin a bit early and noticed a few deprecation warnings coming from the AMP PHP Toolbox:

PHP Deprecated:  Implicitly marking parameter $node as nullable is deprecated, the explicit nullable type must be used instead in /home/runner/work/web-stories-wp/web-stories-wp/third-party/vendor/ampproject/amp-toolbox/src/Dom/Document.php on line 343
PHP Deprecated:  Implicitly marking parameter $node as nullable is deprecated, the explicit nullable type must be used instead in /home/runner/work/web-stories-wp/web-stories-wp/third-party/vendor/ampproject/amp-toolbox/src/Dom/Document.php on line 353
PHP Deprecated:  Implicitly marking parameter $node as nullable is deprecated, the explicit nullable type must be used instead in /home/runner/work/web-stories-wp/web-stories-wp/vendor/ampproject/amp-toolbox/src/Dom/Document.php on line 416
PHP Deprecated:  Implicitly marking parameter $node as nullable is deprecated, the explicit nullable type must be used instead in /home/runner/work/web-stories-wp/web-stories-wp/vendor/ampproject/amp-toolbox/src/Dom/Document.php on line 427
PHP Deprecated:  Implicitly marking parameter $configuration as nullable is deprecated, the explicit nullable type must be used instead in /home/runner/work/web-stories-wp/web-stories-wp/third-party/vendor/ampproject/amp-toolbox/src/Optimizer/TransformationEngine.php on line 51

This is related to the PHP RFC to deprecate implicitly nullable parameter types. See https://php.watch/versions/8.4/implicitly-marking-parameter-type-nullable-deprecated for a good explanation & ways to address this.

The problem is that nullable types were only added in PHP 7.1 but this library still supports PHP 5.6.

I suggest to just bump the minimum PHP requirement to PHP 7.4 (like the AMP plugin and the Web Stories plugin) in a 0.12 release and highlight this as a breaking change.

westonruter commented 3 months ago

@thelovekesh Could you make this change?