site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard

Published 20 hours ago • verified publisher icon google

Deprecated PHP Errors on Site Kit with PHP 8.1

Open wpdarren opened this issue 2 years ago • 6 comments

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: https://github.com/google/site-kit-wp/issues/1146

Test Coverage

QA Brief

  • Install the plugin and query monitor on a site running php 8.1.
  • Activate the plugin and observe query monitor toolbar.
  • No PHP errors or warnings should be raised from site kit in splash, dashboard, settings etc page.
    • Exception: error related to strpos and str_replace will be fixed on #5998
    • rtrip related errors are actually coming from core and we can't fix them here. See this comment.

Changelog entry

wpdarren avatar Apr 19 '22 14:04 wpdarren

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.

glagonikas avatar May 26 '22 14:05 glagonikas

IB ✔️

eugene-manuilov avatar Sep 23 '22 10:09 eugene-manuilov

@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

kuasha420 avatar Oct 05 '22 13:10 kuasha420

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.

eugene-manuilov avatar Oct 10 '22 09:10 eugene-manuilov

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.

kuasha420 avatar Oct 12 '22 01:10 kuasha420

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.

eugene-manuilov avatar Oct 12 '22 15:10 eugene-manuilov

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 https://github.com/google/site-kit-wp/issues/6099.
  • Query Monitor version - 3.10.1

image

image

mohitwp avatar Dec 20 '22 12:12 mohitwp

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.

kuasha420 avatar Dec 20 '22 12:12 kuasha420

@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

mohitwp avatar Dec 20 '22 12:12 mohitwp

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

kuasha420 avatar Dec 20 '22 15:12 kuasha420

QA Update ✅

  • Verified on dev using PHP 8.1
  • 4 Deprecated errors are currently showing under Query monitor which as per @kuasha420 is expected and will get address through https://github.com/google/site-kit-wp/issues/5998.
  • Regarding the REST API error on splash screen and JSON error on widgets, we have separate ticket -https://github.com/google/site-kit-wp/issues/6099.

image

image

image

mohitwp avatar Dec 21 '22 05:12 mohitwp