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

PHP 8.1 ltrim() Compatibility Issue #7809

Open
timnolte opened this issue Nov 1, 2023 · 9 comments · May be fixed by #7810
Open

PHP 8.1 ltrim() Compatibility Issue #7809

timnolte opened this issue Nov 1, 2023 · 9 comments · May be fixed by #7810
Assignees
Labels
P1 Medium priority PHP Type: Bug Something isn't working

Comments

@timnolte
Copy link

timnolte commented Nov 1, 2023

$name = ltrim( $name, '_' );

Since the trim() call prior to the ltrim() call can return null there is no verification check to ensure that null isn't being passed to ltrim() and thus a deprecation notice is thrown.

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

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

Acceptance criteria

  • Site Kit should not raise PHP deprecation notices on PHP 8.1 due to incorrect ltrim usage.

Implementation Brief

Test Coverage

QA Brief

Changelog entry

@timnolte
Copy link
Author

timnolte commented Nov 1, 2023

I'll get a PR opened for a fix for this. I've tested it and a simple validation before the call is all that is needed.

timnolte added a commit to timnolte/site-kit-wp that referenced this issue Nov 1, 2023
* Fixes google#7809
  * Fixes an `ltrim()` deprecation warning in the Tag Manager module by adding a validation check.
@timnolte timnolte linked a pull request Nov 1, 2023 that will close this issue
18 tasks
@kuasha420 kuasha420 self-assigned this Nov 2, 2023
@kuasha420 kuasha420 added Type: Bug Something isn't working P1 Medium priority labels Nov 2, 2023
@kuasha420
Copy link
Collaborator

Thank you for raising the issue @timnolte ! Can you provide a step to reproduce this issue? Cheers!

@aaemnnosttv aaemnnosttv added the PHP label Nov 7, 2023
@mrwweb
Copy link

mrwweb commented Nov 28, 2023

I'm getting the error:

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

Environment: WordPress 6.4.1 on PHP 8.1.25. The error appears both in the dashboard and on the front-end.

Here's a full stacktrace with identifying stuff redacted:

wp-includes/formatting.php:4494
ltrim()
wp-includes/formatting.php:4494
esc_url()
wp-includes/formatting.php:4620
sanitize_url()
wp-includes/formatting.php:4602
esc_url_raw()
wp-content/plugins/google-site-kit/includes/Core/Assets/Assets.php:716
Google\S\C\A\Assets->get_inline_base_data()
wp-content/plugins/google-site-kit/includes/Core/Assets/Assets.php:356
Google\S\C\A\Assets->Google\S\C\A{closure}()
wp-content/plugins/google-site-kit/includes/Core/Assets/Assets.php:356
Google\S\C\A\Script_Data->Google\S\C\A{closure}()
wp-content/plugins/google-site-kit/includes/Core/Assets/Script_Data.php:51
Google\S\C\A\Asset->before_print()
wp-content/plugins/google-site-kit/includes/Core/Assets/Assets.php:1016
Google\S\C\A\Assets->run_before_print_callbacks()
wp-content/plugins/google-site-kit/includes/Core/Assets/Assets.php:1025
Google\S\C\A\Assets->run_before_print_callbacks()
wp-content/plugins/google-site-kit/includes/Core/Assets/Assets.php:1025
Google\S\C\A\Assets->run_before_print_callbacks()
wp-content/plugins/google-site-kit/includes/Core/Assets/Assets.php:1025
Google\S\C\A\Assets->run_before_print_callbacks()
wp-content/plugins/google-site-kit/includes/Core/Assets/Assets.php:155
Google\S\C\A\Assets->Google\S\C\A{closure}()
wp-includes/class-wp-hook.php:324
do_action('wp_print_scripts')
wp-includes/script-loader.php:2214
wp_print_head_scripts()
wp-includes/class-wp-hook.php:324
do_action('wp_head')
wp-includes/general-template.php:3052
wp_head()
wp-content/themes/{theme}/header.php:20
load_template('wp-content/themes/{theme}/header.php')
wp-includes/template.php:725
locate_template()
wp-includes/general-template.php:48
get_header()
wp-content/themes/{theme}/singular.php:15

I'll note that this doesn't seem to be exactly the same as @timnolte's fix, so it's possible this error is in a few different places.

Here is my Site Kit info from Tools > Site Health with identifying stuff removed

Version 1.113.0
PHP Version 8.1.25
WordPress Version 6.4.1
AMP Mode No
Site Status Connected through site credentials
User Status Authenticated
Verification Status Verified through file
Connected user count 1
Active Modules Site Verification, Search Console, Analytics, Analytics 4, PageSpeed Insights, Tag Manager
Recoverable Modules None
Required scopes
openid: ✅
https://www.googleapis.com/auth/userinfo.profile: ✅
https://www.googleapis.com/auth/userinfo.email: ✅
https://www.googleapis.com/auth/siteverification: ✅
https://www.googleapis.com/auth/webmasters: ✅
https://www.googleapis.com/auth/analytics.readonly: ✅
https://www.googleapis.com/auth/tagmanager.readonly: ✅
User Capabilities
googlesitekit_authenticate: ✅
googlesitekit_setup: ✅
googlesitekit_view_posts_insights: ✅
googlesitekit_view_dashboard: ✅
googlesitekit_manage_options: ✅
googlesitekit_update_plugins: ✅
googlesitekit_view_splash: ✅
googlesitekit_view_authenticated_dashboard: ✅
googlesitekit_view_wp_dashboard_widget: ✅
googlesitekit_view_admin_bar_menu: ✅
googlesitekit_view_shared_dashboard: ⭕
googlesitekit_read_shared_module_data::["search-console"]: ⭕
googlesitekit_read_shared_module_data::["analytics"]: ⭕
googlesitekit_read_shared_module_data::["analytics-4"]: ⭕
googlesitekit_read_shared_module_data::["pagespeed-insights"]: ⭕
googlesitekit_manage_module_sharing_options::["search-console"]: ✅
googlesitekit_manage_module_sharing_options::["analytics"]: ✅
googlesitekit_manage_module_sharing_options::["analytics-4"]: ✅
googlesitekit_manage_module_sharing_options::["pagespeed-insights"]: ✅
googlesitekit_delegate_module_sharing_management::["search-console"]: ✅
googlesitekit_delegate_module_sharing_management::["analytics"]: ✅
googlesitekit_delegate_module_sharing_management::["analytics-4"]: ✅
googlesitekit_delegate_module_sharing_management::["pagespeed-insights"]: ⭕
Features
adsenseSetupV2: ✅
enhancedMeasurement: ✅
ga4Reporting: ✅
gm3Components: ⭕
keyMetrics: ⭕
Search Console Shared Roles None
Search Console Management Owner
Analytics Shared Roles None
Analytics Management Owner
Analytics 4 Shared Roles None
Analytics 4 Management Owner
PageSpeed Insights Shared Roles None
PageSpeed Insights Management Any admin signed in with Google
Analytics snippet placed Yes
Analytics 4 snippet placed Yes
Tag Manager AMP container ID None
Tag Manager snippet placed Yes

@adamdunnage
Copy link
Collaborator

@mrwweb Thanks for your comment. We don't provide support via GitHub but we'd be delighted to assist you with this in the Site Kit support forum. Please can you open a new support topic and share the details you have done so here. Thanks and we look forward to helping you with this!

@timnolte
Copy link
Author

@adamdunnage this isn't in need of some additional support forum request. This is a code issue that needs to be fixed, which I already provided a PR to fix this. The output that @mrwweb provided are the details to traceback this depreciation notice. This is a PHP 8.1 compatibility issue not an end user issue.

@timnolte
Copy link
Author

@mrwweb just noticed that your stack trace does appear to call out the ltrim() in WordPress Core. Have you taken a look at the code called out in each line of the stack trace? If not I can take a look to determine if I can update my PR to include fixing this as well. I'm suspecting that this may be partly do an incomplete plugin setup, and there isn't adequate validation happening.

@mrwweb
Copy link

mrwweb commented Nov 29, 2023

@timnolte Haven't done anything else. I saw the error, found this issue, and saw the request for a stacktrace and dumped it here. Hope it's useful!

@kuasha420 kuasha420 removed their assignment Dec 18, 2023
@kevinlisota
Copy link

This issue still exists in Site Kit. I get the deprecation warning in both PHP 8.1 and 8.2 on every page load with stack trace results like originally reported.

@bethanylang
Copy link
Collaborator

@kevinlisota Yes, this issue remains open at the moment, but it's on our list to fix in a future release. Feel free to follow along here for updates. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority PHP Type: Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants