WordPress.org

Make WordPress Core

Opened 9 months ago

Last modified 7 days ago

#51423 assigned task (blessed)

Fix function argument type issues reported by PHPStan

Reported by: SergeyBiryukov Owned by: hellofromTonya
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: General Keywords: php8
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

Attachments (5)

51423-setup_export_contents_test (2.0 KB) - added by hellofromTonya 3 months ago.
Comments Tests_Privacy_WpPrivacyGeneratePersonalDataExportFile::setup_export_contents_test
51423-setup_export_contents_test.diff (2.0 KB) - added by hellofromTonya 3 months ago.
Comments Tests_Privacy_WpPrivacyGeneratePersonalDataExportFile::setup_export_contents_test
51423.wp-admin-network-settings.diff (2.4 KB) - added by SergeyBiryukov 2 months ago.
51423.wp-admin-network-settings.2.diff (2.5 KB) - added by SergeyBiryukov 2 months ago.
51423.type-casting-vs-empty-check-test.php (600 bytes) - added by SergeyBiryukov 8 weeks ago.

Download all attachments as: .zip

Change History (108)

#1 @SergeyBiryukov
9 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.


9 months ago

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


8 months ago

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


8 months ago

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


8 months ago

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


8 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
8 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
7 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.


7 months ago

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


7 months ago

#11 follow-up: @xknown
7 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 7 months ago by hellofromTonya (previous) (diff)

#12 in reply to: ↑ 11 @hellofromTonya
7 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.


7 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.


7 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.


7 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.


7 months ago

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


7 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.


7 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.


7 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.


7 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.


7 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.


7 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.


7 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.


7 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.


7 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.


7 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.


7 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.


7 months ago

Handle error conditions.

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

#33 @prbot
7 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
7 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.


7 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
7 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
7 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
7 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
7 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
7 months ago

  • Description modified (diff)

#45 @hellofromTonya
7 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
7 months ago

  • Milestone changed from 5.6 to 5.7

#47 @SergeyBiryukov
7 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
7 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.


7 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
7 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.


7 months ago

#52 @prbot
7 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
7 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.


7 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.


7 months ago

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


7 months ago

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


7 months 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 months 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.

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


5 months ago

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


4 months ago

#62 @hellofromTonya
4 months ago

  • Milestone changed from 5.7 to 5.8

With 5.7 RC1 landing tomorrow, ran out of time for this ticket in 5.7. Punting it to 5.8.

Continuing in 5.8:
Each instance is being reviewed and validated, one-by-one. Effort will continue in 5.8 with incremental guarding and improvements to be committed for each instance.

#63 @hellofromTonya
3 months ago

  • Keywords has-patch has-unit-tests removed
  • Owner changed from SergeyBiryukov to hellofromTonya
  • Status changed from accepted to assigned

Switching ownership to me as I'm actively working on it and want to shepherd it forward throughout 5.8 cycle.

Also removing patch and tests keywords as the patches are yet ready for review or commit.

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


3 months ago

  • Keywords has-patch added

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

Fixes a data type bug as well as correcting the expected data type for the WP_User_Search::$page property.

#65 @prbot
3 months ago

hellofromtonya commented on PR #731:

After pair programming session, decided to split this PR into 2:

  • Fixing the page property type mismatch in the constructor done here in PR #1091
  • Fixing the other part will require additional investigation to determine what types can be returned and where the mismatch exists at the root

Closing this PR as the new ones take precedence and will contain the latest fixes and progression.

#67 @prbot
3 months ago

hellofromtonya commented on PR #722:

Force push to reset the file back to original state (except for the indentation). Why? To first show the problem exists in PHP 8, but not in the other PHP versions. Doing this step shows the problem in action for crafting the right fix without causing a breaking change.

#68 @prbot
3 months ago

hellofromtonya commented on PR #722:

## Manual Testing BEFORE the Fix

Environment:

  • Using Local as the localhost app
  • Nginx
  • MySQL 8.0.16
  • OS: Mac Big Sur
  • Browser: Firefox

To make it testable, needed to create an environment where fopen fails to create the file:

  • Changed the permissions of the existing wp-config.php to 000

{{{bash


}}}

  • Skipped file_exists check by commenting the check on lines 58 to 69
  • Flushed the realpath cache for the file

{{{php
clearstatcache( ABSPATH . 'wp-config.php' );
}}}

### Test 1: with PHP 7.3.5

Results: Works, but PHP Warning

From the error.log

PHP Warning:  fwrite() expects parameter 1 to be resource, bool given in .../setupconfig/app/public/wp-admin/setup-config.php on line 443

https://i0.wp.com/user-images.githubusercontent.com/7284611/111212275-68d3c200-859d-11eb-9d56-a026711f27ff.gif

### Test 2: with PHP 8.0

Results: Does not work due to TypeError fatal error

From the error.log

PHP Fatal error:  Uncaught TypeError: fwrite(): Argument #1 ($stream) must be of type resource, bool given in .../setupconfig/app/public/wp-admin/setup-config.php:443
Stack trace:
#0 .../setupconfig/app/public/wp-admin/setup-config.php(443): fwrite(false, '<?php\r\n')
#1 {main}
  thrown in .../setupconfig/app/public/wp-admin/setup-config.php on line 443

https://i0.wp.com/user-images.githubusercontent.com/7284611/111212495-a8021300-859d-11eb-89e5-9b794b0e4c28.gif

#69 @prbot
3 months ago

hellofromtonya commented on PR #722:

## Manual Testing AFTER the Fix

Same environment and test conditions.

### Test 1: with PHP 7.3.5

Results: Works with no breaking change.

  • wp-config.php file permissions is changed
  • Error message displays as expected
  • Error Log:
    • No fwrite PHP Warning
    • PHP Warning for fopen as expected: PHP Warning: fopen(/.../setupconfig/app/public/wp-config.php): Failed to open stream: Permission denied

https://i0.wp.com/user-images.githubusercontent.com/7284611/111224407-6cbb1080-85ac-11eb-9fcd-b2fe005f5b53.gif

### Test 2: with PHP 8.0

Results: Works.

  • wp-config.php file permissions is changed
  • Error message displays as expected
  • Error log:
    • No PHP TypeError Fatal Error
    • PHP Warning for fopen as expected: PHP Warning: fopen(/.../setupconfig/app/public/wp-config.php): Failed to open stream: Permission denied

https://i0.wp.com/user-images.githubusercontent.com/7284611/111225019-3b8f1000-85ad-11eb-99ac-0cce5c299043.gif

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


3 months ago

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

Fixes the to string handling for the 'limited_email_domains' option in the network settings admin page.

The 3rd argument for `str_replace` is expected to be a string or array. It will return a string as the search and replace args are strings.

However, get_option returns mixed data types including string, array, false, or any other "filtered" data type (i.e. passing back from the option filters).

Reference:

#71 @prbot
3 months ago

hellofromtonya commented on PR #1100:

I think the PHPStan flag on this one is a false positive and a fix is not needed here.

Why?

  • The code behaves the same regardless if it's PHP 5.6, 7+, or 8.
  • The fix is a breaking change as the fatal error will no longer happen.

Why?

  • str_replace 3rd parameter (haystack) is documented as only accepting a string or array, but in tests it also handles boolean, int, and float without a notice, warning, or error. See it https://3v4l.org/EGTX0. If it receives anything else, a fatal error is throwing in PHP 5.6+. See it in action here https://3v4l.org/poqHP.
  • Though get_site_option can return a variety of mixed values, the str_replace already handles it consistently.
  • If no error occurs, then str_replace will always return a string. Why? Its search and replace params are strings.
  • implode will always received a string that is then type casted to an array.

@jrfnl What do you think?

#72 @prbot
3 months ago

hellofromtonya commented on PR #758:

## Testing BEFORE fixes

### PHP 7.3.5

Results:

  • Export file downloaded ✅
  • 2 PHP Warnings

<img width="567" alt="Screen Shot 2021-03-18 at 4 46 23 PM" src="https://user-images.githubusercontent.com/7284611/111701969-9624a800-8809-11eb-875d-2f2fa8fe896a.png">

Error log:

[18-Mar-2021 21:46:02 UTC] PHP Warning:  array_merge(): Expected parameter 2 to be an array, string given in .../public/wp-admin/includes/privacy-tools.php on line 397
[18-Mar-2021 21:46:02 UTC] PHP Stack trace:
[18-Mar-2021 21:46:02 UTC] PHP   1. {main}() .../public/wp-admin/admin-ajax.php:0
[18-Mar-2021 21:46:02 UTC] PHP   2. do_action() .../public/wp-admin/admin-ajax.php:187
[18-Mar-2021 21:46:02 UTC] PHP   3. WP_Hook->do_action() .../public/wp-includes/plugin.php:484
[18-Mar-2021 21:46:02 UTC] PHP   4. WP_Hook->apply_filters() .../public/wp-includes/class-wp-hook.php:316
[18-Mar-2021 21:46:02 UTC] PHP   5. wp_ajax_wp_privacy_export_personal_data() .../public/wp-includes/class-wp-hook.php:292
[18-Mar-2021 21:46:02 UTC] PHP   6. apply_filters() .../public/wp-admin/includes/ajax-actions.php:4911
[18-Mar-2021 21:46:02 UTC] PHP   7. WP_Hook->apply_filters() .../public/wp-includes/plugin.php:212
[18-Mar-2021 21:46:02 UTC] PHP   8. wp_privacy_process_personal_data_export_page() .../public/wp-includes/class-wp-hook.php:292
[18-Mar-2021 21:46:02 UTC] PHP   9. do_action() .../public/wp-admin/includes/privacy-tools.php:844
[18-Mar-2021 21:46:02 UTC] PHP  10. WP_Hook->do_action() .../public/wp-includes/plugin.php:484
[18-Mar-2021 21:46:02 UTC] PHP  11. WP_Hook->apply_filters() .../public/wp-includes/class-wp-hook.php:316
[18-Mar-2021 21:46:02 UTC] PHP  12. wp_privacy_generate_personal_data_export_file() .../public/wp-includes/class-wp-hook.php:292
[18-Mar-2021 21:46:02 UTC] PHP  13. array_merge() .../public/wp-admin/includes/privacy-tools.php:397
[18-Mar-2021 21:46:02 UTC] PHP Warning:  count(): Parameter must be an array or an object that implements Countable in .../public/wp-admin/includes/privacy-tools.php on line 399
[18-Mar-2021 21:46:02 UTC] PHP Stack trace:
[18-Mar-2021 21:46:02 UTC] PHP   1. {main}() .../public/wp-admin/admin-ajax.php:0
[18-Mar-2021 21:46:02 UTC] PHP   2. do_action() .../public/wp-admin/admin-ajax.php:187
[18-Mar-2021 21:46:02 UTC] PHP   3. WP_Hook->do_action() .../public/wp-includes/plugin.php:484
[18-Mar-2021 21:46:02 UTC] PHP   4. WP_Hook->apply_filters() .../public/wp-includes/class-wp-hook.php:316
[18-Mar-2021 21:46:02 UTC] PHP   5. wp_ajax_wp_privacy_export_personal_data() .../public/wp-includes/class-wp-hook.php:292
[18-Mar-2021 21:46:02 UTC] PHP   6. apply_filters() .../public/wp-admin/includes/ajax-actions.php:4911
[18-Mar-2021 21:46:02 UTC] PHP   7. WP_Hook->apply_filters() .../public/wp-includes/plugin.php:212
[18-Mar-2021 21:46:02 UTC] PHP   8. wp_privacy_process_personal_data_export_page() .../public/wp-includes/class-wp-hook.php:292
[18-Mar-2021 21:46:02 UTC] PHP   9. do_action() .../public/wp-admin/includes/privacy-tools.php:844
[18-Mar-2021 21:46:02 UTC] PHP  10. WP_Hook->do_action() .../public/wp-includes/plugin.php:484
[18-Mar-2021 21:46:02 UTC] PHP  11. WP_Hook->apply_filters() .../public/wp-includes/class-wp-hook.php:316
[18-Mar-2021 21:46:02 UTC] PHP  12. wp_privacy_generate_personal_data_export_file() .../public/wp-includes/class-wp-hook.php:292

### PHP 8.0

Results:

  • Fatal error
  • Export failed

<img width="477" alt="Screen Shot 2021-03-18 at 4 44 47 PM" src="https://user-images.githubusercontent.com/7284611/111701813-58278400-8809-11eb-9a69-fc555f1ed66f.png">

Error log:

[18-Mar-2021 21:37:33 UTC] PHP Fatal error:  Uncaught TypeError: array_merge(): Argument #2 must be of type array, string given in .../public/wp-admin/includes/privacy-tools.php:397
Stack trace:
#0 .../public/wp-admin/includes/privacy-tools.php(397): array_merge(Array, 'invalid type')
#1 .../app/public/wp-includes/class-wp-hook.php(292): wp_privacy_generate_personal_data_export_file(21)
#2 .../public/wp-includes/class-wp-hook.php(316): WP_Hook->apply_filters(NULL, Array)
#3 .../public/wp-includes/plugin.php(484): WP_Hook->do_action(Array)
#4 .../public/wp-admin/includes/privacy-tools.php(844): do_action('wp_privacy_pers...', 21)
#5 .../public/wp-includes/class-wp-hook.php(292): wp_privacy_process_personal_data_export_page(Array, 3, 'tester2@test.co...', 1, 21, false, 'wordpress-media')
#6 .../public/wp-includes/plugin.php(212): WP_Hook->apply_filters(Array, Array)
#7 .../public/wp-admin/includes/ajax-actions.php(4911): apply_filters('wp_privacy_pers...', Array, 3, 'tester2@test.co...', 1, 21, false, 'wordpress-media')
#8 .../public/wp-includes/class-wp-hook.php(292): wp_ajax_wp_privacy_export_personal_data('')
#9 .../public/wp-includes/class-wp-hook.php(316): WP_Hook->apply_filters('', Array)
#10 .../public/wp-includes/plugin.php(484): WP_Hook->do_action(Array)
#11 .../public/wp-admin/admin-ajax.php(187): do_action('wp_ajax_wp-priv...')
#12 {main}
  thrown in .../public/wp-admin/includes/privacy-tools.php on line 397

### How Tested

Used this code in a must-use to change the meta value to a string value:

{{{php
<?php

add_action( 'wp_privacy_personal_data_export_file', function( $request_id ) {

update_post_meta( $request_id, '_export_data_grouped', 'invalid type' );

} , 5 );
}}}

Tests to reproduce:

  1. Create a wp-content/mu-plugins/51423.php and add code above.
  2. Add a test user, e.g. username tester2 and password tester2@test.com.
  3. Go to Tools > Export Personal Data.
  4. Create a request for the user.
  5. Attempt to download the export file.

#73 @hellofromTonya
3 months ago

@SergeyBiryukov PR1091 is for ready for commit. @jrf and I have reviewed and tested it.

This one fixes a data type bug which then also fixes the assignment of the WP_User_Search::$page to match its expected data type.

#74 @prbot
3 months ago

hellofromtonya commented on PR #758:

## Testing AFTER fixes

### PHP 7.3.5

Results:

  • Export file downloaded ✅
  • No PHP Warnings
  • 1 PHP Notice

<img width="1088" alt="Screen Shot 2021-03-22 at 2 18 05 PM" src="https://user-images.githubusercontent.com/7284611/112046002-7cd96f80-8b19-11eb-8a07-0255e63a6cbe.png">

Error log:

[22-Mar-2021 19:16:05 UTC] PHP Notice:  wp_privacy_generate_personal_data_export_file was called <strong>incorrectly</strong>. The <code>'_export_data_grouped'</code> post meta must be an array. Please see <a href="https://wordpress.org/support/article/debugging-in-wordpress/">Debugging in WordPress</a> for more information. (This message was added in version 5.8.0.) in .../public/wp-includes/functions.php on line 5313
[22-Mar-2021 19:16:05 UTC] PHP Stack trace:
[22-Mar-2021 19:16:05 UTC] PHP   1. {main}() .../public/wp-admin/admin-ajax.php:0
[22-Mar-2021 19:16:05 UTC] PHP   2. do_action() .../public/wp-admin/admin-ajax.php:187
[22-Mar-2021 19:16:05 UTC] PHP   3. WP_Hook->do_action() .../public/wp-includes/plugin.php:484
[22-Mar-2021 19:16:05 UTC] PHP   4. WP_Hook->apply_filters() .../public/wp-includes/class-wp-hook.php:316
[22-Mar-2021 19:16:05 UTC] PHP   5. wp_ajax_wp_privacy_export_personal_data() /Users/hellofromtonya/Local/wpcore/app/public/wp-includes/class-wp-hook.php:292
[22-Mar-2021 19:16:05 UTC] PHP   6. apply_filters() .../public/wp-admin/includes/ajax-actions.php:4911
[22-Mar-2021 19:16:05 UTC] PHP   7. WP_Hook->apply_filters() .../public/wp-includes/plugin.php:212
[22-Mar-2021 19:16:05 UTC] PHP   8. wp_privacy_process_personal_data_export_page() .../public/wp-includes/class-wp-hook.php:292
[22-Mar-2021 19:16:05 UTC] PHP   9. do_action() .../public/wp-admin/includes/privacy-tools.php:862
[22-Mar-2021 19:16:05 UTC] PHP  10. WP_Hook->do_action() .../public/wp-includes/plugin.php:484
[22-Mar-2021 19:16:05 UTC] PHP  11. WP_Hook->apply_filters() .../public/wp-includes/class-wp-hook.php:316
[22-Mar-2021 19:16:05 UTC] PHP  12. wp_privacy_generate_personal_data_export_file() .../public/wp-includes/class-wp-hook.php:292
[22-Mar-2021 19:16:05 UTC] PHP  13. _doing_it_wrong() .../public/wp-admin/includes/privacy-tools.php:411
[22-Mar-2021 19:16:05 UTC] PHP  14. trigger_error() .../public/wp-includes/functions.php:5313

In the downloaded zip file:

  • export.json:

{{{json
{"Personal Data Export for tester2@…":null}
}}}

  • `index.html

{{{html
<!DOCTYPE html>
<html>
<head>
<meta http-equiv='Content-Type' content='text/html; charset=UTF-8' />
<style type='text/css'>body { color: black; font-family: Arial, sans-serif; font-size: 11pt; margin: 15px auto; width: 860px; }table { background: #f0f0f0; border: 1px solid #ddd; margin-bottom: 20px; width: 100%; }th { padding: 5px; text-align: left; width: 20%; }td { padding: 5px; }tr:nth-child(odd) { background-color: #fafafa; }.return-to-top { text-align: right; }</style><title>Personal Data Export for tester2@…</title></head>
<body>
<h1 id="top">Personal Data Export</h1></body>
</html>
}}}

### PHP 8.0

Results:

  • Export file downloaded ✅
  • No PHP Fatal error
  • 1 PHP Notice

<img width="1069" alt="Screen Shot 2021-03-22 at 2 23 30 PM" src="https://user-images.githubusercontent.com/7284611/112046632-48b27e80-8b1a-11eb-8c88-89b4d2fc3c2f.png">

Error log:

[22-Mar-2021 19:23:22 UTC] PHP Notice:  wp_privacy_generate_personal_data_export_file was called <strong>incorrectly</strong>. The <code>'_export_data_grouped'</code> post meta must be an array. Please see <a href="https://wordpress.org/support/article/debugging-in-wordpress/">Debugging in WordPress</a> for more information. (This message was added in version 5.8.0.) in .../public/wp-includes/functions.php on line 5313
[22-Mar-2021 19:23:22 UTC] PHP Stack trace:
[22-Mar-2021 19:23:22 UTC] PHP   1. {main}().../public/wp-admin/admin-ajax.php:0
[22-Mar-2021 19:23:22 UTC] PHP   2. do_action() .../public/wp-admin/admin-ajax.php:187
[22-Mar-2021 19:23:22 UTC] PHP   3. WP_Hook->do_action() .../public/wp-includes/plugin.php:484
[22-Mar-2021 19:23:22 UTC] PHP   4. WP_Hook->apply_filters() .../public/wp-includes/class-wp-hook.php:316
[22-Mar-2021 19:23:22 UTC] PHP   5. wp_ajax_wp_privacy_export_personal_data() .../public/wp-includes/class-wp-hook.php:292
[22-Mar-2021 19:23:22 UTC] PHP   6. apply_filters() .../public/wp-admin/includes/ajax-actions.php:4911
[22-Mar-2021 19:23:22 UTC] PHP   7. WP_Hook->apply_filters() .../public/wp-includes/plugin.php:212
[22-Mar-2021 19:23:22 UTC] PHP   8. wp_privacy_process_personal_data_export_page() .../public/wp-includes/class-wp-hook.php:292
[22-Mar-2021 19:23:22 UTC] PHP   9. do_action() .../public/wp-admin/includes/privacy-tools.php:862
[22-Mar-2021 19:23:22 UTC] PHP  10. WP_Hook->do_action() .../public/wp-includes/plugin.php:484
[22-Mar-2021 19:23:22 UTC] PHP  11. WP_Hook->apply_filters() .../public/wp-includes/class-wp-hook.php:316
[22-Mar-2021 19:23:22 UTC] PHP  12. wp_privacy_generate_personal_data_export_file() .../public/wp-includes/class-wp-hook.php:292
[22-Mar-2021 19:23:22 UTC] PHP  13. _doing_it_wrong() .../public/wp-admin/includes/privacy-tools.php:411
[22-Mar-2021 19:23:22 UTC] PHP  14. trigger_error() .../public/wp-includes/functions.php:5313

In the downloaded zip file:

  • export.json:

{{{json
{"Personal Data Export for tester2@…":null}
}}}

  • `index.html

{{{html
<!DOCTYPE html>
<html>
<head>
<meta http-equiv='Content-Type' content='text/html; charset=UTF-8' />
<style type='text/css'>body { color: black; font-family: Arial, sans-serif; font-size: 11pt; margin: 15px auto; width: 860px; }table { background: #f0f0f0; border: 1px solid #ddd; margin-bottom: 20px; width: 100%; }th { padding: 5px; text-align: left; width: 20%; }td { padding: 5px; }tr:nth-child(odd) { background-color: #fafafa; }.return-to-top { text-align: right; }</style><title>Personal Data Export for tester2@…</title></head>
<body>
<h1 id="top">Personal Data Export</h1></body>
</html>
}}}

#75 @prbot
3 months ago

hellofromtonya commented on PR #758:

@jrfnl This PR is ready for a final review. Since we last did a pair programming session on it, I discovered our solution caused a breaking change. Why? When the post meta does not exist or it's value is not an array, no about group content is generated in the downloaded zip file.

Why? See the updated notes in the description of this PR which includes 3v4l links to show it in action.

What's different since you and I last worked on it?

#76 @SergeyBiryukov
3 months ago

In 50563:

Code Modernization: Correct expected data type for WP_User_Search::$page property.

This fixes erroneous parentheses placement and applies the type cast to the variable it was intended for.

Follow-up to [3864].

Props hellofromTonya, jrf, xknown.
See #51423.

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


3 months ago

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

Function: wp_privacy_generate_personal_data_export_file()

PHPStan flagged a type mismatch if false is passed into fwrite for groups JSON:
{{{php
fwrite( $file, $groups_json );
}}}

The groups JSON is assigned from wp_json_encode:
{{{php
$groups_json = wp_json_encode( $groups );
}}}

The return signatures of `wp_json_encode` are string or false:

_(string|false)_ The JSON encoded string, or false if it cannot be encoded.

### Is Passing false a problem?
See it in action here https://3v4l.org/bpmqG

  • No errors, warnings, or notices happen in PHP 5.6 to 8.0.3
  • Generates invalid JSON as false writes nothing into the file:

{{{json
"{Personal Data Export for tester@…:}"
}}}

This is the current behavior 👆, where generating invalid JSON is a bug.

### What can cause JSON encoding to fail?
json_encode returns false when:

  • attempting to encode a non-UTF-8 string
  • a resource is given

WordPress automatically remedies the encoding with _wp_json_sanity_check where it converts string(s) to UTF-8.

A resource could exist an element of the groups _when_ the post meta data is filtered. Using this test script reproduces the failed JSON encoding and invalid JSON report.

#80 follow-up: @hellofromTonya
3 months ago

  • Keywords has-unit-tests commit added

@SergeyBiryukov PR 758 is for ready for commit. @jrf and I have reviewed and tested it. She's approved it.

tl;dr

  • Function: wp_privacy_generate_personal_data_export_file()
  • Problem: When/if the '_export_data_grouped' post meta exists but is not an array, a PHP Warning (< 8)/Error (8+) is thrown for array_merge argument 2 type mismatch.
  • Fix: _doing_it_wrong and sets vars to retain current behavior
  • Also refactors tests to
    • Reduce redundant code
    • Switch to data provider
    • Test on the full HTML output instead of select pieces of the output
    • Expands unhappy path coverage

#81 @SergeyBiryukov
3 months ago

In 50613:

Code Modernization: Check if the _export_data_grouped post meta is an array when generating a personal data export file.

This avoids a fatal error on PHP 8 in wp_privacy_generate_personal_data_export_file() if the _export_data_grouped post meta exists but is not an array.

Additionally, refactor unit tests for the function to:

  • Reduce redundant code
  • Switch to data provider
  • Test on the full HTML output instead of select pieces of the output
  • Expand unhappy path coverage

Follow-up to [43012], [44786], [47146], [47278].

Props hellofromTonya, jrf, xknown.
See #51423.

#82 in reply to: ↑ 80 ; follow-up: @SergeyBiryukov
3 months ago

Replying to hellofromTonya:

PR 758 is for ready for commit.

This looks great, thanks for the test improvements!

Would it be possible to also add documentation for the ::setup_export_contents_test() method?

@hellofromTonya
3 months ago

Comments Tests_Privacy_WpPrivacyGeneratePersonalDataExportFile::setup_export_contents_test

@hellofromTonya
3 months ago

Comments Tests_Privacy_WpPrivacyGeneratePersonalDataExportFile::setup_export_contents_test

#84 in reply to: ↑ 82 @hellofromTonya
3 months ago

Replying to SergeyBiryukov:

Would it be possible to also add documentation for the ::setup_export_contents_test() method?

Great suggestion Sergey. Patch 51423-setup_export_contents_test.diff documents it.

#85 @SergeyBiryukov
3 months ago

In 50616:

Docs: Add documentation for the ::setup_export_contents_test method used in personal data export tests.

Follow-up to [50613].

Props hellofromTonya.
See #51423.

#86 @hellofromTonya
3 months ago

  • Keywords commit removed

Removing commit as commit ready PRs/patches are now committed.

#88 @prbot
2 months ago

hellofromtonya commented on PR #736:

Moved creating the test class for get_bookmark() to PR #1180.

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


2 months ago

#90 follow-up: @hellofromTonya
2 months ago

  • Keywords commit added

Marking PR 1100 ready for commit. cc @SergeyBiryukov.

#91 in reply to: ↑ 90 ; follow-up: @SergeyBiryukov
2 months ago

Replying to hellofromTonya:

Marking PR 1100 ready for commit. cc @SergeyBiryukov.

Thanks for the PR!

  • I don't see how get_site_option( 'limited_email_domains' ) can return null in normal workflow, so it seems a bit random to specifically guard against null here. Even if the option does not exist, it returns false. So it looks like the only way for that to happen is if someone specifically filters the value to null. Instead of targeting null, I think the existing check for a truthy value would be enough here.
  • That said, I agree with some other points of the PR, we can check for is_array() before calling implode(). We should also do it consistently for the banned_email_domains and illegal_names options, though.
  • The str_replace() call was added in mu:1285 / mu:#426 when converting limited_email_domains from an input field to a textarea. This would benefit from a comment to avoid any future confusion of why it is applied for limited_email_domains, but not for banned_email_domains.

So something like 51423.wp-admin-network-settings.diff would be my suggestion here.

#92 in reply to: ↑ 91 ; follow-up: @hellofromTonya
2 months ago

Replying to SergeyBiryukov:

  • I don't see how get_site_option( 'limited_email_domains' ) can return null in normal workflow, so it seems a bit random to specifically guard against null here. Even if the option does not exist, it returns false. So it looks like the only way for that to happen is if someone specifically filters the value to null. Instead of targeting null, I think the existing check for a truthy value would be enough here.

Checking for truthy would result in different value if the the returned filtered option was a string 0 or 0.0. See it in action here. I know that's a very very very far edge case. But here we were seeking to not change the existing behavior for backwards compatibility for any 3rd parties that are filtering the option.

Why guard for null?

In PHP 8.1, it will generate a deprecation notice. But in less than PHP 8.1, all falsey values will work with str_replace as shown here.

The PR could guard against the falsey values except for string 0 and 0.0. However, it might be confusing why. So we opted to guard specifically to prevent the future PHP 8.1 deprecation notice.

@SergeyBiryukov what do you think?

#93 in reply to: ↑ 92 ; follow-up: @SergeyBiryukov
2 months ago

Replying to hellofromTonya:

Checking for truthy would result in different value if the the returned filtered option was a string 0 or 0.0. See it in action here. I know that's a very very very far edge case. But here we were seeking to not change the existing behavior for backwards compatibility for any 3rd parties that are filtering the option.

Thanks for the additional details! While I understand the intention to keep backward compatibility here, I think we should consider the context when making these changes and, if possible, write code that accounts for the intended values without being extra specific for edge cases or particular PHP versions.

We already have a truthy check here, so I think we can just move the str_replace() call into it.

My thinking when reading the current PR includes: "Checking for null? OK, but why would that value ever be null? Is that normal or some kind of back-compat? Do we need to account for that in other instances where it's used?" This kind of checks for specific edge cases could create confusion later and seem a bit hard to maintain the long run.

Looking at this again, I'd like to make adjustments to my previous patch. Since we mostly care about taking an array of values here and converting it to a string, I think we can do exactly that, see 51423.wp-admin-network-settings.2.diff.

Here's the current code: https://3v4l.org/kmiZo
Here's the proposed code: https://3v4l.org/VTVcF

As you can see, the results almost exactly the same, except for an empty array, where the former code inadvertently passes an array instead of a string further down, so the proposed code fixes that as well.

#94 in reply to: ↑ 93 ; follow-up: @hellofromTonya
2 months ago

Replying to SergeyBiryukov:

My thinking when reading the current PR includes: "Checking for null? OK, but why would that value ever be null? Is that normal or some kind of back-compat? Do we need to account for that in other instances where it's used?" This kind of checks for specific edge cases could create confusion later and seem a bit hard to maintain the long run.

Good point. Down the road, this could be confusing. As I read it now, it does cause me to stop when reading the code. Even with the comment, it causes one to stop and wonder.

Looking at this again, I'd like to make adjustments to my previous patch. Since we mostly care about taking an array of values here and converting it to a string, I think we can do exactly that, see 51423.wp-admin-network-settings.2.diff​.

Interesting approach. It does retain the values. However, it's less performant, i.e. when falsey/empty does unnecessary type casting and replace/implode.

While I understand the intention to keep backward compatibility here, I think we should consider the context when making these changes and, if possible, write code that accounts for the intended values without being extra specific for edge cases or particular PHP versions.

How can the code guard and be robust, stable, and performant?

If the edge case values are removed and the code guards itself while staying in context, it could check for empty() in the first conditional guard and skip the prep code altogether. Edgy case falsey values are converted into and empty string. In this context, that's okay as a string 0 is not an email domain.

PR is updated with this guarding pattern for the 3 instances.

#95 in reply to: ↑ 94 @SergeyBiryukov
8 weeks ago

Replying to hellofromTonya:

How can the code guard and be robust, stable, and performant?

If the edge case values are removed and the code guards itself while staying in context, it could check for empty() in the first conditional guard and skip the prep code altogether. Edgy case falsey values are converted into and empty string. In this context, that's okay as a string 0 is not an email domain.

PR is updated with this guarding pattern for the 3 instances.

That looks better to me, thanks! As demonstrated by 51423.type-casting-vs-empty-check-test.php over a million iterations, it is indeed about three times more performant than unnecessary type casting:

type casting: 1.03507 seconds
empty() & is_array() checks: 0.34766 seconds

However, one other consideration I have when making this kind of changes is whether the code can also be as concise as possible while remaining easy to understand and without a significant performance degradation.

Since it's a relatively rarely accessed settings page and the value is only displayed once per page load and does not affect performance in a noticeable way, I think the performance difference is negligible here, so I would prefer a more compact code over more verbose (1 line vs. 6 lines).

What do you think? If you feel strongly the performance difference, we can go with the empty() and is_array() checks as you suggest. Otherwise, keeping the type casting and just addressing the initial str_replace() issue seems like a good compromise.

#96 @hellofromTonya
8 weeks ago

@SergeyBiryukov Great job running the performance benchmarks on the 2 approaches!

What do I think?

Type casting code: it's more concise with less lines of code, but IMO less understandable for what happens and why with different values or lack of value.

Why? When I read it, my first thought is: why is it type casting and doing the processing when there are no emails to be processed.

I'd vote for the empty/is_array version for quick understanding, eliminating unnecessary processing, and clear showing the processing paths for different values (which helps with building happy and unhappy path testing scenarios).

Last edited 8 weeks ago by hellofromTonya (previous) (diff)

#97 @hellofromTonya
8 weeks ago

PR 722 is ready for commit. cc @SergeyBiryukov

File: wp-admin/setup-config.php

  • Fixes a fwrite handle type mismatch which happens if fopen fails, i.e. returns false
  • Adds error messaging for it


#98 @SergeyBiryukov
8 weeks ago

In 50772:

Code Modernization: Bring consistency to preparing some fields on Network Settings screen:

  • illegal_names
  • limited_email_domains
  • banned_email_domains

If any of these options have a falsey value, treat it as an empty string. This addresses a PHP 8.1+ deprecation notice when passing a null value to str_replace().

Additionally, avoid unnecessary type casting for better performance.

Props hellofromTonya, jrf, SergeyBiryukov.
See #51423.

#100 @peterwilsoncc
8 weeks ago

In 50775:

Upgrade/Install: Prevent possible type errors during installation.

Prevent a TypeError from occurring during installation if wp-config.php is not writable. In PHP 8.0 this can cause a fatal error, in earlier versions of PHP a warning would be thrown.

Account for a change in type returned by fopen() coming in a future version of PHP. Minor coding standards fixes in the /wp-admin/setup-config.php file.

Props xknown, hellofromTonya, jrf, peterwilsoncc.
See #51423.

#103 @hellofromTonya
7 days ago

  • Keywords has-patch has-unit-tests commit removed
  • Milestone changed from 5.8 to 5.9

Today is 5.8 Beta 1. The remaining work in this ticket will be completed in 5.9. Removing keywords as there are no PRs to commit at this time.

Note: See TracTickets for help on using tickets.