Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 18 months ago

#53635 closed task (blessed) (fixed)

PHP 8.1: various compatibility fixes for WordPress 5.9

Reported by: sergeybiryukov's profile SergeyBiryukov Owned by: hellofromtonya's profile hellofromTonya
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: General Keywords: php81 has-patch has-unit-tests
Focuses: coding-standards Cc:

Description

Previously:

PHP 8.1 release is planned for November 25, 2021. WordPress 5.9 is currently planned for December 2021 and should aim to support PHP 8.1 as much as possible.

This ticket can be used as an "epic", allowing a variety of small patches each fixing a specific failure to be added to and committed against this ticket.

For patches addressing all instances of failures related to one specific PHP 8.1 change across the codebase, separate tickets should still be opened.

For an example of issues/patches with separate tickets, see:

  • #52825 PHP 8.1: MySQLi error reporting value changes
  • #53299 PHP 8.1: Update is_serialized function to accept Enums
  • #53465 PHP 8.1.: the default value of the flags parameter for htmlentities() et all needs to be explicitly set

When opening a separate ticket, please tag it with the php81 keyword, so these can be easily found using this report: https://core.trac.wordpress.org/query?keywords=~php81

Attachments (10)

53635-jsonserialize.patch (847 bytes) - added by jrf 3 years ago.
Fix a "SomeClass::jsonSerialize() should be compatible with JsonSerializable::jsonSerialize(): mixed" error
53635-WP_Hook-iterator-arrayaccess.patch (3.3 KB) - added by jrf 3 years ago.
Fix various "Deprecated: Return type of WP_Hook::[METHODNAME]() should either be compatible with ...., or the #[ReturnTypeWillChange] attribute should be used to temporarily suppress the notice"
53635-WP_Theme-arrayaccess.patch (1.7 KB) - added by jrf 3 years ago.
Fix various "Deprecated: Return type of WP_Theme::[METHODNAME]($offset) should either be compatible with ArrayAccess::[METHODNAME](): type, or the #[ReturnTypeWillChange] attribute should be used to temporarily suppress the notice"
53635-tests-countable.patch (682 bytes) - added by jrf 3 years ago.
PHP 8.1/Tests: fix deprecation notice - another one for the "return type will change" series
53635-05-null-to-non-nullable-in-testcode.patch (1.8 KB) - added by jrf 3 years ago.
PHP 8.1: Tests_Functions_DoEnclose: fix bug in test
53635-06-improve-auto_detect_line_endings-fix.patch (1.3 KB) - added by jrf 3 years ago.
Pomo/ PHP 8.1: improve the fix for auto_detect_line_endings ... as per the inline comment.
53635-07-incorrect-default-value-for-http_build_query.patch (1.8 KB) - added by jrf 3 years ago.
PHP 8.1: WP_Sitemaps_Provider::get_sitemap_url(): fix deprecation notice
53635-08-null-to-non-nullable-in-testcode.patch (1.4 KB) - added by jrf 3 years ago.
PHP 8.1 | Tests: fix bug in Tests_Admin_IncludesPlugin::test_get_plugin_files_folder() The Tests_Admin_IncludesPlugin::_create_plugin() expects the first parameter to be a text string to be written to a plugin file using fwrite(). Passing null causes a fwrite(): Passing null to parameter #2 ($data) of type string is deprecated notice. Ref: https://www.php.net/manual/en/function.fwrite
Screenshot 2021-11-30 at 11.10.49.png (314.6 KB) - added by spacedmonkey 2 years ago.
Screenshot of unit test run in PHP 8.1
53635-09-script-translations.patch (2.0 KB) - added by Chouby 2 years ago.
Avoid notices in untrailingslashit() due to the default value of script translations path being null instead of an empty string. Also the doc blocks don't document null as an accepted value for the path.

Download all attachments as: .zip

Change History (180)

#1 @SergeyBiryukov
3 years ago

In 51396:

Code Modernization: Only check collation in wpdb methods if the query is not empty.

This avoids a deprecation notice on PHP 8.1 caused by passing null instead of a string to ltrim() in wpdb::check_safe_collation(), and maintains the current behaviour.

Follow-up to [30345], [32162], [33455].

See #53635.

@jrf
3 years ago

Fix a "SomeClass::jsonSerialize() should be compatible with JsonSerializable::jsonSerialize(): mixed" error

#2 @jrf
3 years ago

Some context for the patch I just uploaded:

This relates to the Return types for internal methods RFC in PHP 8.1 and in particular, the change made in PHP PR #7051, which adds a mixed return type to the JsonSerializable::jsonSerialize() interface method.

WP only contains one (test) class which implements the JsonSerializable interface and this patch fixes the issue for that class.

Basically, as of PHP 8.1, the jsonSerialize() method in classes which implement the JsonSerializable interface are expected to have a return type declared. The return type should be mixed or a more specific type.
This complies with the Liskov principle of covariance, which allows the return type of a child overloaded method to be more specific than that of the parent.

The problem with this is that:

  1. The mixed return type was only introduced in PHP 8.0 and therefore cannot be used as WP has a minimum PHP version of 5.6.
  2. Return types in general were only introduced in PHP 7.0 and therefore cannot be used as WP has a minimum PHP version of 5.6.

Luckily an attribute has been added to silence the deprecation warning.

While attributes are a PHP 8.0+ feature, due to the choice of the #[] syntax, in PHP < 8.0, attributes will just be ignored and treated as comments, so there is no drawback to using the attribute.

So to fix this, there is basically a choice between two options:

  • Either decouple from the interface. I'd strongly recommend against this as the interface is being implemented for a reason.
  • Or use the attribute, which is what this patch does.

For more details, see the discussion in the upstream PR: https://github.com/php/php-src/pull/7051

@jrf
3 years ago

Fix various "Deprecated: Return type of WP_Hook::[METHODNAME]() should either be compatible with ...., or the #[ReturnTypeWillChange] attribute should be used to temporarily suppress the notice"

@jrf
3 years ago

Fix various "Deprecated: Return type of WP_Theme::[METHODNAME]($offset) should either be compatible with ArrayAccess::[METHODNAME](): type, or the #[ReturnTypeWillChange] attribute should be used to temporarily suppress the notice"

#3 @jrf
3 years ago

Two more patches for the same type of errors as before. The same explanation as outlined in comment #2 applies to these patches as well.

#4 @SergeyBiryukov
3 years ago

In 51517:

Code Modernization: Fix "JsonSerializable_Object::jsonSerialize() should be compatible with JsonSerializable::jsonSerialize(): mixed" error on PHP 8.1.

This relates to the Return types for internal methods RFC in PHP 8.1 and in particular, the change made in PHP PR #7051, which adds a mixed return type to the JsonSerializable::jsonSerialize() interface method.

WordPress only contains one (test) class which implements the JsonSerializable interface and this commit fixes the issue for that class.

As of PHP 8.1, the jsonSerialize() method in classes which implement the JsonSerializable interface are expected to have a return type declared. The return type should be mixed or a more specific type. This complies with the Liskov principle of covariance, which allows the return type of a child overloaded method to be more specific than that of the parent.

The problem with this is that:

  1. The mixed return type was only introduced in PHP 8.0.
  2. Return types in general were only introduced in PHP 7.0.

WordPress still has a minimum PHP version of 5.6, so adding the return type is not feasible for the time being.

The solution chosen for now is to add an attribute to silence the deprecation warning. While attributes are a PHP 8.0+ feature, due to the choice of the #[] syntax, in PHP < 8.0, attributes will just be ignored and treated as comments, so there is no drawback to using the attribute.

Props jrf.
See #53635.

#5 @hellofromTonya
3 years ago

Notes from live working session:

  • Check the docs generator to determine if there any issues from the placement of the attribute (which is above the method/function and below the existing DocBlock)

This check needs action before the docs are generated for 5.9.

#6 @jrf
3 years ago

Linking #53858 for reference - "PHP 8.1: syntax error due to new 'readonly' property"

#7 @SergeyBiryukov
3 years ago

In 51529:

Code Modernization: Silence the deprecation warnings for missing return type in WP_Theme.

This fixes the "Deprecated: Return type of WP_Theme::[METHODNAME]($offset) should be compatible with ArrayAccess::[METHODNAME](): type" warnings on PHP 8.1.

PHP native interfaces now have declared return types and methods in classes implementing these interfaces need to either have the return type declared (in a covariant compatible manner with the PHP native interface method declaration), or need to silence the deprecation warning using the #[ReturnTypeWillChange] attribute.

Follow-up to [51517].

Props jrf.
See #53635.

#8 @SergeyBiryukov
3 years ago

In 51530:

Code Modernization: Silence the deprecation warnings for missing return type in WP_Hook.

This fixes the "Deprecated: Return type of WP_Hook::[METHODNAME]() should be compatible with ArrayAccess::[METHODNAME](): type" warnings on PHP 8.1.

PHP native interfaces now have declared return types and methods in classes implementing these interfaces need to either have the return type declared (in a covariant compatible manner with the PHP native interface method declaration), or need to silence the deprecation warning using the #[ReturnTypeWillChange] attribute.

Follow-up to [51517], [51529].

Props jrf.
See #53635.

#9 @SergeyBiryukov
3 years ago

In 51531:

Code Modernization: Silence the deprecation warnings for missing return type in WP_REST_Request.

This fixes the "Deprecated: Return type of WP_REST_Request::[METHODNAME]($offset) should be compatible with ArrayAccess::[METHODNAME](): type" warnings on PHP 8.1.

PHP native interfaces now have declared return types and methods in classes implementing these interfaces need to either have the return type declared (in a covariant compatible manner with the PHP native interface method declaration), or need to silence the deprecation warning using the #[ReturnTypeWillChange] attribute.

Follow-up to [51517], [51529], [51530].

Props jrf.
See #53635.

#10 @SergeyBiryukov
3 years ago

In 51532:

Code Modernization: Silence the deprecation warnings for missing return type in WP_Block_List.

This fixes the "Deprecated: Return type of WP_Block_List::[METHODNAME]() should be compatible with ArrayAccess::[METHODNAME](): type" warnings on PHP 8.1.

PHP native interfaces now have declared return types and methods in classes implementing these interfaces need to either have the return type declared (in a covariant compatible manner with the PHP native interface method declaration), or need to silence the deprecation warning using the #[ReturnTypeWillChange] attribute.

Follow-up to [51517], [51529], [51530], [51531].

Props jrf.
See #53635.

#11 @SergeyBiryukov
3 years ago

In 51533:

Code Modernization: Pass correct default value to new DateTime() in wp_default_packages_inline_scripts().

This fixes a "Deprecated: DateTime::__construct(): Passing null to parameter #1 ($datetime) of type string is deprecated" warning on PHP 8.1.

Follow-up to [49083].

See #53635.

This ticket was mentioned in Slack in #core by sergey. View the logs.


3 years ago

@jrf
3 years ago

PHP 8.1/Tests: fix deprecation notice - another one for the "return type will change" series

#13 @jrf
3 years ago

FYI: some info about some of the runtime dependencies of WordPress (incomplete)

Sodium Compat

I've opened a PR to fix various PHP 8.1 deprecation warnings about the return type change, as well as a PR to turn on CI testing against PHP 8.1.

With the open PHP 8.1 PR merged, there would still be one issue remaining, but how to fix that should be decided by the maintainer in this case.

Random Compat

I've opened a PR to turn on CI testing against PHP 8.1.

There are two test failures in their tests, but how to fix these should be decided by the maintainer in this case.

PHPMailer

Testing against PHP 8.1 has been turned on over a month ago.

I've pulled two fixes and both have been merged:

As things stand, the PHPMailer tests pass against PHP 8.1.

No release containing the fixes is available yet.

Requests

Testing against PHP 8.1 has been turned on the other week.

I've pulled three fixes and these have been merged:

As things stand, the Requests tests pass against PHP 8.1.

No release containing the fixes is available yet.

GetID3

CI (linting) has been running against PHP 8.1 since December 2020 🎉

The other side of the coin is that there are no tests, so, aside from there not being any parse errors, the actual PHP 8.1 compatibility status of the repo is anyone's guess.

SimplePie

An issue has been open since last year about getting the tests running on PHP 8.0, let alone PHP 8.1. So far there has been no response from the maintainers...

This ticket was mentioned in PR #1556 on WordPress/wordpress-develop by jrfnl.


3 years ago
#14

  • Keywords has-patch has-unit-tests added

Fixes strpos(): Passing null to parameter #1 ($haystack) of type string is deprecated notices on PHP 8.1.

Impact: 38 of the pre-existing tests are affected by this issue.

Note: trac ticket link below not added yet on purpose while this ticket is still WIP.

Trac ticket:

This ticket was mentioned in PR #1558 on WordPress/wordpress-develop by jrfnl.


3 years ago
#15

Fixes parse_str(): Passing null to parameter #1 ($string) of type string is deprecated notices on PHP 8.1.

Impact: 311 of the pre-existing tests are affected by this issue.

Note: trac ticket link below not added yet on purpose while this ticket is still WIP.

Trac ticket:

#16 @jrf
3 years ago

  • Keywords has-patch has-unit-tests removed

Linking #53897 for reference: PHP 8.1: strftime() is deprecated

#17 @SergeyBiryukov
3 years ago

In 51588:

Build/Test Tools: Enable running the tests on PHP 8.1.

PHP 8.1 is expected to be released at the end of November 2021.

Enabling the tests to run in CI on PHP 8.1 allows us to get WordPress ready in time.

As an interim measure, while working through the PHP 8.1 issues, separate conditional steps are added to run the tests on PHP 8.1 with the continue-on-error option. That allows the test builds to show as "successful" if all non-PHP 8.1 test runs pass.

Follow-up to [51517], [51543], [51545], [51574], [51582], [51586].

Props jrf.
Fixes #53891. See #53635.

#18 @jrf
3 years ago

Sodium Compat has just released a PHP 8.1 compatible version. Ticket #53907 has been opened to handle the upgrade.

#19 @jrf
3 years ago

FYI: I've been preparing various patches for known PHP 8.1 test failures - mostly "passing null to non-nullable" errors - and adding dedicated, targeted tests for these.
Unfortunately, though not unexpected, the functions affected often don't have any tests yet or the tests which are available, need a lot of work.

So slow going, but we've still got a few months time and it would be good to significantly improve the tests along with making these fixes.

Some WIP PRs are already open on GH (not yet linked to this ticket) and more are forthcoming.

#20 @jrf
3 years ago

@SergeyBiryukov When you've got some time, would be great if the 53635-tests-countable.patch patch could get committed. Nothing exiting, just the last of the WP Core "return type does not match" patches.

#21 @SergeyBiryukov
3 years ago

In 51594:

Code Modernization: Silence the deprecation warning for missing return type in phpunit/tests/compat.php.

This fixes the "Deprecated: Return type of CountableFake::count() should be compatible with Countable::count(): int" warning on PHP 8.1.

PHP native interfaces now have declared return types and methods in classes implementing these interfaces need to either have the return type declared (in a covariant compatible manner with the PHP native interface method declaration), or need to silence the deprecation warning using the #[ReturnTypeWillChange] attribute.

Follow-up to [51517], [51529], [51530], [51531], [51532].

Props jrf.
See #53635.

#22 @jrf
3 years ago

FYI: Based on a quick scan, I've found one PHP 8.1 issue in SimplePie and I have submitted a fix this to the repo. Their test suite is far from complete though (similar to WP's own test suite), so there may be more issues lurking under the surface.

@jrf
3 years ago

PHP 8.1: Tests_Functions_DoEnclose: fix bug in test

#23 @jrf
3 years ago

The patch I've just uploaded - 53635-05-null-to-non-nullable-in-testcode.patch - is a test code only patch and ready for commit.

Fixes two tests which were erroring out.

Commit details:

PHP 8.1: Tests_Functions_DoEnclose: fix bug in test

As per the PHP manual:

If the component parameter is omitted, an associative array is returned.
If the component parameter is specified, parse_url() returns a string (or an int, in the case of PHP_URL_PORT) instead of an array. If the requested component doesn't exist within the given URL, null will be returned.

Ref: https://www.php.net/manual/en/function.parse-url.php#refsect1-function.parse-url-returnvalues

In this case, parse_url() is called with the PHP_URL_PATH as $component, but the returned value is subsequently checked against false.
In other words, this condition would previously always result true and would lead to null, potentially, being passed to the PHP native pathinfo() function which expects a string.

On PHP 8.1, this will result in a test failure with a pathinfo(): Passing null to parameter #1 ($path) of type string is deprecated error.

Ref: https://www.php.net/manual/en/function.pathinfo.php

This ticket was mentioned in PR #1570 on WordPress/wordpress-develop by jrfnl.


3 years ago
#24

  • Keywords has-patch added

No input validation was done. Covered by existing Tests_Functions::test_validate_file() test (well, the null and string part is).

Error fixed: preg_match_all(): Passing null to parameter #2 ($subject) of type string is deprecated

QUESTION: should we add tests for other scalar input types ?

Note: trac ticket link below not added yet on purpose while this ticket is still WIP.

Trac ticket:

This ticket was mentioned in PR #1572 on WordPress/wordpress-develop by jrfnl.


3 years ago
#25

  • Keywords has-unit-tests added

### Tests_Admin_includesFile: add two tests for the download_url() function

The first test is "girl-scouting" to make sure that the code up to the point where the error is expected is tested.

The second test exposed a PHP 8.1 basename(): Passing null to parameter #1 ($path) of type string is deprecated error due to the call to parse_url() returning null when the component requested does not exist in the passed URL.

### PHP 8.1: download_url(): prevent "passing null to non-nullable" notice [1]

Includes removing duplicate function calls.

I've verified that neither the $url, nor the $url_filename are changed between when they are first received/defined and when they are re-used, so there is no need to repeat the function calls.

### Tests_Admin_includesFile: add yet another test for the download_url() function

The output of the call to parse_url() stored in the $url_path variable is used in more places in the function logic.
This test exposes a second PHP 8.1 deprecation notice, this time for substr(): Passing null to parameter #1 ($string) of type string is deprecated.

### PHP 8.1: download_url(): prevent "passing null to non-nullable" notice [2]

Note: trac ticket link below not added yet on purpose while this ticket is still WIP.

Trac ticket:

This ticket was mentioned in Slack in #core by sergey. View the logs.


3 years ago

This ticket was mentioned in PR #1576 on WordPress/wordpress-develop by jrfnl.


3 years ago
#27

### Tests_Cron: add test for wp_schedule_event()

### PHP 8.1: wp_schedule_event(): fix deprecation notice

The following deprecation notice shows at the top of a test run on PHP 8.1-beta2:

Deprecated: Automatic conversion of false to array is deprecated in path/to/src/wp-includes/cron.php on line 305

In wp_schedule_event(), the cron info array is retrieved via a call to _get_cron_array(), but as the documentation for that function (correctly) states, the return type of that function is array|false, where false is returned for a virgin site, where no cron jobs have been scheduled yet.

However, no type check is done on the return value and the wp_schedule_event() function just blindly continues by assigning a value to a subkey of the $crons "array".

Fixed by adding validation for the returned value from _get_cron_array() and initializing an empty array if false was returned.

Ref: https://developer.wordpress.org/reference/functions/_get_cron_array/

Trac ticket: https://core.trac.wordpress.org/ticket/53635

This ticket was mentioned in PR #1577 on WordPress/wordpress-develop by jrfnl.


3 years ago
#28

Fixes WP::parse_request(): Passing null to parameter #1 ($string) of type string is deprecated notices on PHP 8.1.

Impact: 611 of the pre-existing tests are affected by this issue.

Note: trac ticket link below not added yet on purpose while this ticket is still WIP.

Trac ticket:

#29 @hellofromTonya
3 years ago

  • Owner set to hellofromTonya
  • Status changed from new to accepted

Assigning myself as owner for code reviews and pair programming with @jrf.

#30 @SergeyBiryukov
3 years ago

In 51606:

Tests: Use correct comparison in do_enclose() tests.

As per the PHP manual:

If the component parameter is omitted, an associative array is returned.
If the component parameter is specified, parse_url() returns a string (or an int, in the case of PHP_URL_PORT) instead of an array. If the requested component doesn't exist within the given URL, null will be returned.

Reference: PHP Manual: parse_url(): Return Values

In this case, parse_url() is called with the PHP_URL_PATH as $component, but the returned value is subsequently checked against false.

In other words, this condition would previously always result in true and would lead to null potentially being passed to the PHP native pathinfo() function which expects a string.

On PHP 8.1, this would result in a test failure with a pathinfo(): Passing null to parameter #1 ($path) of type string is deprecated error.

Reference: PHP Manual: pathinfo()

Follow-up to [46175].

Props jrf.
See #53635.

This ticket was mentioned in PR #1582 on WordPress/wordpress-develop by jrfnl.


3 years ago
#31

More fixes for null after parse_url(), but in files for which unit tests can't easily be added.

Note: trac ticket link below not added yet on purpose while this ticket is still WIP.

Trac ticket:

This ticket was mentioned in PR #1582 on WordPress/wordpress-develop by jrfnl.


3 years ago
#32

More fixes for null after parse_url(), but in files for which unit tests can't easily be added.

Note: trac ticket link below not added yet on purpose while this ticket is still WIP.

Trac ticket:

jrfnl commented on PR #1558:


3 years ago
#33

As discussed in the live stream session:

  • Rebased on current master
  • Fixed up the double arrow alignment
  • Wrote up the details of the issue in the "fix" commit message.

#34 @SergeyBiryukov
3 years ago

In 51619:

Code Modernization: Check the return type of _get_cron_array() in wp_schedule_event().

This fixes a "Deprecated: Automatic conversion of false to array is deprecated" warning on PHP 8.1.

In wp_schedule_event(), the cron info array is retrieved via a call to _get_cron_array(), but as the documentation (correctly) states, the return type of that function is array|false, where false is returned for a virgin site, with no cron jobs scheduled yet.

However, no type check is done on the return value, and the wp_schedule_event() function just blindly continues by assigning a value to a subkey of the $crons "array".

Fixed by adding validation for the returned value from _get_cron_array() and initializing an empty array if false was returned.

Reference: WordPress Developer Resources: _get_cron_array()

Props jrf, hellofromTonya, lucatume, pbearne, iluy, pedromendonca, SergeyBiryukov.
See #53635.

#35 @SergeyBiryukov
3 years ago

In 51622:

Code Modernization: Check the return type of parse_url() in WP::parse_request().

As per the PHP manual:

If the component parameter is omitted, an associative array is returned.
If the component parameter is specified, parse_url() returns a string (or an int, in the case of PHP_URL_PORT) instead of an array. If the requested component doesn't exist within the given URL, null will be returned.

Reference: PHP Manual: parse_url(): Return Values

In this case, parse_url() is called with the PHP_URL_PATH as $component. This will return null in the majority of cases, as – exсept for subdirectory-based sites – home_url() returns a URL without the trailing slash, like http://example.org.

The return value of parse_url() was subsequently passed to trim(), leading to a trim(): Passing null to parameter #1 ($string) of type string is deprecated notice on PHP 8.1.

Fixed by adjusting the logic flow to:

  • Only pass the return value of parse_url() to follow-on functions if it makes sense, i.e. if it isn't null, nor an empty string.
  • Preventing calls to preg_replace() and trim() further down in the function logic flow, when preg_replace()/trim() would have nothing to do anyhow.

Follow-up to [25617].

Props jrf, hellofromTonya, SergeyBiryukov.
See #53635.

#36 @SergeyBiryukov
3 years ago

In 51624:

Code Modernization: Correct handling of null in wp_parse_str().

This fixes parse_str(): Passing null to parameter #1 ($string) of type string is deprecated notices on PHP 8.1, without change in behaviour.

Impact: 311 of the pre-existing tests are affected by this issue.

The PHP native parse_str() function expects a string, however, based on the failing tests, it is clear there are functions in WordPress which passes a non-string – including null – value to the wp_parse_str() function, which would subsequently pass it onto the PHP native function without further input validation.

Most notable offender is the wp_parse_args() function which special cases arrays and objects, but passes everything else off to wp_parse_str().

Several ways to fix this issue have been explored, including checking the received value with is_string() or is_scalar() before passing it off to the PHP native parse_str() function.

In the end it was decided against these in favor of a string cast as:

  • is_string() would significantly change the behavior for anything non-string.
  • is_scalar() up to a point as well, as it does not take objects with a __toString() method into account.

Executing a string cast on the received value before passing it on maintains the pre-existing behavior while still preventing the deprecation notice coming from PHP 8.1.

Reference: PHP Manual: parse_str()

Follow-up to [5709].

Props jrf, hellofromTonya, lucatume, SergeyBiryukov.
See #53635.

jrfnl commented on PR #1576:


3 years ago
#37

Closing as committed via changeset 51619.

jrfnl commented on PR #1576:


3 years ago
#38

Closing as committed via changeset 51619.

jrfnl commented on PR #1577:


3 years ago
#39

Closing as committed via changeset 51622.

jrfnl commented on PR #1558:


3 years ago
#40

Closing as committed via changeset 51624.

#41 @SergeyBiryukov
3 years ago

In 51625:

Code Modernization: Check the input type in validate_file().

This fixes a preg_match_all(): Passing null to parameter #2 ($subject) of type string is deprecated notice on PHP 8.1.

The behavior for null and string input is covered by the existing Tests_Functions::test_validate_file() test.

Effect: Errors down by 238, assertions up by 1920, failures down by 1.

Props jrf, hellofromTonya, SergeyBiryukov.
See #53635.

#42 @SergeyBiryukov
3 years ago

In 51626:

Code Modernization: Check the return type of parse_url() in download_url().

As per the PHP manual:

If the component parameter is omitted, an associative array is returned.
If the component parameter is specified, parse_url() returns a string (or an int, in the case of PHP_URL_PORT) instead of an array. If the requested component doesn't exist within the given URL, null will be returned.

Reference: PHP Manual: parse_url(): Return Values

This commit adds three unit tests for download_url():

  • The first test is "girl-scouting" to make sure that the code up to the point where the error is expected is tested.
  • The second test exposed a PHP 8.1 basename(): Passing null to parameter #1 ($path) of type string is deprecated error due to the call to parse_url() returning null when the component requested does not exist in the passed URL.
  • The output of the call to parse_url() stored in the $url_path variable is used in more places in the function logic. The third test exposes a second PHP 8.1 deprecation notice, this time for substr(): Passing null to parameter #1 ($string) of type string is deprecated.

This commit also removes duplicate parse_url() calls. Neither $url nor $url_filename are changed between when they are first received/defined and when they are re-used, so there is no need to repeat the function calls.

Follow-up to [51606], [51622].

Props jrf, hellofromTonya, SergeyBiryukov.
See #53635.

#43 @SergeyBiryukov
3 years ago

In 51627:

Tests: Use a better return type check for parse_url() in do_enclose() tests.

Since the pathinfo() function accepts a string, checking for that specifically is more consistent with similar checks elsewhere in core.

Follow-up to [51606], [51622], [51626].

See #53635.

jrfnl commented on PR #1572:


3 years ago
#44

Closing as committed via changeset 51626.

jrfnl commented on PR #1570:


3 years ago
#45

Closing as committed via changeset 51625.

#46 @hellofromTonya
3 years ago

  • Keywords commit added

@SergeyBiryukov PR 1582 is ready for commit.

#47 @SergeyBiryukov
3 years ago

In 51629:

Code Modernization: Check the return type of parse_url() on Plugin/Theme Editor screens.

As per the PHP manual:

If the component parameter is omitted, an associative array is returned.
If the component parameter is specified, parse_url() returns a string (or an int, in the case of PHP_URL_PORT) instead of an array. If the requested component doesn't exist within the given URL, null will be returned.

Reference: PHP Manual: parse_url(): Return Values

While it is probably unlikely that someone would have a direct link to the plugin/theme editor on their home page or even on someone else's homepage, it is entirely possible for the referrer URL to not have a "path" component.

In PHP 8.1, this would lead to a basename(): Passing null to parameter #1 ($string) of type string is deprecated notice.

Changing the logic around and adding validation for the return type value of parse_url() prevents that.

Follow-up to [51606], [51622], [51626].

Props jrf, hellofromTonya, SergeyBiryukov.
See #53635.

#48 @SergeyBiryukov
3 years ago

In 51630:

Code Modernization: Check the return type of parse_url() in ms_cookie_constants().

As per the PHP manual:

If the component parameter is omitted, an associative array is returned.
If the component parameter is specified, parse_url() returns a string (or an int, in the case of PHP_URL_PORT) instead of an array. If the requested component doesn't exist within the given URL, null will be returned.

Reference: PHP Manual: parse_url(): Return Values

It is entirely possible for the siteurl option to not have a "path" component.

In PHP 8.1, this would lead to a trim(): Passing null to parameter #1 ($string) of type string is deprecated notice.

Changing the logic around and adding validation for the return type value of parse_url() prevents that.

As this function is declaring global constants, adding tests for this change is not really an option without potentially affecting other tests.

Follow-up to [51606], [51622], [51626], [51629].

Props jrf, hellofromTonya, SergeyBiryukov.
See #53635.

jrfnl commented on PR #1582:


3 years ago
#49

Closing as committed via 51629 and 51630

P.S.: @SergeyBiryukov Why did you removed the extra parentheses in the condition in ms_cookie_constants() ? They were there to prevent issues with operator precedence.

SergeyBiryukov commented on PR #1582:


3 years ago
#50

Why did you removed the extra parentheses in the condition in ms_cookie_constants() ? They were there to prevent issues with operator precedence.

I think they are redundant here, as && always takes precedence over ||, unless I'm missing something.

In my understanding, they are necessary in cases like this:
if ( a && ( b || c ) )
but don't make a difference in cases like this:
if ( a || ( b && c ) )

If that is incorrect, happy to add them back :)

jrfnl commented on PR #1582:


3 years ago
#51

@SergeyBiryukov That is correct, but most people do not know the ins and outs of operator precedence in as much detail, so the parentheses clarify that for the next person reading the code. I'd say: leave it for now, but know that I did put them in for a reason.

SergeyBiryukov commented on PR #1582:


3 years ago
#52

Ah, that makes perfect sense. I appreciate the explanation and will keep it in mind from now on.

This ticket was mentioned in PR #1599 on WordPress/wordpress-develop by jrfnl.


3 years ago
#53

### Tests: add dedicated test for _set_cron_array()

Add a full set of tests for the _set_cron_array() function.

### PHP 8.1: add input validation to the _set_cron_array() function

The _set_cron_array() function expects a cron array as the first parameter, but will often be passed the - potentially updated - output of a call to _get_cron_array().

The _get_cron_array() function may, however, return false, in which case a "Deprecated: Automatic conversion of false to array is deprecated" warning will be thrown on PHP 8.1.

Fixed now by adding input validation to the _set_cron_array() function.

Ref: https://developer.wordpress.org/reference/functions/_get_cron_array/

Trac ticket: https://core.trac.wordpress.org/ticket/53635

/cc @pbearne I did see your test for _set_cron_array() in #1595, but I felt this function needed a more extensive set of tests, especially as it is prone to be hit by the PHP 8.1 deprecation notice. Please review when you have a chance.

This ticket was mentioned in PR #1599 on WordPress/wordpress-develop by jrfnl.


3 years ago
#54

### Tests: add dedicated test for _set_cron_array()

Add a full set of tests for the _set_cron_array() function.

### PHP 8.1: add input validation to the _set_cron_array() function

The _set_cron_array() function expects a cron array as the first parameter, but will often be passed the - potentially updated - output of a call to _get_cron_array().

The _get_cron_array() function may, however, return false, in which case a "Deprecated: Automatic conversion of false to array is deprecated" warning will be thrown on PHP 8.1.

Fixed now by adding input validation to the _set_cron_array() function.

Ref: https://developer.wordpress.org/reference/functions/_get_cron_array/

Trac ticket: https://core.trac.wordpress.org/ticket/53635

/cc @pbearne I did see your test for _set_cron_array() in #1595, but I felt this function needed a more extensive set of tests, especially as it is prone to be hit by the PHP 8.1 deprecation notice. Please review when you have a chance.

This ticket was mentioned in Slack in #hosting-community by crixu. View the logs.


3 years ago

#56 @SergeyBiryukov
3 years ago

In 51633:

Code Modernization: Only set auto_detect_line_endings in PHP < 8.1.

Since PHP 8.1, the auto_detect_line_endings setting is deprecated:

The auto_detect_line_endings ini setting modifies the behavior of file() and fgets() to support an isolated \r (as opposed to \n or \r\n) as a newline character. These newlines were used by “Classic” Mac OS, a system which has been discontinued in 2001, nearly two decades ago. Interoperability with such systems is no longer relevant.

Reference: PHP RFC: Deprecations for PHP 8.1: auto_detect_line_endings ini setting

The auto_detect_line_endings ini setting has been deprecated. If necessary, handle \r line breaks manually instead.

Reference: PHP 8.1 Upgrade Notes.

This commit fixes the warning when running tests for the PO class:

Deprecated: auto_detect_line_endings is deprecated in /var/www/src/wp-includes/pomo/po.php on line 16

Follow-up to [10584], [51628].

See #53635.

#57 @jrf
3 years ago

FYI: PHPMailer has just released a new version with preliminary PHP 8.1 support. Ticket #53953 has been opened to handle the upgrade.

#58 follow-up: @jrf
3 years ago

@SergeyBiryukov I'd advocate for a different fix for [51633]: silencing the deprecation notice (for now).

The actual auto_detect_line_endings functionality has not been removed from PHP (yet) and will still work until PHP 9.0 and while the old MacOS line ending of \r on its own, is nowadays a rarity to encounter, it may still exist in translation files which haven't been updated for a long time.

Also see this similar discussion elsewhere about this.

#59 in reply to: ↑ 58 ; follow-up: @SergeyBiryukov
3 years ago

Replying to jrf:

The actual auto_detect_line_endings functionality has not been removed from PHP (yet) and will still work until PHP 9.0 and while the old MacOS line ending of \r on its own, is nowadays a rarity to encounter, it may still exist in translation files which haven't been updated for a long time.

Thanks for these details! If the functionality still exists, I agree with silencing the notice for now :)

jrfnl commented on PR #1599:


3 years ago
#60

Oh, before I forget: as this is a "private" function (prefixed with a _ and annotated as such in the docblock). it _could_ be considered to add an array type declaration instead, which has been supported since PHP 5.0.

  • For the function signature, this would not be a BC-break as this function cannot be overloaded.
  • For the function functionality, it would be as passing it something which is not an array will now result in a (catchable) fatal error/TypeError and any such instances in Core and plugins/themes would have to be fixed.

With that second reason in mind, I'm proposing the current solution, but I wanted to mention that the type declaration option had been considered.

#61 @hellofromTonya
3 years ago

PR 1599 is ready for commit.

Update:
There's one tiny nitpick of a full-stop at the end of an error message (noted in the PR) that the committer could fix. Resolved.

/cc @SergeyBiryukov

Last edited 3 years ago by hellofromTonya (previous) (diff)

This ticket was mentioned in Slack in #hosting-community by jadonn. View the logs.


3 years ago

jrfnl commented on PR #1556:


3 years ago
#63

Rebased without changes to get it passed an imaginary merge conflict.

@jrf
3 years ago

Pomo/ PHP 8.1: improve the fix for auto_detect_line_endings ... as per the inline comment.

#64 in reply to: ↑ 59 @jrf
3 years ago

Replying to SergeyBiryukov:

Thanks for these details! If the functionality still exists, I agree with silencing the notice for now :)

@SergeyBiryukov Patch 53635-06-improve-auto_detect_line_endings-fix.patch uploaded to do just that (with an extensive inline comment explaining it).

This ticket was mentioned in Slack in #core by sergey. View the logs.


3 years ago

#66 @SergeyBiryukov
3 years ago

In 51636:

Code Modernization: Silence the deprecation warning for auto_detect_line_endings.

Since PHP 8.1, the auto_detect_line_endings setting is deprecated:

The auto_detect_line_endings ini setting modifies the behavior of file() and fgets() to support an isolated \r (as opposed to \n or \r\n) as a newline character. These newlines were used by “Classic” Mac OS, a system which has been discontinued in 2001, nearly two decades ago. Interoperability with such systems is no longer relevant.

Reference: PHP RFC: Deprecations for PHP 8.1: auto_detect_line_endings ini setting

The auto_detect_line_endings ini setting has been deprecated. If necessary, handle \r line breaks manually instead.

Reference: PHP 8.1 Upgrade Notes.

This commit fixes the warning when running tests for the PO class:

Deprecated: auto_detect_line_endings is deprecated in /var/www/src/wp-includes/pomo/po.php on line 16

While deprecated, the actual auto_detect_line_endings functionality has not been removed from PHP (yet) and will still work until PHP 9.0.

For now, we're silencing the deprecation notice as there may still be translation files around which haven't been updated in a long time and which still use the old MacOS standalone \r as a line ending.

This should be revisited when PHP 9.0 is in alpha/beta.

Follow-up to [51633].

Props jrf.
See #53635.

@jrf
3 years ago

PHP 8.1: WP_Sitemaps_Provider::get_sitemap_url(): fix deprecation notice

#67 @jrf
3 years ago

I've just uploaded another patch - 53635-07-incorrect-default-value-for-http_build_query.patch - which is ready for commit.

Full patch description:

PHP 8.1: WP_Sitemaps_Provider::get_sitemap_url(): fix deprecation notice

The WP_Sitemaps_Provider::get_sitemap_url() method calls the PHP native http_build_query() function, the second parameter of which is the _optional_ $numeric_prefix parameter which expects a string.

A parameter being optional, however, does not automatically make it nullable.

As of PHP 8.1, passing null to a non-nullable PHP native function will generate a deprecation notice.
In this case, this function call yielded a http_build_query(): Passing null to parameter #2 ($numeric_prefix) of type string is deprecated notice.

Changing the null to an empty string fixes this without BC-break.

This change is already covered by tests as 14 of the existing tests (targetting the sitemap functions) failed on these function calls when running the tests on PHP 8.1.

Refs:

#68 @jrf
3 years ago

FYI: I've pulled fixes for all the known PHP 8.1 issues in SimplePie GetID3 (based on the WP test suite).
https://github.com/JamesHeinrich/getID3/pulls/jrfnl

I suspect there will be (plenty) more issues to fix in SimplePie GetID3, but as there are no tests for the package itself and the tests in WP are limited, these will be hard to pinpoint.

Last edited 3 years ago by jrf (previous) (diff)

#69 @SergeyBiryukov
3 years ago

In 51652:

Code Modernization: Pass correct default value to http_build_query() in WP_Sitemaps_Provider::get_sitemap_url().

The WP_Sitemaps_Provider::get_sitemap_url() method calls the PHP native http_build_query() function, the second parameter of which is the optional $numeric_prefix parameter which expects a string.

A parameter being optional, however, does not automatically make it nullable.

As of PHP 8.1, passing null to a non-nullable PHP native function will generate a deprecation notice.

In this case, this function call yielded a http_build_query(): Passing null to parameter #2 ($numeric_prefix) of type string is deprecated notice.

Changing the null to an empty string fixes this without a backward compatibility break.

This change is already covered by tests as 14 of the existing tests failed on these function calls when running the tests on PHP 8.1.

References:

Follow-up to [48470].

Props jrf.
See #53635.

This ticket was mentioned in Slack in #core by sergey. View the logs.


3 years ago

This ticket was mentioned in PR #1625 on WordPress/wordpress-develop by jrfnl.


3 years ago
#71

The Requests_Cookie class expects valid - non-null - attributes to be passed, either as an array or as a Requests_Utility_CaseInsensitiveDictionary object.

However, the WP_Http_Cookie::get_attributes() explicitly sets the expires, path and domain index keys in an array with values which _may_ be null. This will cause strtotime(): Passing null to parameter #1 ($datetime) of type string is deprecated-like errors when the attributes are passed to the Requests_Cookie class.

Note: a null value for path would generate a similar deprecation notice, but for the preg_match() function.

Fixed by using array_filter() on the attributes to explicitly filter out null values before passing the attributes to Requests_Cookie.

Note: I'm chosing to explicitly only filter null values. Using array_filter() without a callback would filter out all "empty" values, but that may also remove values which are explicitly set to false or 0, which may be valid values.

Fixes two errors in the external-http group in the WordPress Core test suite:

1) Tests_HTTP_Functions::test_get_response_cookies_with_wp_http_cookie_object
strtotime(): Passing null to parameter #1 ($datetime) of type string is deprecated

/var/www/src/wp-includes/Requests/Cookie.php:268
/var/www/src/wp-includes/Requests/Cookie.php:237
/var/www/src/wp-includes/Requests/Cookie.php:90
/var/www/src/wp-includes/class-http.php:460
/var/www/src/wp-includes/class-http.php:349
/var/www/src/wp-includes/class-http.php:624
/var/www/src/wp-includes/http.php:162
/var/www/tests/phpunit/tests/http/functions.php:156

2) Tests_HTTP_Functions::test_get_cookie_host_only
strtotime(): Passing null to parameter #1 ($datetime) of type string is deprecated

/var/www/src/wp-includes/Requests/Cookie.php:268
/var/www/src/wp-includes/Requests/Cookie.php:237
/var/www/src/wp-includes/Requests/Cookie.php:90
/var/www/src/wp-includes/class-http.php:460
/var/www/tests/phpunit/tests/http/functions.php:235

Trac ticket: https://core.trac.wordpress.org/ticket/53635

This ticket was mentioned in PR #1628 on WordPress/wordpress-develop by jrfnl.


3 years ago
#72

### Tests: add a dedicated test file for the wpdb::_real_escape() method

### PHP 8.1 | wpdb::_real_escape(): fix "passing null to non-nullable" deprecation notices

The PHP native mysqli_real_escape_string() function expects to be passed a string as the second parameter and this is not a nullable parameter.
Passing null to it will result in a mysqli_real_escape_string(): Passing null to parameter #2 ($string) of type string is deprecated notice on PHP 8.1.

Previously, an input type check was put in place to prevent fatal errors on PHP 8.0 when an array, object or resource was passed. Changeset [48980].

A null value was explicitly excluded from that check, even though a null value being passed would only ever result in an empty string anyway.

This commit changes the previous input type check to also bow out early for null values and to automatically return an empty string for those.

Refs:

Trac ticket: https://core.trac.wordpress.org/ticket/53635

@jrf
3 years ago

PHP 8.1 | Tests: fix bug in Tests_Admin_IncludesPlugin::test_get_plugin_files_folder() The Tests_Admin_IncludesPlugin::_create_plugin() expects the first parameter to be a text string to be written to a plugin file using fwrite(). Passing null causes a fwrite(): Passing null to parameter #2 ($data) of type string is deprecated notice. Ref: https://www.php.net/manual/en/function.fwrite

#73 @jrf
3 years ago

Patch 53635-08-null-to-non-nullable-in-testcode.patch​ which I just uploaded is ready for commit. Simple test only fix.

GitHub PRs 1625 and 1628 which I opened earlier, are both ready for review.

This ticket was mentioned in PR #1631 on WordPress/wordpress-develop by jrfnl.


3 years ago
#74

👉🏻 This PR will be easiest to review by examining the individual commits in the order they are lined up in.

This commit splits the tests in the tests/phpunit/tests/compat.php file up into individual test classes for each function being tested.

The tests have been reviewed for typical test issues and those issues have been fixed in each test class.
Fixes are along the lines of:

  • Adding @covers tags.
  • Adding visibility modifiers to all methods.
  • Adding function availability test.
  • Where relevant, fixing the assertion parameter order.
  • Where relevant, reworking a test to use a data provider.
  • Where relevant, renaming data provider methods to have a more obvious link to the test it applies to.
  • Making data providers more readable by adding keys within the data sets and often also updating them to named data sets.
  • Making the actual test code more readable by using descriptive variables and multi-line function calls.
  • Adding the $message parameter to all assertions when a test method contains more than one assertion.
  • Adding/removing data sets in data providers.
  • Renaming fixture classes to more unique names.

One of the tests was causing errors in PHP 8.1. A fix for this is included in a separate commit.

### PHP 8.1 | Compat/_mb_substr(): fix passing null to non-nullable deprecation

On PHP 8.1, the _mb_substr() function would throw a preg_match_all(): Passing null to parameter #2 ($subject) of type string is deprecated notice when passed null.

To maintain the same behaviour as before, let's just bow out early when $str is passed as null as the outcome will, in that case, only ever be an empty string.

Note: this does mean that the _mb_substr() function now has a subtle difference in behaviour compared to the PHP native mb_substr() function as the latter *will* throw the deprecation notice.

The existing tests for the compat function already cover this issue.

Trac ticket: https://core.trac.wordpress.org/ticket/53635
Trac ticket: https://core.trac.wordpress.org/ticket/53363

This ticket was mentioned in PR #1632 on WordPress/wordpress-develop by jrfnl.


3 years ago
#75

### Tests_Functions_Anonymization: add a few more invalid IP test cases

... and add extra @covers tag for the two functions which are testing the wp_privacy_anonymize_ip() function.
(At class level, the wp_privacy_anonymize_data() is set as covered).

### PHP 8.1 | wp_privacy_anonymize_ip(): fix null to non-nullable deprecation

The wp_privacy_anonymize_ip() function expects a string for the $ip_addr parameter, but did not do any input validation.

One of the pre-existing test cases, passed null to the function, leading to a substr_count(): Passing null to parameter #1 ($haystack) of type string is deprecated notice on PHP 8.1.

Fixed now by doing a cursory check on the variable at the start of the function and bowing out early for a number of cases (null, false, 0, '') which would all result in the same 0.0.0.0 output anyway.

Trac ticket: https://core.trac.wordpress.org/ticket/53635

This ticket was mentioned in PR #1633 on WordPress/wordpress-develop by jrfnl.


3 years ago
#76

The WP_Comment_Query::get_comment_ids() method is supposed to handle null as a search query, but was throwing a strlen(): Passing null to parameter #1 ($string) of type string is deprecated notice on PHP 8.1.

Discovered via and already covered via the pre-existing Tests_Comment_Query::test_search_null_should_be_ignored() test method.

Trac ticket: https://core.trac.wordpress.org/ticket/53635

This ticket was mentioned in PR #1633 on WordPress/wordpress-develop by jrfnl.


3 years ago
#77

The WP_Comment_Query::get_comment_ids() method is supposed to handle null as a search query, but was throwing a strlen(): Passing null to parameter #1 ($string) of type string is deprecated notice on PHP 8.1.

Discovered via and already covered via the pre-existing Tests_Comment_Query::test_search_null_should_be_ignored() test method.

Trac ticket: https://core.trac.wordpress.org/ticket/53635

This ticket was mentioned in PR #1635 on WordPress/wordpress-develop by jrfnl.


3 years ago
#78

The img_caption_shortcode() method expects a string for the $content parameter and is expected to return a string as well.

However, the default value for the $content parameter was set to null and previously, if the $content parameter was not passed, the function would return null.

This to me seems more like a bug in the function declaration signature, than anything else, so I'm suggesting to update the default parameter value for $content to an empty string.

This _does_ result in a minor/inconsequential BC break, in that the function will now return an empty string instead of null for that situation, but that seems _more correct_ than before and complies with the specification of the function in the docblock.

Tests which specifically tested for null being returned have been updated to expect an empty string.

Fixes a few notices in the existing test suite:

strpos(): Passing null to parameter #1 ($haystack) of type string is deprecated

Trac ticket: https://core.trac.wordpress.org/ticket/53635

This ticket was mentioned in PR #1636 on WordPress/wordpress-develop by jrfnl.


3 years ago
#79

The term_exists() function expects a string or an integer for the $term parameter, but did not do any input validation.

One of the pre-existing test cases, passed null to the function, leading to a trim(): Passing null to parameter #1 ($string) of type string is deprecated notice on PHP 8.1.

Fixed now by doing a cursory check on the variable at the start of the function and bowing out early in case the $term is null.

Side-note: this function should in all reality be refactored/rewritten to two or three different functions, as the number of different return types for this function introduces a high risk of failure into any code using this function.

The issue was discovered via and is already covered by the Tests_TermExists::test_term_exists_unknown() test method.

Trac ticket: https://core.trac.wordpress.org/ticket/53635

This ticket was mentioned in PR #1637 on WordPress/wordpress-develop by jrfnl.


3 years ago
#80

The get_option() function will return false for a widget which was not previously registered.

In PHP 8.1, this will lead to an Automatic conversion of false to array is deprecated notice.

This fixes the test callback to prevent this notice.

Trac ticket: https://core.trac.wordpress.org/ticket/53635

This ticket was mentioned in PR #1638 on WordPress/wordpress-develop by jrfnl.


3 years ago
#81

Okay, so this one takes a little explaining.

Basically, the whole assertSameIgnoreEOL() assertion was fundamentally flawed. The assertion contends that it checks that the expected and actual values are of the same type and value, but the reality was very different.

  • The function uses map_deep() to potentially handle all sorts of inputs.
  • map_deep() handles arrays and objects with special casing, but will call the callback on everything else without further distinction.
  • The callback used passes the expected/actual value on to the str_replace() function to remove potential new line differences.
  • And the str_replace() function will - with a non-array input for the $subject - always return a string.
  • The output of these calls to map_deep() will therefore have "normalized" _all properties_ in objects, _all values_ in arrays and _all non-object, non-array values_ to strings.
  • And a call to assertSame() will therefore NEVER do a proper type check as the type of all input has already, unintentionally, been "normalized" to string.

Aside from this clear flaw in the design of the assertion, PHP 8.1 now exposes a further issue as a null value for an object property, an array value or a plain value, will now yield a str_replace(): Passing null to parameter #3 ($subject) of type array|string is deprecated notice.

To fix both these issues, the fix in this PR ensures that the call to str_replace() will now only be made if the input is a text string.
All other values passed to the callback are left in their original type.

This ensures that a proper value AND type comparison can be done as well as prevents the PHP 8.1 deprecation notices.

Includes adjusting the method documentation for both this method and the assertEqualsIgnoreEOL() alias method to document that the $expected and $actual parameters can be of any type.

Ref:

👉🏻 Note: I'm fully aware that this change _may_ cause existing tests using these assertions to fail. (all the more reason to run this PR through GH) If any tests _would_ fail those tests should be investigated and fixed. Those failures should not be a reason for not accepting this patch.

Trac ticket: https://core.trac.wordpress.org/ticket/53635
Trac ticket: https://core.trac.wordpress.org/ticket/53363

This ticket was mentioned in PR #1640 on WordPress/wordpress-develop by jrfnl.


3 years ago
#82

### Tests_Option_Option::test_bad_option_names(): rework to data provider

  • Rework the test method to use a data provider with named data sets instead of a foreach().
  • Add method visibility modifiers.
  • Add the $message parameter for each assertion.

### PHP 8.1: (get|add|update|delete)_option: fix null to non-nullable deprecations

In all four of the get_option(), add_option(), update_option() and delete_option() functions, the $option parameter is passed to the PHP native trim() function without prior validation.

In PHP 8.1, this could lead to a trim(): Passing null to parameter #1 ($string) of type string is deprecated for each of these methods.

trim() expects a text string and is only useful when _passed_ a text string as no other variable type can contain whitespace.

Fixed now by verifying that the $option is a string before processing it with trim().

This issue is already covered by the existing Tests_Option_Option::test_bad_option_names() test.

Trac ticket: https://core.trac.wordpress.org/ticket/53635

This ticket was mentioned in PR #1641 on WordPress/wordpress-develop by jrfnl.


3 years ago
#83

In the WP_Meta_Query::get_sql_for_clause(), the 'value' index from a meta query array is passed to the PHP native trim() function without prior validation.

In PHP 8.1, this could lead to a trim(): Passing null to parameter #1 ($string) of type string is deprecated notice.

trim() expects a text string and is only useful when _passed_ a text string as no other variable type can contain whitespace.

Fixed now by verifying that the _value_ is a string before processing it with trim().

This issue is already covered by the existing Tests_Meta_Query::test_null_value_sql() and the Tests_Meta_Query::test_convert_null_value_to_empty_string(0 tests.

Trac ticket: https://core.trac.wordpress.org/ticket/53635

#84 @hellofromTonya
3 years ago

In 51695:

Code Modernization: Add input validation to _set_cron_array().

The private _set_cron_array() function expects a cron array as the first parameter, but will often be passed the - potentially updated - output of a call to _get_cron_array().

When the _get_cron_array() function returns false, a "Deprecated: Automatic conversion of false to array is deprecated" warning will be thrown on PHP 8.1.

The input validation resolves the deprecation warning by setting the cron value to an empty array when not given an array data type.

Adds a full set of _set_cron_array() tests.

Follow-up to [4189], [50152].

Props jrf, hellofromTonya, peterwilsoncc.
See #53635.

This ticket was mentioned in Slack in #core-php by hellofromtonya. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


3 years ago

This ticket was mentioned in PR #1657 on WordPress/wordpress-develop by jrfnl.


3 years ago
#88

... which could be passing null onto a context which doesn't allow for null due to the output of wp_parse_url() with the $component parameter potentially being null.

This patch was discussed in detail during the Sept 3rd livestream.

Trac ticket: https://core.trac.wordpress.org/ticket/53635

This ticket was mentioned in PR #1658 on WordPress/wordpress-develop by jrfnl.


3 years ago
#89

Potential fix for the below issue, but the fix _may_ have unforeseen side-effects. Needs second opinion from someone with detailed domain knowledge of the XMLRPC functionality.

Tests_XMLRPC_mw_newPost::test_post_thumbnail
strpos(): Passing null to parameter #1 ($haystack) of type string is deprecated

path/to/src/wp-includes/class-wp-xmlrpc-server.php:5681
path/to/src/wp-includes/src/wp-includes/class-wp-xmlrpc-server.php:5606
path/to/tests/phpunit/tests/xmlrpc/mw/newPost.php:147

Trac ticket: https://core.trac.wordpress.org/ticket/53635

This ticket was mentioned in Slack in #core by jrf. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by sergey. View the logs.


3 years ago

#92 @hellofromTonya
3 years ago

In 51793:

Code Modernization: Fix null to non-nullable deprecation in wp_privacy_anonymize_ip().

The wp_privacy_anonymize_ip() function expects a string for the $ip_addr parameter, but did not do any input validation.

One of the pre-existing test cases, passed null to the function, leading to a substr_count(): Passing null to parameter #1 ($haystack) of type string is deprecated notice on PHP 8.1.

Fixed now by doing a cursory check on the variable at the start of the function and bowing out early for a number of cases (null, false, 0, '') which would all result in the same 0.0.0.0 output anyway.

Follow-up [42971].

Props jrf, hellofromTonya.
See #53635.

#94 @hellofromTonya
3 years ago

In 51796:

Code Modernization: Fix null to non-nullable deprecation in term_exists().

The term_exists() function expects a string or an integer for the $term parameter. It validates for integer, but not for string or null.

One of the pre-existing test cases, passed null to the function, leading to a trim(): Passing null to parameter #1 ($string) of type string is deprecated notice on PHP 8.1.

Fixed now by doing a cursory check on the variable at the start of the function and bowing out early in case the $term is null.

The issue was discovered via and is already covered by the Tests_TermExists::test_term_exists_unknown() test method.

Follow-up to [15220]. [38716].

Props jrf, hellofromTonya.
See #53635.

#96 @hellofromTonya
3 years ago

In 51797:

Code Modernization: Fix null to non-nullable deprecations in WP_Meta_Query::get_sql_for_clause().

In the WP_Meta_Query::get_sql_for_clause(), the 'value' index from a meta query array is passed to the PHP native trim() function without prior validation.

In PHP 8.1, this could lead to a trim(): Passing null to parameter #1 ($string) of type string is deprecated notice.

trim() expects a text string and is only useful when passed a text string as no other variable type can contain whitespace.

Fixed now by verifying that the value is a string before processing it with trim().

This issue is already covered by the existing Tests_Meta_Query::test_null_value_sql() and the Tests_Meta_Query::test_convert_null_value_to_empty_string() tests.

Follow-up to [17699], [29887], [29940].

Props jrf, hellofromTonya.
See #53635.

#98 @hellofromTonya
3 years ago

In 51799:

Code Modernization: Fix "passing null to non-nullable" deprecation in wpdb::_real_escape().

The PHP native mysqli_real_escape_string() function expects to be passed a string as the second parameter and this is not a nullable parameter.

Passing null to it will result in a mysqli_real_escape_string(): Passing null to parameter #2 ($string) of type string is deprecated notice on PHP 8.1.

Previously, an input type check was put in place to prevent fatal errors on PHP 8.0 when an array, object or resource was passed. Changeset [48980].

A null value was explicitly excluded from that check, even though a null value being passed would only ever result in an empty string anyway.

This commit changes the previous input type check to also bow out early for null values and to automatically return an empty string for those.

Refs:

Follow-up to [48980].

Props jrf, hellofromTonya.
See #53635.

#100 @hellofromTonya
3 years ago

In 51800:

Tests: Fix "null to non-nullable" deprecation notice in Tests_Admin_IncludesPlugin::test_get_plugin_files_folder().

The Tests_Admin_IncludesPlugin::_create_plugin() expects the first parameter to be a text string to be written to a plugin file using fwrite().

Passing null causes a fwrite(): Passing null to parameter #2 ($data) of type string is deprecated notice.

Ref: https://www.php.net/manual/en/function.fwrite

Follow-up to [31002]. [41806].

Props jrf, hellofromTonya.
See #53635.

#102 @hellofromTonya
3 years ago

In 51801:

Code Modernization: Fix "passing null to non-nullable" deprecation notices in WP_Http::normalize_cookies().

The Requests_Cookie class expects valid - non-null - attributes to be passed, either as an array or as a Requests_Utility_CaseInsensitiveDictionary object.

However, the WP_Http_Cookie::get_attributes() explicitly sets the expires, path and domain index keys in an array with values which _may_ be null. This will cause strtotime(): Passing null to parameter #1 ($datetime) of type string is deprecated-like errors when the attributes are passed to the Requests_Cookie class.

Note: a null value for path would generate a similar deprecation notice, but for the preg_match() function.

Fixed by using array_filter() on the attributes to explicitly filter out null values before passing the attributes to Requests_Cookie.

Note: I'm choosing to explicitly only filter null values. Using array_filter() without a callback would filter out all "empty" values, but that may also remove values which are explicitly set to false or 0, which may be valid values.

Fixes two errors in the external-http group in the WordPress Core test suite:

1) Tests_HTTP_Functions::test_get_response_cookies_with_wp_http_cookie_object
strtotime(): Passing null to parameter #1 ($datetime) of type string is deprecated

/var/www/src/wp-includes/Requests/Cookie.php:268
/var/www/src/wp-includes/Requests/Cookie.php:237
/var/www/src/wp-includes/Requests/Cookie.php:90
/var/www/src/wp-includes/class-http.php:460
/var/www/src/wp-includes/class-http.php:349
/var/www/src/wp-includes/class-http.php:624
/var/www/src/wp-includes/http.php:162
/var/www/tests/phpunit/tests/http/functions.php:156

2) Tests_HTTP_Functions::test_get_cookie_host_only
strtotime(): Passing null to parameter #1 ($datetime) of type string is deprecated

/var/www/src/wp-includes/Requests/Cookie.php:268
/var/www/src/wp-includes/Requests/Cookie.php:237
/var/www/src/wp-includes/Requests/Cookie.php:90
/var/www/src/wp-includes/class-http.php:460
/var/www/tests/phpunit/tests/http/functions.php:235

Follow-up to [38164], [45135], [51657].

Props jrf, hellofromTonya.
See #53635.

#105 @hellofromTonya
3 years ago

In 51806:

Code Modernization: Fix "passing null to non-nullable" deprecation notice in WP_Comment_Query::get_comment_ids().

The WP_Comment_Query::get_comment_ids() method is supposed to handle null as a search query, but was throwing a strlen(): Passing null to parameter #1 ($string) of type string is deprecated notice on PHP 8.1.

Discovered via and already covered via the pre-existing Tests_Comment_Query::test_search_null_should_be_ignored() test method.

Follow-up to [36345], [48275].

Props jrf, hellofromTonya.
See #53635.

hellofromtonya commented on PR #1635:


3 years ago
#107

Asked for review by Core Media Maintainers.

Feedback from Joe Dolson

I took at look at the top dozen or so results for plugins that use img_caption_shortcode, and I didn't find any examples that would throw an error with this change. The only reference to null at all used loose comparisons, so it would still be fine. (And that didn't compare against $content, anyway.) Overall, it looks pretty safe.

hellofromtonya commented on PR #1635:


3 years ago
#108

Feedback from @azaozz on slack #core-media channel:

So thinking that it's for the better to change the return type from null to empty string when there's an error with the shortcode. Might impact something but it should be very minimal. $content is expected to always be a string anyway

#109 @hellofromTonya
3 years ago

In 51816:

Media: Fix $content parameter default value in img_caption_shortcode().

The shortcode content is expected to be a string, not null. do_shortcode() expects a string for $content.

The img_caption_shortcode() also expects a string for the $content parameter and is expected to return a string for the HTML content to display the caption.

Prior to this commit:
The default value for the $content parameter was set to null. If no $content was passed, the function:

  • could return null when the $atts['width'] < 1 or there was no caption
  • else, it invoked do_shortcode( $content ) passing null which on PHP 8.1+ triggers a deprecation notice:
    strpos(): Passing null to parameter #1 ($haystack) of type string is deprecated
    

This commit:

  • Fixes the default $content value to align to the expected shortcode content of string, not null.
  • Fixes the PHP 8.1 deprecation notice when null was being passed to do_shortcode().
  • Changes the assertion in a couple of tests to check for the empty string instead of `null.

Follow-up to [8196], [8925], [8239], [26915], [31530], [42704].

Props jrf, hellofromTonya, azaozz, joedolson.
See #53635.

This ticket was mentioned in Slack in #core by sergey. View the logs.


3 years ago

#112 @hellofromTonya
3 years ago

In 51817:

Build/Test Tools: Reworks Tests_Option_Option::test_bad_option_names() into data provider.

The existing tests were running multiple functions through a foreach(). If any test failed, it would bail out and not test against the other scenarios.

This commit:

  • Moves the scenarios to a data provider with named data sets, i.e. to ensure all scenarios are run and tested regardless if any fail.
  • Splits each function under test into individual test methods.
  • Adds a float scenario.
  • Adds method visibility modifiers.

Follow-up to [25002].

Props jrf, hellofromTonya, pbearne.
See #53635.

#113 @hellofromTonya
3 years ago

In 51818:

Options, Meta APIs: Fix "passing null to non-nullable" deprecations to (get|add|update|delete)_option().

In all four of the get_option(), add_option(), update_option() and delete_option() functions, the $option parameter (i.e. the option name) is passed to the PHP native trim() function without prior input validation.

In PHP 8.1, this could lead to a trim(): Passing null to parameter #1 ($string) of type string is deprecated for each of these functions.

trim():

  • expects a text string and is only useful when passed a text string as no other variable type can contain whitespace.
  • will always return a string, which means that in practice for any non-string values passed, it would effectively function as a type cast to string.

This commit:

  • Adds a check to verify the $option name is a scalar before processing it with trim().
  • The "type cast" behavior is maintained.
  • If the given $option name is not a scalar, such as null, the fix prevents the PHP 8.1 deprecation notice.
  • Tests are added for valid but undesired option names to safeguard against regressions.

This issue is already covered by:

  • the existing Tests_Option_Option::test_bad_option_names() test group.
  • the new test_valid_but_undesired_option_names() tests.

Follow-up to [13858], [22633], [23510], [25002], [51817].

Props jrf, hellofromTonya, pbearne.
See #53635.

#115 @hellofromTonya
2 years ago

In 51829:

Login and Registration: Fix "passing null to non-nullable" deprecation for authorize_application error message.

If there is no URL query in the $_GET['redirect_to'], wp_parse_url() will return null. Passing null to parse_str()` results in a PHP 8.1 deprecation notice

Deprecated: parse_str(): Passing null to parameter #1 ($string) of type string is deprecated

This commit:

  • Fixes the deprecation notice.
  • Skips doing the parse_str() when there's no URL query.
  • Provides a micro-optimization performance boost.

Follow-up to [49109].

Props jrf, hellofromTonya, BinaryKitten.
See #53635.

#117 @hellofromTonya
2 years ago

In 51830:

REST API: Fix autovivification deprecation notice in WP_Test_REST_Widgets_Controller::set_up().

If the 'widget_testwidget' option does not exist, false was returned from get_option(). The set_up() logic expects an array() and assigns values to keys without checking for an array. The automatic creation of an array (autovivification) triggers a Deprecated: Automatic conversion of false to array is deprecated in deprecation notice on PHP 8.1.

This commit:

  • Fixes the deprecation notice by making the default value an empty array.
  • Moves getting the option within the conditional where it's needed.
  • Provides a micro-optimization by only getting the options when the conditions are correct for processing.
  • Makes the code consistent within the set_up() for both get_option() instances.

Follow-up to [51029].

Props jrf, hellofromTonya, BinaryKitten.
See #53635.

#119 @hellofromTonya
2 years ago

  • Keywords commit removed

In 51831:

Build/Test Tools: Fix null handling and string type casting in WP_UnitTestCase_Base::assertSameIgnoreEOL().

Basically, the whole assertSameIgnoreEOL() assertion was fundamentally flawed. The assertion contends that it checks that the expected and actual values are of the same type and value, but the reality was very different.

  • The function uses map_deep() to potentially handle all sorts of inputs.
  • map_deep() handles arrays and objects with special casing, but will call the callback on everything else without further distinction.
  • The callback used passes the expected/actual value on to the str_replace() function to remove potential new line differences.
  • And the str_replace() function will - with a non-array input for the $subject - always return a string.
  • The output of these calls to map_deep() will therefore have "normalized" _all properties_ in objects, _all values_ in arrays and _all non-object, non-array values_ to strings.
  • And a call to assertSame() will therefore NEVER do a proper type check as the type of all input has already, unintentionally, been "normalized" to string.

Aside from this clear flaw in the design of the assertion, PHP 8.1 now exposes a further issue as a null value for an object property, an array value or a plain value, will now yield a str_replace(): Passing null to parameter #3 ($subject) of type array|string is deprecated notice.

To fix both these issues, the fix in this PR ensures that the call to str_replace() will now only be made if the input is a text string.
All other values passed to the callback are left in their original type.

This ensures that a proper value AND type comparison can be done as well as prevents the PHP 8.1 deprecation notices.

Ref:

This commit:

  • Fixes type-casting of non-string values to string (the flawed part of this assertion) by invoking str_replace() when the value is of string type.
  • Fixes the PHP 8.1 str_replace(): Passing null to parameter #3 ($subject) of type array|string is deprecated deprecation notice.
  • Micro-optimization: skips map_deep() when actual and/or expected are null (no need to process).
  • Adjusts the method documentation for both this method and the assertEqualsIgnoreEOL() alias method to document that the $expected and $actual parameters can be of any type.

Follow-up to [48937], [51135], [51478].

Props jrf, hellofromTonya.
See #53363, #53635.

#121 @costdev
2 years ago

Linking #54149 for reference - "Audit get_comment() response checks."

#122 @jrf
2 years ago

GetID3 has just released a new version which includes the PHP 8.1 fixes I pulled previously.
I've opened a separate ticket for the update: https://core.trac.wordpress.org/ticket/54162

This ticket was mentioned in Slack in #core by sergey. View the logs.


2 years ago

#124 @hellofromTonya
2 years ago

In 51853:

Code Modernization: Fix "passing null to non-nullable" deprecation in _mb_substr().

The _mb_substr() function expects a string for the $str parameter, but does not do input validation. This function contains a preg_match_all() which also expects a string type for the given subject (i.e. $str).

Passing null to this parameter results in preg_match_all(): Passing null to parameter #2 ($subject) of type string is deprecated notice on PHP 8.1.

To maintain the same behaviour as before, a guard clause is added to bail out early when $str is passed as null. The outcome will, in that case, only ever be an empty string.

Note: this does mean that the _mb_substr() function now has a subtle difference in behaviour compared to the PHP native mb_substr() function as the latter will throw the deprecation notice.

The existing tests already cover this issue.

Follow-up to [17621], [36017], [32364].

Props jrf, hellofromTonya.
See #53635.

This ticket was mentioned in PR #1695 on WordPress/wordpress-develop by jrfnl.


2 years ago
#126

This adds an expectation for a "passing null to non-nullable" deprecation notice to select tests where the deprecation is generated by one of the functions in the wp-includes/formatting.php file, either via a filter hook callback or by a direct call.

Instead of haphazardly fixing these issues exposed by the tests, a more structural and all-encompassing solution for input validation should be architected and implemented as otherwise, we'll keep running into similar issues time and again with each new PHP version.

To discourage people from "fixing" these issues now anyway, this commit "hides" nearly all of these issues from the test runs.

Once a more structural solution has been designed, these tests and the underlying functions causing the deprecation notices should be revisited and the structural solution put in place.

Includes a few minor other tweaks to select tests:

  • Removing a stray return (twice) from assertion statements.
  • Removing calls to ob_*() functions in favour of letting PHPUnit manage the output catching.

This prevents warnings along the lines of Test code or tested code did not (only) close its own output buffers.

A follow up Trac ticket will be opened to address further uses of ob_*() functions in the test suite as all of those are liable to cause these same type of warnings.

Trac ticket: https://core.trac.wordpress.org/ticket/53635

jrfnl commented on PR #1695:


2 years ago
#127

Actually, there is already a ticket open to review the use of output buffering/output expectation setting in test - will follow up on the other issues in that ticket: https://core.trac.wordpress.org/ticket/53118

jrfnl commented on PR #1695:


2 years ago
#128

Actually, there is already a ticket open to review the use of output buffering/output expectation setting in test - will follow up on the other issues in that ticket: https://core.trac.wordpress.org/ticket/53118

#129 @hellofromTonya
2 years ago

In 51911:

FileSystem API: Fix autovivification deprecation notice in recurse_dirsize().

PHP natively allows for autovivification (auto-creation of arrays from falsey values). This feature is very useful and used in a lot of PHP projects, especially if the variable is undefined. However, there is a little oddity that allows creating an array from a false and null value.

The above quote is from the PHP 8.1 RFC and the (accepted) RFC changes the behaviour described above to deprecated auto creation of arrays from false. As it is deprecated, it _will_ still work for the time being, but as of PHP 9.0, this will become a Fatal Error, so we may as well fix it now.

The recurse_dirsize() function retrieves a transient and places it in the $directory_cache variable, but the get_transient() function in WP returns false when the transient doesn't exist, which subsequently can lead to the above mentioned deprecation notice.

By verifying that the $directory_cache variable is an array before assigning to it and initializing it to an empty array, if it's not, we prevent the deprecation notice, as well as harden the function against potentially corrupted transients where this transient would not return the expected array format, but some other variable type.

Includes adding dedicated unit tests for both the PHP 8.1 issue, as well as the hardening against corrupted transients.

Includes some girl-scouting: touching up a parameter description and some code layout.

Refs:

Follow-up to [49212], [49744].

Props jrf, hellofromTonya.
See #53635.

#130 @hellofromTonya
2 years ago

In 51968:

Build/Test Tools: Ignore "null to nullable" deprecations for select tests.

Adds an expectation for PHP 8.1 "passing null to non-nullable" deprecation notice to select tests where the deprecation is generated by one of the functions in the wp-includes/formatting.php file, either via a filter hook callback or by a direct call.

Instead of haphazardly fixing these issues exposed by the tests, a more structural and all-encompassing solution for input validation should be architected and implemented as otherwise, we'll keep running into similar issues time and again with each new PHP version.

To discourage people from "fixing" these issues now anyway, this commit "hides" nearly all of these issues from the test runs.

Once a more structural solution is designed, these tests and the underlying functions causing the deprecation notices should be revisited and the structural solution put in place.

Includes a few minor other tweaks to select tests:

  • Removing a stray return (twice) from assertion statements.
  • Removing calls to ob_*() functions in favour of letting PHPUnit manage the output catching. This prevents warnings along the lines of Test code or tested code did not (only) close its own output buffers.

Props jrf, hellofromTonya.
See #53635.

This ticket was mentioned in PR #1798 on WordPress/wordpress-develop by anomiex.


2 years ago
#132

This fixes the "Deprecated: Return type of Requests_Cookie_Jar::offsetExists() should be compatible with ArrayAccess::offsetExists(): bool" and similar warnings on PHP 8.1.

PHP native interfaces now have declared return types and methods in classes implementing these interfaces need to either have the return type declared (in a covariant compatible manner with the PHP native interface method declaration), or need to silence the deprecation warning using the #[ReturnTypeWillChange] attribute.

Trac ticket: https://core.trac.wordpress.org/ticket/53635

This ticket was mentioned in PR #1799 on WordPress/wordpress-develop by anomiex.


2 years ago
#133

This fixes the "Deprecated: Return type of Requests_Utility_CaseInsensitiveDictionary::offsetExists() should be compatible with ArrayAccess::offsetExists(): bool" and similar warnings on PHP 8.1.

PHP native interfaces now have declared return types and methods in classes implementing these interfaces need to either have the return type declared (in a covariant compatible manner with the PHP native interface method declaration), or need to silence the deprecation warning using the #[ReturnTypeWillChange] attribute.

Trac ticket: https://core.trac.wordpress.org/ticket/53635

This ticket was mentioned in PR #1800 on WordPress/wordpress-develop by anomiex.


2 years ago
#134

This fixes the "Deprecated: http_build_query(): Passing null to parameter #2 ($numeric_prefix) of type string is deprecated" warning on PHP 8.1.

PHP has started requiring that built-in method arguments that are not explicitly declared as nullable may no longer be passed null. The correct value for the $numeric_prefix parameter to http_build_query() is an empty string.

Trac ticket: https://core.trac.wordpress.org/ticket/53635

This ticket was mentioned in PR #1801 on WordPress/wordpress-develop by anomiex.


2 years ago
#135

This fixes a "Deprecated: trim(): Passing null to parameter #1 ($string) of type string is deprecated" warning on PHP 8.1.

PHP has started requiring that built-in method arguments that are not explicitly declared as nullable may no longer be passed null. In this case it seems to make more sense to avoid calling WordPress's own email_exists() function with null, as that method is declared as taking only strings and a null email is never going to exist anyway.

Trac ticket: https://core.trac.wordpress.org/ticket/53635

#136 @bjorsch
2 years ago

Hi! I just pushed four PRs to the GitHub mirror. Per the instructions I'm leaving a comment pointing that out as the bot's comments above apparently don't trigger notifications to people watching the ticket.

hellofromtonya commented on PR #1798:


2 years ago
#137

Hello @anomiex and thank you for your contribution. The fixes in this PR are for an external library, i.e. Requests. Fixes for this library should be made in its repo https://github.com/wordpress/requests and not in Core itself. Once Requests releases a new version, then Core will be updated.

hellofromtonya commented on PR #1799:


2 years ago
#138

Hello @anomiex and thank you for your contribution. The fixes in this PR are for an external library, i.e. Requests. Fixes for this library should be made in its repo https://github.com/wordpress/requests and not in Core itself. Once Requests releases a new version, then Core will be updated.

anomiex commented on PR #1800:


2 years ago
#139

Hmm. Hopefully you get the library updated sooner rather than later, as these errors are blocking our own work on PHP 8.1 compatibility.

anomiex commented on PR #1801:


2 years ago
#140

FYI, you are in fact running into the deprecation this is fixing in your existing tests. See for example https://github.com/WordPress/wordpress-develop/runs/4083354600?check_suite_focus=true#step:20:403

And a test that would try to pass an empty string as an email address won't actually reach this code, as that gets rejected as "invalid email address" during parameter validation. I can still add such a test if you insist, but it wouldn't actually test the code being changed here.

kraftbj commented on PR #1800:


2 years ago
#141

Hmm. Hopefully you get the library updated sooner rather than later, as these errors are blocking our own work on PHP 8.1 compatibility.

The project was recently moved into the WordPress organization, so the Core group with access (not me!) can cut a new version when needed. At worst, I would expect it in time for the 5.9 Beta 1 (Nov 16 ETA).

https://core.trac.wordpress.org/changeset/50842

jrfnl commented on PR #1801:


2 years ago
#142

I looked at that failing test before and my assessment at the time was that this is an issue with the way the test is set up, not with the actual code.

self::$user is not being correctly mocked in the wpSetUpBeforeClass() method in the WP_Test_REST_Users_Controller class which leads to the null for the email address.
This is not a situation which should ever occur in "real life" and should therefore not be solved in the src code, but by improving the test.

anomiex commented on PR #1801:


2 years ago
#143

You may want to check again. The problem isn't whether the user has an email address set, it's whether the incoming REST request specifies a value for the 'email' parameter (i.e. to update the email).

You should be able to prove that to yourself easily enough: Add something like
{{{php
if ( null === $requestemail? ) return new WP_Error( 'email_is_null', 'Email is null', array( 'status' => '500' ) );
}}}
just before the email_exists() call, then try to make a REST request to update the description of an existing user that does have an email address.

This ticket was mentioned in Slack in #core by sergey. View the logs.


2 years ago

#146 @hellofromTonya
2 years ago

In 52019:
Code Modernization: Pass correct default value to http_build_query() in get_core_checksums() and wp_version_check().

The get_core_checksums() and wp_version_check() functions call the PHP native http_build_query() function, the second parameter of which is the optional $numeric_prefix parameter which expects a non-nullable string.

A parameter being optional, however, does not automatically make it nullable.

As of PHP 8.1, passing null to a non-nullable PHP native function will generate a deprecation notice.

In this case, this function call yielded a http_build_query(): Passing null to parameter #2 ($numeric_prefix) of type string is deprecated notice.

Changing the null to an empty string fixes this without a backward compatibility break.

References:

Follow-up to [18697], [25540].

Props bjorsch, kraftbj, hellofromTonya, jrf.
See #54229.

#147 @hellofromTonya
2 years ago

#54428 was marked as a duplicate.

#148 @SergeyBiryukov
2 years ago

In 52253:

Tests: Remove unexpected output in wp_dashboard_recent_drafts() tests on PHP 8.1.

This follows the approach used in other tests to let PHPUnit manage the output catching and effectively ignore the output until retrieving it later via getActualOutput().

Follow-up to [45505], [51968], [52173].

See #53635, #53363.

#149 @SergeyBiryukov
2 years ago

In 52259:

Tests: Use a simpler approach to test the output in some tests.

Instead of ignoring the output and catching it later with getActualOutput(), we can use expectOutputRegex() directly, which evaluates the output after the rest of the test code has run, if no unexpected errors were encountered.

Follow-up to [52173], [52253].

Props jrf.
See #53635, #53363.

This ticket was mentioned in Slack in #core-php by dingo_d. View the logs.


2 years ago

@spacedmonkey
2 years ago

Screenshot of unit test run in PHP 8.1

#151 @spacedmonkey
2 years ago

As part of my work on the plugin Web stories, we upgraded our tests to run under PHP 8.1. There were a couple of notice errors we saw in our logs.

These errors are

PHP Deprecated: rtrim(): Passing null to parameter #1 ($string) of type string is deprecated in /tmp/wordpress/wp-includes/formatting.php on line 2753

PHP Deprecated: trim(): Passing null to parameter #1 ($string) of type string is deprecated in /tmp/wordpress/wp-includes/class-wp-user.php on line 211

These will need to be fixed for full PHP 8.1 support.

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


2 years ago

#153 @jrf
2 years ago

@spacedmonkey Deprecations are not errors and the behaviour of PHP has not (yet) changed for these things. The code as-is is compatible with PHP 8.1.

The typical "passing null to non-nullable" deprecations are generally all related to (the lack of) input validation/defensive coding. This is something which has been plaguing us for years now with PHP increasingly getting stricter.

So rather than making yet more hap-snap ad-hoc fixes, a discussion has been started about solving these type of issues in a more structural way by introducing input validation in a consistent manner in Core.

This discussion has only just started and has not reached a conclusion yet, but as these are deprecations (not errors), we have time until PHP 9.0 to have these conversations and solve this properly.
I'd be very happy for you to join these conversations.

Please also see my comment about this on Slack: https://wordpress.slack.com/archives/C02RQBWTW/p1637194383021500?thread_ts=1637182618.499600&cid=C02RQBWTW

#154 @SergeyBiryukov
2 years ago

#54615 was marked as a duplicate.

@Chouby
2 years ago

Avoid notices in untrailingslashit() due to the default value of script translations path being null instead of an empty string. Also the doc blocks don't document null as an accepted value for the path.

This ticket was mentioned in PR #2053 on WordPress/wordpress-develop by noisysocks.


2 years ago
#155

As of PHP 8.1, explode() does not permit null as its second argument. This results in warnings being spat out on every page because of a usage of this in wp-includes/block-supports/layout.php.

Trac ticket: https://core.trac.wordpress.org/ticket/53635

#156 @isabel_brison
2 years ago

In 52380:

Fix deprecated usage of passing null to explode()

As of PHP 8.1, explode() does not permit null as its second argument. This results in warnings being spat out on every page because of a usage of this in wp-includes/block-supports/layout.php.

See #53635.
Props noisysocks.

#157 follow-up: @jrf
2 years ago

@isabel_brison Where are the tests to accompany [52380] ? There is no safeguard in place now to ensure that this patch does not break something else.

#158 in reply to: ↑ 157 ; follow-up: @noisysocks
2 years ago

Replying to jrf:

@isabel_brison Where are the tests to accompany [52380] ? There is no safeguard in place now to ensure that this patch does not break something else.

It's a new function added in WP 5.9. Unfortunately there aren't unit tests for the function though there may be some E2E tests in Gutenberg that cover the behaviour. @youknowriad would know for sure. I wanted to fix this as it's a very prominent warning when visiting any page on a WP 5.9 site running PHP 8.1 🙂

#159 in reply to: ↑ 158 @jrf
2 years ago

Replying to noisysocks:

It's a new function added in WP 5.9. Unfortunately there aren't unit tests for the function though there may be some E2E tests in Gutenberg that cover the behaviour. @youknowriad would know for sure. I wanted to fix this as it's a very prominent warning when visiting any page on a WP 5.9 site running PHP 8.1 🙂

The fact that there apparently are no tests, is all the more reason why tests should be added.

Keep in mind that deprecation warnings are just that: deprecations. The behaviour of PHP is unchanged at this point, it is just a gentle warning that the behaviour of PHP will change in PHP 9.0.

On a normal WP install, deprecation warnings will never show. It is only when someone turns WP_DEBUG on or fiddles with PHP error_reporting settings that these will show. So these warnings being displayed is nothing to be concerned about, they are part of a bigger discussion on input validation in Core.

In contrast to what PHP is doing, your change, however, is a behavioural change, so should be covered by tests.

#160 @youknowriad
2 years ago

Indeed, it seems we have unit tests for most block supports but not the layout, we'd definitely benefit from a new one there. These features are all covered in e2e tests but for these frontend rendering functions, some additional unit tests would be good.

As for the changes itself, I'd like to encourage everyone to ideally make all the Gutenberg specific changes (block supports, ...) in the plugin as well if possible. The plugin is supposed to support two WP versions meaning that folks on previous versions won't benefit from these changes and it would create inconsistencies and some headaches for us doing the back porting for these features. (For instance this might get inadvertently reverted in the next block supports backports)

#161 @isabel_brison
2 years ago

@youknowriad the fix has been merged in Gutenberg too: https://github.com/WordPress/gutenberg/pull/37392

The main reason it was done separately in core and not as part of a backport was so @noisysocks could walk me through the core commit process :)

#162 @jrf
2 years ago

#54672 was marked as a duplicate.

#163 @SergeyBiryukov
2 years ago

#54704 was marked as a duplicate.

#164 @hellofromTonya
2 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed
  • Summary changed from PHP 8.1: various compatibility fixes to PHP 8.1: various compatibility fixes for WordPress 5.9

With 5.9 RC1 tomorrow, closing this ticket. Ongoing compatibility fixes will continue in #54730 epic ticket during the 6.0 cycle.

Thank you everyone for your contributions!!

If a regression or blocker is identified during the 5.9 RC cycle, this can be referenced and reopened if necessary with dev-reviewed.

anomiex commented on PR #1801:


2 years ago
#165

I see that you've closed https://core.trac.wordpress.org/ticket/53635 without addressing the issue this PR fixes. What's the way forward now?

hellofromtonya commented on PR #1801:


2 years ago
#166

I see that you've closed https://core.trac.wordpress.org/ticket/53635 without addressing the issue this PR fixes. What's the way forward now?

@anomiex Trac ticket https://core.trac.wordpress.org/ticket/54730 is now open for continuing PHP 8.x compatibility fixes during the 6.0 cycle. I updated the description in this PR to point at the new ticket.

anomiex commented on PR #1801:


2 years ago
#167

Looks like #2133 will fix this too, by making the called functions no longer object to being passed null. If that gets merged, this can be closed.

hellofromtonya commented on PR #1801:


2 years ago
#168

Looks like https://github.com/WordPress/wordpress-develop/pull/2133 will fix this too, by making the called functions no longer object to being passed null. If that gets merged, this can be closed.

Once #2133 gets merged, then yes, it will resolve this PR's deprecation notice without further changes. Plus #2133 resolves the issue at the lower level, i.e. where the deprecation notice is triggered. I think overall this PR can be closed in favor of #2133.

Note: There's an improvement opportunity, though out of scope for the purpose of this PR:

  • Guard email_exists() to invoke get_user_by() only when a string type is passed to email_exists(), i.e. providing a teeny tiny micro-optimization when a non-string is passed. (This tweak deserves a separate PR and tests.)

SergeyBiryukov commented on PR #1658:


18 months ago
#169

Thanks for the PR! I believe this was superseded by #3187.

jrfnl commented on PR #1658:


18 months ago
#170

I believe this was superseded by #3187.

Correct. Thanks for getting the improved version merged!

Note: See TracTickets for help on using tickets.