Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecated PHP Errors on Site Kit with PHP 8.1 #5110

Closed
wpdarren opened this issue Apr 19, 2022 · 11 comments
Closed

Deprecated PHP Errors on Site Kit with PHP 8.1 #5110

wpdarren opened this issue Apr 19, 2022 · 11 comments
Labels
Good First Issue Good first issue for new engineers P1 Medium priority PHP Rollover Issues which role over to the next sprint Type: Bug Something isn't working

Comments

@wpdarren
Copy link
Collaborator

wpdarren commented Apr 19, 2022

Bug Description

On a test site set up on PHP 8.1. When on any Site Kit page, within Query Monitor there are a large number Deprecated PHP Errors. Some are for WordPress Core but most are related to Site Kit. We have completed a QA of Site Kit on this PHP version and no issues appeared in the console or front end.

Creating this ticket so we can look at any of the errors and if they need fixing to avoid issues in the future.

  • Constant FILTER_SANITIZE_STRING is deprecated
  • filter_input(): Passing null to parameter #4 ($options) of type array|int is deprecated
  • strpos(): Passing null to parameter #1 ($haystack) of type string is deprecated
  • str_replace(): Passing null to parameter #3 ($subject) of type array|string is deprecated
  • rtrim(): Passing null to parameter #1 ($string) of type string is deprecated
  • Constant FILTER_SANITIZE_STRING is deprecated

image.png

A more comprehensive list of deprecation notices
PHP Deprecated:  Constant FILTER_SANITIZE_STRING is deprecated in /.../wp-content/plugins/google-site-kit/includes/Core/Admin/Screens.php on line 370
PHP Deprecated:  Constant FILTER_SANITIZE_STRING is deprecated in /.../wp-content/plugins/google-site-kit/includes/Core/Admin/Standalone.php on line 95
PHP Deprecated:  Constant FILTER_SANITIZE_STRING is deprecated in /.../wp-content/plugins/google-site-kit/includes/Core/Assets/Assets.php on line 838
PHP Deprecated:  Constant FILTER_SANITIZE_STRING is deprecated in /.../wp-content/plugins/google-site-kit/includes/Core/Authentication/Authentication.php on line 306
PHP Deprecated:  filter_input(): Passing null to parameter #4 ($options) of type array|int is deprecated in /.../wp-content/plugins/google-site-kit/includes/Core/Util/Input.php on line 64
PHP Deprecated:  preg_replace_callback(): Passing null to parameter #3 ($subject) of type array|string is deprecated in /.../wp-content/plugins/google-site-kit/third-party/guzzlehttp/guzzle/src/Url.php on line 465
PHP Deprecated:  Return type of Google\Site_Kit_Dependencies\GuzzleHttp\Collection::count() should either be compatible with Countable::count(): int, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /.../wp-content/plugins/google-site-kit/third-party/guzzlehttp/guzzle/src/HasDataTrait.php on line 37
PHP Deprecated:  Return type of Google\Site_Kit_Dependencies\GuzzleHttp\Collection::getIterator() should either be compatible with IteratorAggregate::getIterator(): Traversable, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /.../wp-content/plugins/google-site-kit/third-party/guzzlehttp/guzzle/src/HasDataTrait.php on line 13
PHP Deprecated:  Return type of Google\Site_Kit_Dependencies\GuzzleHttp\Collection::offsetExists($offset) should either be compatible with ArrayAccess::offsetExists(mixed $offset): bool, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /.../wp-content/plugins/google-site-kit/third-party/guzzlehttp/guzzle/src/HasDataTrait.php on line 25
PHP Deprecated:  Return type of Google\Site_Kit_Dependencies\GuzzleHttp\Collection::offsetGet($offset) should either be compatible with ArrayAccess::offsetGet(mixed $offset): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /.../wp-content/plugins/google-site-kit/third-party/guzzlehttp/guzzle/src/HasDataTrait.php on line 17
PHP Deprecated:  Return type of Google\Site_Kit_Dependencies\GuzzleHttp\Collection::offsetSet($offset, $value) should either be compatible with ArrayAccess::offsetSet(mixed $offset, mixed $value): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /.../wp-content/plugins/google-site-kit/third-party/guzzlehttp/guzzle/src/HasDataTrait.php on line 21
PHP Deprecated:  Return type of Google\Site_Kit_Dependencies\GuzzleHttp\Collection::offsetUnset($offset) should either be compatible with ArrayAccess::offsetUnset(mixed $offset): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /.../wp-content/plugins/google-site-kit/third-party/guzzlehttp/guzzle/src/HasDataTrait.php on line 29
PHP Deprecated:  Return type of Google\Site_Kit_Dependencies\GuzzleHttp\Ring\Future\CompletedFutureArray::count() should either be compatible with Countable::count(): int, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /.../wp-content/plugins/google-site-kit/third-party/guzzlehttp/ringphp/src/Future/CompletedFutureArray.php on line 30
PHP Deprecated:  Return type of Google\Site_Kit_Dependencies\GuzzleHttp\Ring\Future\CompletedFutureArray::getIterator() should either be compatible with IteratorAggregate::getIterator(): Traversable, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /.../wp-content/plugins/google-site-kit/third-party/guzzlehttp/ringphp/src/Future/CompletedFutureArray.php on line 34
PHP Deprecated:  Return type of Google\Site_Kit_Dependencies\GuzzleHttp\Ring\Future\CompletedFutureArray::offsetExists($offset) should either be compatible with ArrayAccess::offsetExists(mixed $offset): bool, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /.../wp-content/plugins/google-site-kit/third-party/guzzlehttp/ringphp/src/Future/CompletedFutureArray.php on line 14
PHP Deprecated:  Return type of Google\Site_Kit_Dependencies\GuzzleHttp\Ring\Future\CompletedFutureArray::offsetGet($offset) should either be compatible with ArrayAccess::offsetGet(mixed $offset): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /.../wp-content/plugins/google-site-kit/third-party/guzzlehttp/ringphp/src/Future/CompletedFutureArray.php on line 18
PHP Deprecated:  Return type of Google\Site_Kit_Dependencies\GuzzleHttp\Ring\Future\CompletedFutureArray::offsetSet($offset, $value) should either be compatible with ArrayAccess::offsetSet(mixed $offset, mixed $value): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /.../wp-content/plugins/google-site-kit/third-party/guzzlehttp/ringphp/src/Future/CompletedFutureArray.php on line 22
PHP Deprecated:  Return type of Google\Site_Kit_Dependencies\GuzzleHttp\Ring\Future\CompletedFutureArray::offsetUnset($offset) should either be compatible with ArrayAccess::offsetUnset(mixed $offset): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /.../wp-content/plugins/google-site-kit/third-party/guzzlehttp/ringphp/src/Future/CompletedFutureArray.php on line 26
PHP Deprecated:  Return type of Google\Site_Kit\Core\REST_API\Data_Request::offsetExists($key) should either be compatible with ArrayAccess::offsetExists(mixed $offset): bool, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /.../wp-content/plugins/google-site-kit/includes/Core/REST_API/Data_Request.php on line 124
PHP Deprecated:  Return type of Google\Site_Kit\Core\REST_API\Data_Request::offsetGet($key) should either be compatible with ArrayAccess::offsetGet(mixed $offset): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /.../wp-content/plugins/google-site-kit/includes/Core/REST_API/Data_Request.php on line 135
PHP Deprecated:  Return type of Google\Site_Kit\Core\REST_API\Data_Request::offsetSet($key, $value) should either be compatible with ArrayAccess::offsetSet(mixed $offset, mixed $value): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /.../wp-content/plugins/google-site-kit/includes/Core/REST_API/Data_Request.php on line 149
PHP Deprecated:  Return type of Google\Site_Kit\Core\REST_API\Data_Request::offsetUnset($key) should either be compatible with ArrayAccess::offsetUnset(mixed $offset): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /.../wp-content/plugins/google-site-kit/includes/Core/REST_API/Data_Request.php on line 158
PHP Deprecated:  stripos(): Passing null to parameter #1 ($haystack) of type string is deprecated in /.../wp-content/plugins/google-site-kit/includes/Core/REST_API/REST_Routes.php on line 104
PHP Deprecated:  strlen(): Passing null to parameter #1 ($string) of type string is deprecated in /.../wp-content/plugins/google-site-kit/third-party/guzzlehttp/guzzle/src/Url.php on line 39
PHP Deprecated:  strtotime(): Passing null to parameter #1 ($datetime) of type string is deprecated in /.../wp-content/plugins/google-site-kit/includes/Modules/Analytics.php on line 587

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • No PHP deprecation notices should be raised from 1st party Site Kit sources (src/tests/etc)
  • Any remaining deprecation notices should only be from dependencies which should have new issues opened for upgrading them (assuming a newer, compliant version exists)
    • If no newer version exists which is compatible with PHP 8.1, this should be called out in the IB

Implementation Brief

  • Replace the use of the FILTER_SANITIZE_STRING filter by htmlspecialchars.
  • Update the use of filter_input function to set the options parameter to an empty string in case it's null.
  • Update the use of strtotime and stripos functions to have their parameters as an empty string in case it's null.
  • Most of the third party warnings come from Guzzle where we already have a ticket for upgrading it: Update to Guzzle 6+ #1146

Test Coverage

QA Brief

Changelog entry

  • Fix various PHP deprecation notices on PHP 8.1.
@wpdarren wpdarren added the Type: Bug Something isn't working label Apr 19, 2022
@glagonikas
Copy link

As a minimun, those deprecation notices should be fixed before PHP 8 goes to "security fixes only" (https://www.php.net/supported-versions.php), which is due to happen in November 2022, so about 5 months from now.

Websites should aim to be on the latest version of PHP that's actively supported, but bugs like those prevent many store owners from upgrading.

It would be good if you could make this a priority task.

@aaemnnosttv aaemnnosttv added P1 Medium priority PHP labels Aug 2, 2022
@asvinb asvinb assigned asvinb and unassigned asvinb Sep 22, 2022
@asvinb asvinb added the Good First Issue Good first issue for new engineers label Sep 22, 2022
@eugene-manuilov eugene-manuilov self-assigned this Sep 23, 2022
@eugene-manuilov
Copy link
Collaborator

IB ✔️

@eugene-manuilov eugene-manuilov removed their assignment Sep 23, 2022
@kuasha420 kuasha420 self-assigned this Oct 2, 2022
@kuasha420
Copy link
Contributor

@asvinb @aaemnnosttv @eugene-manuilov I am working on this and confused about a few things.

  1. The IB suggests using '' as a $option replacing null for filter_input but it also raises similar error, the default is actually 0 which works, is it correct?
  2. The error related to strpos (1 error), str_replace (1 error) and rtrim (32 errors) originate from our plugin but are result of WordPress functions (wp_normalize_path->wp_is_stream, wp_normalize_path->str_replace and load_script_textdomain->untrailingslashit respectively). From my understanding, we are using those functions correctly and it's something that needs to be fixed on WP core. Is that assessment correct? I've added the stack trace from Query Monitor for your assessment.
  3. Finally, htmlspecialchars is not a 1-to-1 replacement for FILTER_SANITIZE_STRING but I think for our use case, use of htmlspecialchars is adequate. Is that correct?

Cheers.

strpos(): Passing null to parameter #1 ($haystack) of type string is deprecated
    wp-includes/functions.php:7012
    strpos()
    wp-includes/functions.php:7012
    wp_is_stream()
    wp-includes/functions.php:2160
    wp_normalize_path()
    wp-includes/plugin.php:768
    plugin_basename()
    wp-admin/includes/plugin.php:1405
    add_submenu_page()
    wp-content/plugins/google-site-kit/includes/Core/Admin/Screen.php:203
    Google\S\C\A\Screen->add()
    wp-content/plugins/google-site-kit/includes/Core/Admin/Screens.php:227
    Google\S\C\A\Screens->add_screen()
    Unknown location
    array_walk()
    wp-content/plugins/google-site-kit/includes/Core/Admin/Screens.php:216
    Google\S\C\A\Screens->add_screens()
    wp-content/plugins/google-site-kit/includes/Core/Admin/Screens.php:113
    Google\S\C\A\Screens->Google\S\C\A\{closure}()
    wp-includes/class-wp-hook.php:308
    do_action('admin_menu')
    wp-admin/includes/menu.php:155

str_replace(): Passing null to parameter #3 ($subject) of type array|string is deprecated
    wp-includes/functions.php:2167
    str_replace()
    wp-includes/functions.php:2167
    wp_normalize_path()
    wp-includes/plugin.php:768
    plugin_basename()
    wp-admin/includes/plugin.php:1405
    add_submenu_page()
    wp-content/plugins/google-site-kit/includes/Core/Admin/Screen.php:203
    Google\S\C\A\Screen->add()
    wp-content/plugins/google-site-kit/includes/Core/Admin/Screens.php:227
    Google\S\C\A\Screens->add_screen()
    Unknown location
    array_walk()
    wp-content/plugins/google-site-kit/includes/Core/Admin/Screens.php:216
    Google\S\C\A\Screens->add_screens()
    wp-content/plugins/google-site-kit/includes/Core/Admin/Screens.php:113
    Google\S\C\A\Screens->Google\S\C\A\{closure}()
    wp-includes/class-wp-hook.php:308
    do_action('admin_menu')
    wp-admin/includes/menu.php:155

rtrim(): Passing null to parameter #1 ($string) of type string is deprecated
    wp-includes/formatting.php:2782
    rtrim()
    wp-includes/formatting.php:2782
    untrailingslashit()
    wp-includes/l10n.php:1068
    load_script_textdomain()
    Unknown location
    Google\S\C\U\BC_Functions::__callStatic()
    wp-content/plugins/google-site-kit/includes/Core/Assets/Script.php:112
    Google\S\C\A\Script->set_locale_data()
    wp-content/plugins/google-site-kit/includes/Core/Assets/Script.php:93
    Google\S\C\A\Script->register()
    wp-content/plugins/google-site-kit/includes/Core/Assets/Assets.php:252
    Google\S\C\A\Assets->register_assets()
    wp-content/plugins/google-site-kit/includes/Core/Assets/Assets.php:90
    Google\S\C\A\Assets->Google\S\C\A\{closure}()
    wp-includes/class-wp-hook.php:308
    do_action('admin_enqueue_scripts')
    wp-admin/admin-header.php:118

@eugene-manuilov
Copy link
Collaborator

2. The error related to strpos (1 error), str_replace (1 error) and rtrim (32 errors) originate from our plugin but are result of WordPress functions (wp_normalize_path->wp_is_stream, wp_normalize_path->str_replace and load_script_textdomain->untrailingslashit respectively). From my understanding, we are using those functions correctly and it's something that needs to be fixed on WP core. Is that assessment correct? I've added the stack trace from Query Monitor for your assessment.

Looks like this happens because we call the add_submenu_page function with NULL for the parent slug parameter which is wrong because the parent slug is required for this function. We should probably call the add_menu_page function when our screen has no parent. Could you please take a look and try to find a workaround that will fix this problem and preserves the current behaviour?

3. Finally, htmlspecialchars is not a 1-to-1 replacement for FILTER_SANITIZE_STRING but I think for our use case, use of htmlspecialchars is adequate. Is that correct?

That is what we need to figure out. Try to see where we use that filter. If it has been used for sanitization purposes, then using the htmlspecialchars function is okay for us.

  1. The IB suggests using '' as a $option replacing null for filter_input but it also raises similar error, the default is actually 0 which works, is it correct?

Yes, that's fine. Let's use 0 then.

@kuasha420
Copy link
Contributor

kuasha420 commented Oct 12, 2022

Thank you for the investigation and recommendations,@eugene-manuilov !

It appears that we intentionally call add_submenu_page with null as parent slug to hide it from the Sidebar menu (ie. User Input Page) while still registering the page. It's a well known hack to achieve the goal. There's other way to do it, however I'm not sure if it is better solved here or in a separate issue. Any advise?

The rtrim errors are also from the load_script_textdomain function and I don't think we can fix it here as we're not doing anything silly here.

@eugene-manuilov
Copy link
Collaborator

It appears that we intentionally call add_submenu_page with null as parent slug to hide it from the Sidebar menu (ie. User Input Page) while still registering the page. It's a well known hack to achieve the goal. There's other way to do it, however I'm not sure if it is better solved here or in a separate issue. Any advise?

@kuasha420, yes, i think a separate ticket will be better because we will need to figure out a better way of adding hidden pages. Could you please create a new ticket?

The rtrim errors are also from the load_script_textdomain function and I don't think we can fix it here as we're not doing anything silly here.

Ok, just leave a comment about it in your PR.

@kuasha420 kuasha420 removed their assignment Oct 13, 2022
@asvinb asvinb assigned asvinb and kuasha420 and unassigned asvinb Oct 13, 2022
@kuasha420 kuasha420 assigned asvinb and unassigned kuasha420 Oct 14, 2022
@asvinb asvinb assigned kuasha420 and unassigned asvinb Oct 18, 2022
@FlicHollis FlicHollis added the Rollover Issues which role over to the next sprint label Oct 23, 2022
@asvinb asvinb assigned aaemnnosttv and unassigned asvinb Nov 2, 2022
@aaemnnosttv aaemnnosttv removed their assignment Nov 10, 2022
@kuasha420 kuasha420 assigned techanvil and unassigned kuasha420 Dec 16, 2022
techanvil added a commit that referenced this issue Dec 16, 2022
@techanvil techanvil removed their assignment Dec 16, 2022
@mohitwp mohitwp self-assigned this Dec 19, 2022
@mohitwp
Copy link
Collaborator

mohitwp commented Dec 20, 2022

QA Update ❌

@kuasha420

  • Verified on dev.
  • Tested using both PHP version 8.1 and 8.2.
  • I'm getting 4 Deprecated PHP errors.
  • Also, SK widgets showing JSON errors.
  • If query monitor is active, then user is not able to Set up Site kit and getting Rest API error on Splash screen.
  • For JSON errors, we have a separate issue All widgets showing error state #6099.
  • Query Monitor version - 3.10.1

image

image

@mohitwp mohitwp assigned kuasha420 and unassigned mohitwp Dec 20, 2022
@kuasha420
Copy link
Contributor

kuasha420 commented Dec 20, 2022

Hi @mohitwp Can you give screenshots of the 4 warnings you're seeing? They may be caused by the exceptions mentioned in the QAB. #5998 will address some of them.

Does the "then user is not able to Set up Site kit and getting Rest API error on Splash screen" only happen with Query Monitor Active? Does this also happen on the current release without this?

Cheers.

@mohitwp
Copy link
Collaborator

mohitwp commented Dec 20, 2022

@kuasha420

  • strpos(): Passing null to parameter #1 $haystack of type string is deprecated
  • str_replace(): Passing null to parameter #3 $subject of type array|string is deprecated
  • REST API error coming only when I'm using Query monitor plugin. I'm not getting RESI API error on splash page without query monitor with current release.

image

image

@kuasha420
Copy link
Contributor

@mohitwp These are expected and will be explored on #5998. Cheers

@kuasha420 kuasha420 assigned mohitwp and unassigned kuasha420 Dec 20, 2022
@mohitwp
Copy link
Collaborator

mohitwp commented Dec 21, 2022

QA Update ✅

image

image

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Good first issue for new engineers P1 Medium priority PHP Rollover Issues which role over to the next sprint Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

9 participants