WordPress.org

Make WordPress Core

Opened 4 months ago

Last modified 7 weeks ago

#51423 accepted task (blessed)

Fix function argument type issues reported by PHPStan

Reported by: SergeyBiryukov Owned by: SergeyBiryukov
Milestone: 5.7 Priority: normal
Severity: normal Version:
Component: General Keywords: php8 has-patch has-unit-tests
Focuses: coding-standards Cc:

Description (last modified by hellofromTonya)

Background: #50913

Splitting this out from comment:44:ticket:50913. @xknown is going through some issues reported by PHPStan, mostly in places where we are passing the incorrect type to WP core or PHP's built-in functions.

Let's use this ticket to include these fixes in core.

Goals are to:

  • retain the current behavior (i.e. to avoid breaking changes)
  • retain the triggering of notice/warning/error, which are clues and indicators (i.e. rather than silently letting the problem propagate downstream)
  • gain PHP 8 compatibility
  • ensure code coverage to validate changes

Change History (59)

#1 @SergeyBiryukov
4 months ago

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

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


4 months ago

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


4 months ago

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


3 months ago

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


3 months ago

This ticket was mentioned in PR #553 on WordPress/wordpress-develop by xknown.


3 months ago

  • Keywords has-patch has-unit-tests added

This PR attempts to fix the arguments type passed to a WP core and PHP's built-in functions, which would hopefully help reduce the number of PHP fatals for PHP 8.0.

The full list of reported issues can be seen at

https://gist.github.com/xknown/77f8cbe233da75080d1e9258a8c94f95

https://core.trac.wordpress.org/ticket/51423

#7 @prbot
3 months ago

jrfnl commented on PR #553:

Looked through a few of the changes (by far not all) and would like to observe the following: a lot of these changes bypass underlying errors by doing type casting, instead of handling the errors properly. While this will work, I'm not sure this is the always the right choice.

#8 @prbot
3 months ago

xknown commented on PR #553:

Looked through a few of the changes (by far not all) and would like to observe the following: a lot of these changes bypass underlying errors by doing type casting, instead of handling the errors properly. While this will work, I'm not sure this is always the right choice.

Thanks for the feedback, and sorry for the late response. I have been busy with some work-related things, and some afk time. I agree that most of the changes are only adding explicit casts, which is probably fine in most cases.

I've been manually reviewing these changes, and while I add some proper error checking in some places, some commits only add explicit casts mostly because the surrounding code don't really handle very well error conditions (one example that comes to my mind is errors happening when running wp_install()). If you point me out a few cases where do you think we can do something different, then please let me know.

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


3 months ago

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


3 months ago

#11 follow-up: @xknown
2 months ago

I compiled a list of changes that it'd good to include in the 5.6 release to avoid potential PHP 8.0 issues (some of them fix real PHP 8.0 fatals), if we want to leave the unneeded type casts changes out of them, that's fine for me. At this point I think it'd be good to add either these patches here or maybe have an individual PR for each change, so it's easier to be reviewed.

Last edited 2 months ago by hellofromTonya (previous) (diff)

#12 in reply to: ↑ 11 @hellofromTonya
2 months ago

Replying to xknown:

At this point I think it'd be good to add either these patches here or maybe have an individual PR for each change, so it's easier to be reviewed.

Talking with @xknown and @jrf, I'll help with the task of splitting these commits into separate patches or PRs. These smaller chunks of code will allow for discussion and code review at more granular level. I'll get this task done today.

This ticket was mentioned in PR #709 on WordPress/wordpress-develop by xknown.


2 months ago

Updates wp_update_link to handle invalid link ids, it also adds a couple
of unit tests.

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

This ticket was mentioned in PR #711 on WordPress/wordpress-develop by xknown.


2 months ago

It fixes the following PHP fatal when the fetch_feed function is called.

PHP Fatal error:  Uncaught TypeError: call_user_func_array(): Argument
WP_Feed_Cache::create() cannot be called statically in /var/www/wp/wp-includes/SimplePie/Registry.php:214

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

This ticket was mentioned in PR #712 on WordPress/wordpress-develop by xknown.


2 months ago

  • Make sure we handle properly the cases where core functions return a

WP_Error instance. Otherwise they could produce type errors.

  • Update the PHPDoc type hints to take into account the default values.
  • Adds explicit casts to str_repeat which expects an integer. This is

not required, but might be good to start fixing them as they might need
to be updated in future PHP versions.

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

This ticket was mentioned in PR #718 on WordPress/wordpress-develop by hellofromtonya.


2 months ago

This ticket was mentioned in PR #719 on WordPress/wordpress-develop by hellofromtonya.


2 months ago

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

wp-admin/nav-menus.php: Handle error conditions, and also pass the appropriate types to avoid potential PHP type errors/warnings.

Props @xknown.

Split of larger PR for this commit.

This ticket was mentioned in PR #720 on WordPress/wordpress-develop by hellofromtonya.


2 months ago

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

Props @xknown

Split of larger PR for commit.

This ticket was mentioned in PR #722 on WordPress/wordpress-develop by hellofromtonya.


2 months ago

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

Props @xknown

Split of a larger PR in commit https://github.com/WordPress/wordpress-develop/commit/ab4657c1adbd7b104543a6d0b9423365513eca3d

`fopen`

Returns a file pointer resource on success, or FALSE on failure

This ticket was mentioned in PR #726 on WordPress/wordpress-develop by xknown.


2 months ago

Update WP_Site_Icon::create_attachment_object() as well as
WP_Site_Icon::insert_attachment() to handle error conditions.

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

This ticket was mentioned in PR #727 on WordPress/wordpress-develop by xknown.


2 months ago

Add WP_Error checks for the get_terms calls to avoid PHP 8.0 type errors.

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

This ticket was mentioned in PR #728 on WordPress/wordpress-develop by xknown.


2 months ago

list_files might return false, which will produce a type error in PHP 8.0 when used in the

array_map call.

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

This ticket was mentioned in PR #729 on WordPress/wordpress-develop by xknown.


2 months ago

Don't attempt to use wp_list_pluck on a WP_Error object.

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

This ticket was mentioned in PR #730 on WordPress/wordpress-develop by xknown.


2 months ago

Check the potential error conditions which might produce type errors in PHP 8.0.

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

This ticket was mentioned in PR #731 on WordPress/wordpress-develop by xknown.


2 months ago

  • Adds a check to make sure we are iterating on a non null collection.
  • Fixes also a bug on a ternary operator that casts the condition to integer (the cast operator takes precedence over the ternary operator), not the page value.

See the example below.

$page = 'a0';
var_dump((int) ( '' == $page ) ? 1 : $page);
---
string(2) "a0"

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

This ticket was mentioned in PR #732 on WordPress/wordpress-develop by xknown.


2 months ago

Check for error conditions to avoid potential type errors.

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

This ticket was mentioned in PR #733 on WordPress/wordpress-develop by xknown.


2 months ago

Handle error conditions to avoid potential type errors.

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

This ticket was mentioned in PR #734 on WordPress/wordpress-develop by xknown.


2 months ago

Handle error conditions.

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

#33 @prbot
2 months ago

xknown commented on PR #553:

Closing in favor of individual PRs. See https://core.trac.wordpress.org/ticket/51423 for more details.

#35 @prbot
2 months ago

hellofromtonya commented on PR #734:

Closing in favor of PR https://github.com/WordPress/wordpress-develop/pull/735 which includes using false failure check for fopen for consistency, ie instead of is_resource.

This ticket was mentioned in PR #736 on WordPress/wordpress-develop by hellofromtonya.


2 months ago

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

Fixes wp-includes/bookmark.php for array_unique:

"Parameter #1 $input of function array_unique expects array, array|WP_Error given."

#40 follow-up: @jrf
2 months ago

Hi @xknown and @hellofromTonya, thank you so much for taking this on, breaking the changes up into individual PRs and adding tests. This will definitely make it easier to review and discuss the proposed changes.

At the request of @hellofromTonya I've reviewed PR #736 now and left an extensive comment for you to consider: https://github.com/WordPress/wordpress-develop/pull/736#pullrequestreview-528734649

The feedback I left there might also help inform how to handle some of the other changes needed.

Hope it helps!

#41 @prbot
2 months ago

hellofromtonya commented on PR #736:

@jrfnl You are correct.

### Changed code's behavior

The code's behavior was changed when WP_Error is returned when 'link_category' taxonomy is not registered. This is not desired as it could be a breaking change with no benefits.

This was bugging me last night, but wasn't well-formed. Thank you for thoroughly and clearly laying out why and next steps. 👏

Updates:

  • is_wp_error() fires an action. Using it then also changes the behavior.
  • Switched to check for the non-error state, i.e. array where wp_get_object_terms() returns array unless the taxonomy does not exist, then returns WP_Error.

### Test Cases

#42 in reply to: ↑ 40 @hellofromTonya
2 months ago

Replying to jrf:

At the request of @hellofromTonya I've reviewed PR #736 now and left an extensive comment for you to consider: https://github.com/WordPress/wordpress-develop/pull/736#pullrequestreview-528734649

The feedback I left there might also help inform how to handle some of the other changes needed.

Hope it helps!

Your thorough and extensive comment gave me an Aha Moment! I can now clearly see both the goal of this ticket, the process, and next steps for all of its patches/PRs.

The goal of this ticket is to match the data types of each function parameter, i.e. as part of the PHP 8 compatibility.

We should seek:

  • to retain the current behavior (i.e. to avoid breaking changes)
  • to retain the triggering of notice/warning/error, which are clues and indicators (i.e. rather than silently letting the problem propagate downstream)
  • to gain PHP 8 compatibility

#43 @prbot
2 months ago

jrfnl commented on PR #736:

@hellofromtonya Glad you found the feedback useful.

As a side-note: the chances of this error condition ever being hit in a real life situation are very small as that would involve a plugin/theme explicitly (and potentially maliciously) de-registering the WP native link_category taxonomy and if that would happen, more things are likely to break anyhow.

Having said that, this doesn't negate the validity of this patch. No matter how small the chance of an error condition being hit, the error condition should preferably be handled gracefully, which is now is. So :+1: for me for this patch to be committed.

#44 @hellofromTonya
2 months ago

  • Description modified (diff)

#45 @hellofromTonya
2 months ago

  • Keywords early added

Moving this ticket to 5.7 as RC1 is tomorrow. Why? Discussions are ongoing to figure out how to achieve the stated goals. The incremental PRs/patches do change source code. Thus, need more time past RC.

Marking as early to ensure it gets continued attention early enough in the next cycle.

#46 @hellofromTonya
2 months ago

  • Milestone changed from 5.6 to 5.7

#47 @SergeyBiryukov
2 months ago

  • Keywords early removed
  • Milestone changed from 5.7 to 5.6

Didn't get a chance to review this before 5.6 RC, but I think if some of these fixes help prevent unnecessary breakage, they might still be a good fit for this release, even if there are some code changes.

Moving back for now, would like to take a closer look for 5.6 RC2.

#48 @prbot
2 months ago

hellofromtonya commented on PR #720:

Closing this PR as the fix is not necessary. This is a false PHPStan trigger.

Why? str_replace returns either string or array for PHP 5, 7, and 8. See check here https://3v4l.org/Lfmgp. The original code will work the same in PHP 8.

This ticket was mentioned in PR #755 on WordPress/wordpress-develop by hellofromtonya.


2 months ago

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

  • Walker_Comment: ensures avatar size is an integer type when passed to get_avatar()
  • Adds happy and unhappy path to validate avatar for different size types

#50 @prbot
2 months ago

hellofromtonya commented on PR #739:

Closes in preference to https://github.com/WordPress/wordpress-develop/pull/755 which contains fix to HTML5 method and adds more unhappy and happy test cases.

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


2 months ago

#52 @prbot
2 months ago

hellofromtonya commented on PR #718:

AtomParser is part of an external library that was added to core's source in 2.3.0. The original library has not been updated since 2007.

This PR modifies this original library's source. Do we wish to make this change in core or contribute upstream?

#53 @prbot
2 months ago

hellofromtonya commented on PR #709:

Status: Blocked

The solution for this PR is currently blocked until we figure out the best way to handle wp_get_object_terms. The discussion is happening in https://github.com/WordPress/wordpress-develop/pull/736. Once we land on an implementation, I can update this PR.

This ticket was mentioned in PR #758 on WordPress/wordpress-develop by hellofromtonya.


2 months ago

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

Function: wp_privacy_generate_personal_data_export_file()

Resolves "Parameter #1 $arr1 of function array_merge expects array, array|bool given."

  • Fixes the type match for array_merge to prevent the PHP 8 fatal error
  • Adds test coverage
  • Refactors tests to use data provider

## TODO:

  • [x] Analyze
  • [x] Add code coverage
  • [x] Update fix
  • [ ] Add error for when option is exists but is not an array, ie meaning a 3rd party has changed its expected type.

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


8 weeks ago

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


8 weeks ago

This ticket was mentioned in PR #776 on WordPress/wordpress-develop by hellofromtonya.


8 weeks ago

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

Function: wp_privacy_process_personal_data_export_page()

Resolves "Parameter #1 $arr1 of function array_merge expects array, array|bool given."

Fixes the type match for array_merge to prevent the PHP 8 fatal error

  • Adds test coverage
  • Retains error behavior when/if '_export_data_raw' is not an array type. Error is passed via wp_send_json_error. Tests added to validate.

#59 @hellofromTonya
7 weeks ago

  • Milestone changed from 5.6 to 5.7

Punting this ticket to 5.7 as today is 5.6 RC2.

If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

Note: See TracTickets for help on using tickets.