Problem/Motivation

There are some methods (sorting is one I faced) after https://github.com/php/php-src/pull/11200 (included in alpha1)

https://github.com/php/php-src/commit/85338569debd3f669ef5bc793822b2d9f3...

Steps to reproduce

$ php core/modules/jsonapi/src/Normalizer/Value/TemporaryArrayObjectThrowingExceptions.php
PHP Deprecated:  Return type of Drupal\jsonapi\Normalizer\Value\TemporaryArrayObjectThrowingExceptions::asort($flags = Drupal\jsonapi\Normalizer\Value\SORT_REGULAR): bool should either be compatible with ArrayObject::asort(int $flags = SORT_REGULAR): true, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/web/core/modules/jsonapi/src/Normalizer/Value/TemporaryArrayObjectThrowingExceptions.php on line 40

Deprecated: Return type of Drupal\jsonapi\Normalizer\Value\TemporaryArrayObjectThrowingExceptions::asort($flags = Drupal\jsonapi\Normalizer\Value\SORT_REGULAR): bool should either be compatible with ArrayObject::asort(int $flags = SORT_REGULAR): true, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/web/core/modules/jsonapi/src/Normalizer/Value/TemporaryArrayObjectThrowingExceptions.php on line 40
PHP Deprecated:  Return type of Drupal\jsonapi\Normalizer\Value\TemporaryArrayObjectThrowingExceptions::ksort($flags = Drupal\jsonapi\Normalizer\Value\SORT_REGULAR): bool should either be compatible with ArrayObject::ksort(int $flags = SORT_REGULAR): true, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/web/core/modules/jsonapi/src/Normalizer/Value/TemporaryArrayObjectThrowingExceptions.php on line 144

Deprecated: Return type of Drupal\jsonapi\Normalizer\Value\TemporaryArrayObjectThrowingExceptions::ksort($flags = Drupal\jsonapi\Normalizer\Value\SORT_REGULAR): bool should either be compatible with ArrayObject::ksort(int $flags = SORT_REGULAR): true, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/web/core/modules/jsonapi/src/Normalizer/Value/TemporaryArrayObjectThrowingExceptions.php on line 144
PHP Deprecated:  Return type of Drupal\jsonapi\Normalizer\Value\TemporaryArrayObjectThrowingExceptions::uasort($callback): bool should either be compatible with ArrayObject::uasort(callable $callback): true, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/web/core/modules/jsonapi/src/Normalizer/Value/TemporaryArrayObjectThrowingExceptions.php on line 285

Deprecated: Return type of Drupal\jsonapi\Normalizer\Value\TemporaryArrayObjectThrowingExceptions::uasort($callback): bool should either be compatible with ArrayObject::uasort(callable $callback): true, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/web/core/modules/jsonapi/src/Normalizer/Value/TemporaryArrayObjectThrowingExceptions.php on line 285
PHP Deprecated:  Return type of Drupal\jsonapi\Normalizer\Value\TemporaryArrayObjectThrowingExceptions::uksort($callback): bool should either be compatible with ArrayObject::uksort(callable $callback): true, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/web/core/modules/jsonapi/src/Normalizer/Value/TemporaryArrayObjectThrowingExceptions.php on line 304

Deprecated: Return type of Drupal\jsonapi\Normalizer\Value\TemporaryArrayObjectThrowingExceptions::uksort($callback): bool should either be compatible with ArrayObject::uksort(callable $callback): true, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/web/core/modules/jsonapi/src/Normalizer/Value/TemporaryArrayObjectThrowingExceptions.php on line 304
PHP Deprecated:  Return type of Drupal\jsonapi\Normalizer\Value\TemporaryArrayObjectThrowingExceptions::natsort(): bool should either be compatible with ArrayObject::natsort(): true, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/web/core/modules/jsonapi/src/Normalizer/Value/TemporaryArrayObjectThrowingExceptions.php on line 172

Deprecated: Return type of Drupal\jsonapi\Normalizer\Value\TemporaryArrayObjectThrowingExceptions::natsort(): bool should either be compatible with ArrayObject::natsort(): true, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/web/core/modules/jsonapi/src/Normalizer/Value/TemporaryArrayObjectThrowingExceptions.php on line 172
PHP Deprecated:  Return type of Drupal\jsonapi\Normalizer\Value\TemporaryArrayObjectThrowingExceptions::natcasesort(): bool should either be compatible with ArrayObject::natcasesort(): true, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/web/core/modules/jsonapi/src/Normalizer/Value/TemporaryArrayObjectThrowingExceptions.php on line 158

Deprecated: Return type of Drupal\jsonapi\Normalizer\Value\TemporaryArrayObjectThrowingExceptions::natcasesort(): bool should either be compatible with ArrayObject::natcasesort(): true, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/web/core/modules/jsonapi/src/Normalizer/Value/TemporaryArrayObjectThrowingExceptions.php on line 158

Proposed resolution

Add [#\ReturnTypeWillChange] attribute to keep code compatible as we should keep compatibility with 8.1
PHP 8.2 added true return type https://wiki.php.net/rfc/true-type

Remaining tasks

- find all affected places
- patch, review and commit

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#2 3366288-2.patch2.84 KBandypost

Comments

andypost created an issue. See original summary.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Active » Needs review
StatusFileSize
new 3366288-2.patch2.84 KB
andypost’s picture

andypost
he/him
Primary language Russian
commented

Other files are not affected, script to check find core -name '*.php' -exec php -l {} \; >/dev/null

andypost’s picture

andypost
he/him
Primary language Russian
commented

Baseline test runs

- 10.1 https://www.drupal.org/pift-ci-job/2691058
- 11.0 https://www.drupal.org/pift-ci-job/2691057

andypost’s picture

andypost
he/him
Primary language Russian
commented

Only 2 tests failed comparing to 118 in base run

Test 'Drupal\Tests\filter\Kernel\FilterKernelTest::testCaptionFilter' ended

Failed asserting that two strings are identical.
Expected :'<img data-caption="This is a &lt;a href=&quot;https://www.drupal.org&quot;&gt;quick&lt;/a&gt; test…" src="llama.jpg" />'
Actual   :'<img data-caption="This is a &amp;lt;a href=&amp;quot;https://www.drupal.org&amp;quot;&amp;gt;quick&amp;lt;/a&amp;gt; test…" src="llama.jpg" />'

which means & escaped twice

Test 'Drupal\Tests\editor\Unit\EditorXssFilter\StandardTest::testFilterXss with data set #190 ('<img src="butterfly.jpg" data...t;" />', '<img src="butterfly.jpg" data...t;" />')' ended

Failed asserting that two strings are identical.
Expected :'<img src="butterfly.jpg" data-caption="&amp;lt;script&amp;gt;alert();&amp;lt;/script&amp;gt;" />'
Actual   :'<img src="butterfly.jpg" data-caption="&amp;amp;lt;script&amp;amp;gt;alert();&amp;amp;lt;/script&amp;amp;gt;" />'

the same

longwave’s picture

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

Looks OK to me.

I did wonder if we should just change the return types here, and perhaps make the class final, but that can be done in a followup so we can discuss it.

edit: we can't make it final because something does extend it.

  • catch committed af9c329e on 10.1.x 
    Issue #3366288 by andypost, longwave: Add [#\ReturnTypeWillChange]...

  • catch committed 99c902a2 on 11.x 
    Issue #3366288 by andypost, longwave: Add [#\ReturnTypeWillChange]...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x and 10.1.x, thanks!

andypost’s picture

andypost
he/him
Primary language Russian
commented

@longwave Sadly we can't add true type hint as it was added in 8.2 https://wiki.php.net/rfc/true-type

andypost’s picture

andypost
he/him
Primary language Russian
commented

Example of using bool on 8.1

/srv # php -v
PHP 8.1.20 (cli) (built: Jun  7 2023 09:47:40) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.1.20, Copyright (c) Zend Technologies
    with Zend OPcache v8.1.20, Copyright (c), by Zend Technologies

/srv # php core/modules/jsonapi/src/Normalizer/Value/TemporaryArrayObjectThrowingExceptions.php
PHP Fatal error:  Cannot use 'Drupal\jsonapi\Normalizer\Value\true' as class name as it is reserved in /srv/core/modules/jsonapi/src/Normalizer/Value/TemporaryArrayObjectThrowingExceptions.php on line 158

Fatal error: Cannot use 'Drupal\jsonapi\Normalizer\Value\true' as class name as it is reserved in /srv/core/modules/jsonapi/src/Normalizer/Value/TemporaryArrayObjectThrowingExceptions.php on line 158
andypost’s picture

andypost
he/him
Primary language Russian
commented
Version: 10.1.x-dev » 11.x-dev
Status: Fixed » Needs review
Related issues: +#3366843: Fix tests broken on PHP 8.3

Filed follow-up for #6

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Fixed
andypost’s picture

andypost
he/him
Primary language Russian
commented
Version: 11.x-dev » 10.1.x-dev

sorry

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Ahh thanks @andypost, I did not know that true was only PHP 8.2+.

Status: Fixed » Closed (fixed)

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