Make WordPress Core

Opened 8 months ago

Closed 7 days ago

Last modified 7 days ago

#59818 closed defect (bug) (fixed)

[PHP 8.1] deprecated null in `explode()` for 'ping_sites' in `wp-admin/options-writing.php` with "Discourage search engines" on

Reported by: kitchin's profile kitchin Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.6 Priority: normal
Severity: normal Version:
Component: Administration Keywords: php81 has-patch has-testing-info commit
Focuses: php-compatibility Cc:

Description

When "Discourage search engines" is on, any update to Dashboard / Writing causes:
Deprecated: explode(): Passing null to parameter #2 ($string) of type string is deprecated in .../wp-includes/formatting.php

In this case, the 'ping_sites' option is not displayed, so the $_POST value is null. The deprecation warning is triggered in wp-includes/formatting.php, in sanitize_option(), "case 'ping_sites':".

Tested on today's nightly, WordPress 6.5-alpha-57063. Also affects latest release, WP 6.3.2.

Attachments (3)

59818.diff (1.2 KB) - added by SergeyBiryukov 8 months ago.
kitchin59818.diff (585 bytes) - added by kitchin 8 months ago.
First patch, minimal
kitchin59818.alt.diff (4.3 KB) - added by kitchin 8 months ago.
Alternate patch, not tested.

Download all attachments as: .zip

Change History (21)

#1 @swissspidy
8 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 6.5
  • Version trunk deleted

#2 @SergeyBiryukov
8 months ago

  • Keywords has-patch added; needs-patch removed
  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

Hi there, thanks for the ticket!

59818.diff follows the approach from [56132] / #57728.

#3 @SergeyBiryukov
8 months ago

  • Keywords php81 added

#4 follow-up: @kitchin
8 months ago

Thanks for the quick action! I have a different approach that addresses the underling issue.

The controller wp-admin/options.php uses update_option() unconditionally on all relevant elements of $allowed_options, even if they don't appear in $_POST. Luckily in the current codebase, or as part of its design, there is a filter on get_option( 'ping_sites' ), called privacy_ping_filter(), which checks:

if ( '1' === get_option( 'blog_public' ) ) {
	return $sites;
} else {
	return '';
}

So when update_option() uses get_option() to compare $value to $old_value, it sees '' === '', where explode( "\n", $value ) has cast NULL to ''. Thus no data loss.

My first patch mods $allowed_options. It does it directly, rather than using the filter, since the variable is very specific to the file it is in. Might as well make it easy to read. This patch is tested as far as updating options goes.

My alternate patch also reconciles the strictness and specificity of the evaluation of 'blog_public' across the codebase, except multisite. In other words, '1' === vs. '0' != , etc. It also makes the filter signature in wp-includes/functions.php correct. Multisite seems to have its own way of doing things. This patch is not tested.

@kitchin
8 months ago

First patch, minimal

@kitchin
8 months ago

Alternate patch, not tested.

#5 in reply to: ↑ 4 @devmuhib
6 months ago

Can you add the testing instruction please?

Replying to kitchin:

Thanks for the quick action! I have a different approach that addresses the underling issue.

The controller wp-admin/options.php uses update_option() unconditionally on all relevant elements of $allowed_options, even if they don't appear in $_POST. Luckily in the current codebase, or as part of its design, there is a filter on get_option( 'ping_sites' ), called privacy_ping_filter(), which checks:

if ( '1' === get_option( 'blog_public' ) ) {
	return $sites;
} else {
	return '';
}

So when update_option() uses get_option() to compare $value to $old_value, it sees '' === '', where explode( "\n", $value ) has cast NULL to ''. Thus no data loss.

My first patch mods $allowed_options. It does it directly, rather than using the filter, since the variable is very specific to the file it is in. Might as well make it easy to read. This patch is tested as far as updating options goes.

My alternate patch also reconciles the strictness and specificity of the evaluation of 'blog_public' across the codebase, except multisite. In other words, '1' === vs. '0' != , etc. It also makes the filter signature in wp-includes/functions.php correct. Multisite seems to have its own way of doing things. This patch is not tested.

#6 @kitchin
5 months ago

STR:

  1. Drop all tables. Delete all files in public_html.
  2. wget https://wordpress.org/nightly-builds/wordpress-latest.zip
  3. unzip wordpress-latest.zip
  4. Move files and upload wp-config.php, with define('WP_DEBUG', true );
  5. Visit /wp-admin to install. Check "Discourage search engines" and install.
  6. Login and go to Settings / Writing.
  7. Click Save Changes.

Depending on your PHP settings, you will see the error on the screen and/or in wp-admin/error_log.

WordPress 6.5-alpha-57320
Linux 4.18... x86_64
PHP 8.2...

#7 @swissspidy
4 months ago

  • Keywords has-testing-info added

#8 @swissspidy
4 months ago

  • Milestone changed from 6.5 to 6.6

#9 @rajinsharwar
2 months ago

Hi @kitchin, thanks for the test instructions. Your 2nd patch does seem to solve the issue and address this behavior, on the single-site installation. I didn't get a chance to test this on the multi-site today, but if someone can share if it addresses the issue, we can get this merged sooner.

#10 @devmuhib
2 months ago

I downloaded the latest version with wget https://wordpress.org/nightly-builds/wordpress-latest.zip and my version is 6.6-alpha-58025. But I could not reproduce the error. I also checked wp-admin/error_log. but not found anything error code.

wp-admin/error_log.

https://i.imgur.com/B5gycDX.jpeg

https://i.imgur.com/jVLk6sQ.jpeg

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


11 days ago

#12 @hellofromTonya
10 days ago

Reproduction Report

Environment

  • OS: macOS 14.5
  • Web Server: Nginx
  • PHP: 8.3
  • WordPress: 6.6-beta2-58411
  • Browser: Firefox 126.0.1
  • Theme: TT4
  • Active Plugins: None

Testing Steps

  1. Ran wp db reset --yes to wipe the database.
  2. Walked through the install, while checking "Discourage search engines" option.
  3. Logged in.
  4. Navigated to Settings / Writing.
  5. Clicked "Save Changes" button.
  6. Then checked the logs.

Actual Results

  • ✅ Able to reproduce after clicking the "Save Changes" button.
[14-Jun-2024 15:38:51 UTC] PHP Deprecated:  explode(): Passing null to parameter #2 ($string) of type string is deprecated in ./wp-includes/formatting.php on line 4952

#13 follow-up: @hellofromTonya
10 days ago

  • Keywords commit added

To evaluate the different patches / approaches in this ticket, I needed to first understand the relationship between 'blog_public' and 'ping_sites'.

Relationship between 'blog_public' and 'ping_sites'

Source: https://developer.wordpress.org/apis/options/#writing

ping_sites: When you publish a new post, WordPress automatically notifies the following site update services. For more about this, see Update Services. Separate multiple service URLs with line breaks. Requires blog_public to have a value of 1.
Default: ‘http://rpc.pingomatic.com/
Data type:String (possibly multi-line)

This relationship shows itself in the options-writing.php code (shown here), as the textarea#ping_sites only renders when '1' === get_option( 'blog_public' ).

Commit history for context:

  • options.php 'ping_sites' added to the options array[] via [12825].
  • options-writing.php: textarea#ping_sites added in [949].
  • options-writing.php: conditionally rendering the ping_sites based on blog_public added in [4326].

Evaluating the patches / approaches

The root cause is as @kitchin noted. kitchin59818.diff makes sense to me given the relationship between these 2 options and the purpose of the list of allowed 'writing' options.

While defensive guarding and consistency from 59818.diff also makes sense, the expected data type for the 'ping_sites' option is string (not nullable string).

So if the ping_sites option is not in the post back, then it shouldn't be added to the options list to sanitize in sanitize_option().

As I note, both patches resolve the issue, while IMO kitchin59818.diff resolves the root cause that is driving the error.

For 6.6, I'm in favor of moving forward with kitchin59818.diff.

@SergeyBiryukov any objections? What do you think?

Marking kitchin59818.diff for commit, but will wait until before 6.6 Beta 3 to give Sergey time to review.

#14 in reply to: ↑ 13 ; follow-up: @SergeyBiryukov
9 days ago

Replying to hellofromTonya:

For 6.6, I'm in favor of moving forward with kitchin59818.diff.

@SergeyBiryukov any objections? What do you think?

I agree with your assessment, thanks!

#15 in reply to: ↑ 14 @hellofromTonya
7 days ago

  • Owner changed from SergeyBiryukov to hellofromTonya
  • Status changed from accepted to assigned

Replying to SergeyBiryukov:

Replying to hellofromTonya:

For 6.6, I'm in favor of moving forward with kitchin59818.diff.

@SergeyBiryukov any objections? What do you think?

I agree with your assessment, thanks!

Awesome :) Thanks @SergeyBiryukov.

I'm preparing the commit now.

This ticket was mentioned in PR #6845 on WordPress/wordpress-develop by @hellofromTonya.


7 days ago
#16

Commit confidence check to ensure `kitchin59818.diff` passes all CI jobs before committing.

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

#17 @hellofromTonya
7 days ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 58425:

Code Modernization: Fix 'ping_sites' non-nullable deprecation.

When saving options from the Settings page, include the 'ping_sites' option in the allowed "writing" options list only when the 'blog_public' option is '1'.

Fixes a PHP 8.1 and above "null to non-nullable" deprecation notice in sanitize_option() (which happens when here as part of [22255]):

Deprecated: explode(): Passing null to parameter #2 ($string) of type string is deprecated in .../wp-includes/formatting.php

Explanation

Per the documentation, the ping_sites option requires the 'blog_public' option to have a value of '1' and must be a string data type. null is not valid for this option.

The relationship between the 2 options shows itself in the options-writing.php code (shown here and in [4326]), as the textarea#ping_sites only renders when '1' === get_option( 'blog_public' ).

What happens if 'blog_public' is not '1'?

The 'ping_sites' option will not be a field on the page. Upon saving:

  • HTTP POST ($_POST) does not include 'ping_sites'.
  • Before this commit:
    • The option's value was set to null before being passed to update_option().
    • update_option() invokes sanitize_option().
    • A null value for the 'ping_sites' case was passed to explode(), which threw a deprecation notice on PHP 8.1 and above.
  • With this commit, the 'ping_sites' option is no longer included in the allow list and thus will not be passed to update_options() > sanitize_option() > explode().

Follow-up to [22255], [12825], [4326], [949].

Props kitchin, SergeyBiryukov, swissspidy, devmuhib, rajinsharwar, hellofromTonya.
Fixes #59818.

Note: See TracTickets for help on using tickets.