Make WordPress Core

Opened 11 months ago

Closed 4 weeks ago

Last modified 4 weeks ago

#59195 closed defect (bug) (fixed)

deprecation notice triggered in post.php by passing null to parameter #2 in preg_match()

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

Description

I've trying to force myself to use PHP 8.2 to get our plugins into shape, but am still running into a deprecation notice in post.php when viewing the Media Library in list mode:

PHP Deprecated: preg_match(): Passing null to parameter #2 ($subject) of type string is deprecated in /sites/test.example.com/files/wp-includes/post.php on line 3298

Checking if $real is true before preg_match() is run seems to fix this particular one, but maybe to be safe an is_string( $real ) should be run also?

Change History (22)

This ticket was mentioned in PR #5083 on WordPress/wordpress-develop by nosilver4u.


11 months ago
#1

  • Keywords has-patch added

Prevent passing null to param #2 of preg_match()

Since param #2 of preg_match() needs to be a string, this checks to make sure $real is a string, and that $real is not empty, since it's a waste of CPU to run preg_match() on an empty string/value.

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

#2 @nosilver4u
11 months ago

  • Keywords needs-testing added

Submitted a pull request, let me know what you think.

#3 @jrf
11 months ago

@nosilver4u Thanks for opening this ticket.

The only way this deprecation should be able to show is if the input data passed to the wp_match_mime_types() function is invalid, i.e. either null was passed to $real_mime_types or $real_mime_types is an array with one or more null values, while the function expects a string or an array of strings.

In other words, this should not be fixed in this function. This needs a backtrace to see where the incorrect data is coming from and should be fixed at the source of the incorrect data.

#4 follow-up: @jrf
11 months ago

  • Keywords php81 added

#5 @nosilver4u
11 months ago

@jrf I agree that it should be fixed at the source, but there are at least 10 places where wp_match_mime_types() is used in core. And since it could also be used by plugins/themes, wp_match_mime_types() should also verify the data it receives, right?

#6 in reply to: ↑ 4 @nosilver4u
11 months ago

Replying to jrf:
I should have also mentioned, this was happening with zero plugins active, and the Twenty Twenty theme active, but I'll see if I can get a backtrace to pin down the culprit.

#7 @nosilver4u
11 months ago

Here's the trace:

wp-includes/post.php:3298
preg_match()
wp-includes/post.php:3298
wp_match_mime_types()
wp-admin/includes/class-wp-media-list-table.php:138
WP_Media_List_Table->get_views()
wp-admin/includes/class-wp-media-list-table.php:273
WP_Media_List_Table->views()
wp-admin/upload.php:435

I had done some debugging previously, and $real_mime_types was an array with multiple values, with one of them set to null.

#8 @ironprogrammer
11 months ago

  • Keywords changes-requested added

Per comment:3, flagging with changes-requested. The current PR is subject to update(s) following additional research tracking down where invalid data is being passed to wp_match_mime_types().

#9 @nosilver4u
5 months ago

I finally figured out where this was coming from. I didn't do a regular "trace", but here's the code path:
wp_match_mime_types() in wp-admin/includes/post.php - 3376
called by WP_Media_List_Table->get_views()
which uses the global $avail_post_mime_types, populated by
wp_edit_attachments_query() called in WP_Media_List_Table->prepare_items().
wp_edit_attachments_query() is in wp-admin/includes/posts.php and runs this:
$avail_post_mime_types = get_available_post_mime_types( 'attachment' );

The query in get_available_post_mime_types() returns all the distinct post_mime_type values from $wpdb->posts. And in my case, there were a pile of PDF files with no post_mime_type defined.

I don't know how that happened, but a bunch of them were from 2018/2016 (I already "fixed" them, but could probably dig up a backup if needed).
So, any attachments with an empty post_mime_type column will trigger this bug, as you'll end up with an empty item in the global $avail_post_mime_types. Theoretically, it could be fixed by altering the SQL, but I'm not sure what the best approach is here.

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


3 months ago

#11 @antpb
3 months ago

  • Keywords needs-unit-tests needs-patch added; has-patch needs-testing removed

Thanks for the report! This ticket was discussed in the recent Media component meeting and we think there are two options to be explored:

1) We omit null values from get_available_post_mime_types()
2) It is typically recommended to use a blank string instead of null in these cases so maybe we can just set the value of a no-mime to '' which should act the same as null previously

#12 @nosilver4u
3 months ago

With #2, do you mean converting the null values within get_available_post_mime_types() to ''? Otherwise, once null values are in the db, I think WP needs to be able to handle the null values without issues, and then #1 would make the most sense.

Either way, I would lean towards #1, as it doesn't seem particularly useful to return an empty mime type from get_available_post_mime_types(). That is, unless there is some code that allows querying for the attachments with an empty mime?
In the media library, I'm only seeing filters for known upload types, so an empty type certainly isn't helpful there. Oddly, I get Spreadsheets in grid view, but not in list view, that's unexpected...

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


3 months ago
#13

  • Keywords has-patch has-unit-tests added; needs-unit-tests needs-patch removed

Removes nulls from returned available post MIME types, and adds unit test coverage for get_available_post_mime_types() and pre_get_available_post_mime_types filter.

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

#14 @ironprogrammer
3 months ago

  • Keywords needs-testing added; changes-requested removed

I agree, @nosilver4u, that nulls and blank MIME types are unhelpful return values here. Thanks for digging deeper into what was happening!

I've pushed up a PR that tries to address the first option noted in comment:11, and adds test coverage to this function and the relatively new pre filter (added in 6.4).

Removing changes-requested for clarity in case it gets re-added for this new PR 😅.

#15 @ironprogrammer
3 months ago

  • Keywords has-testing-info added
  • Milestone changed from Awaiting Review to 6.6

Testing Instructions

I added the following to an mu plugin to force a null into the available post mime types:

add_filter( 'pre_get_available_post_mime_types', 'override_post_mime_types' );
function override_post_mime_types( $type ) {
    return array( 'image/jpeg', null, 'image/png' );
}

You can alternatively clear the post_mime_type from an attachment in the DB, more closely matching comment:9.

Steps to Reproduce

  1. Set up your environment to use PHP 8.1+.
  2. Implement the above mu plugin, OR find an attachment in wp_posts and clear its post_mime_type value (keep track of what you changed here in case you need to set it back).

Expected Results

When reproducing the bug:

  • ❌ Load the Media page in your environment, observe the logged deprecation notices identified in the report.
  • ❌ [Optional] From the console, run wp eval "print_r( get_available_post_mime_types( 'attachment' ) );" or use wp shell to call get_available_post_mime_types( 'attachment' ); directly. Observe that the returned array contains a null (blank).

When testing a patch to validate it works as expected:

  • ✅ Load the Media page. The deprecation notices should no longer be present.
  • ✅ [Optional] From the wp console commands, the returned array should not contain nulls (blanks).

#16 @antonvlasenko
3 months ago

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/6443

### Environment

  • WordPress: 6.6-alpha-57778-src
  • PHP: 8.3.3
  • Server: Apache/2.4.57 (Unix) PHP/8.3.3
  • Database: mysqli (Server: 5.7.43 / Client: mysqlnd 8.3.3)
  • Browser: Safari 17.4.1 (macOS)
  • Theme: Twenty Twenty-Four 1.1
  • MU-Plugins: None activated
  • Plugins:
    • JSON Basic Authentication 0.1
    • WordPress Beta Tester 3.5.3

Actual Results

  • ✅ No deprecation errors triggered on the Media page;
  • ✅ No empty values returned when running the wp eval "print_r( get_available_post_mime_types( 'attachment' ) );" command.

Additional Notes

N/A

Supplemental Artifacts

https://cldup.com/fNCk3_1WGT.png
https://cldup.com/R6IwV8wr63.png
https://cldup.com/DTQkgN-6sX.png

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


6 weeks ago

#18 @rajinsharwar
5 weeks ago

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/6443

### Environment

  • WordPress: 6.6-beta2-58392-src
  • PHP: 8.3.0
  • Server: NGINX
  • Browser: Chrome
  • Theme: Twenty Twenty-Three

Actual Results

  • ✅ The deprecation warnings don't seem to trigger when visiting the Media page;
  • ✅ After running wp eval "print_r( get_available_post_mime_types( 'attachment' ) );" command, no empty element found in the array.

#19 @hellofromTonya
4 weeks ago

  • Keywords commit added; needs-testing removed
  • Owner set to hellofromTonya
  • Status changed from new to reviewing

Patch: https://github.com/WordPress/wordpress-develop/pull/6443

I agree the root cause is get_available_post_mime_types() returning a null element. This function is documented to return an array of string MIME types. It should not be nullable. And an empty element is not a valid MIME type.

The patch addresses the root cause of the deprecation notice ✅, as confirmed by multiple test reports and my own testing.

Marking the patch for commit and will commit shortly for 6.6 Beta 3.

#20 @hellofromTonya
4 weeks ago

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

In 58437:

Code Modernization: Fix non-nullable deprecation in get_available_post_mime_types().

Fixes a PHP 8.1 and above "null to non-nullable" deprecation notice in get_available_post_mime_types():

Deprecated: preg_match(): Passing null to parameter #2 ($subject) of type string is deprecated in ./wp-includes/post.php on line 3395

This function is documented to:

  • Return An array of MIME types.
  • as an array of strings, i.e. string[].

A null or empty element within the returned array is not a valid MIME type. If a null exists in the returned array, it is the root cause of PHP throwing the deprecation notice.

This commit removes the null and empty elements from the returned array of MIME types. It also adds a unit test.

Follow-up to [56623], [56452].

Props nosilver4u, jrf, ironprogrammer, antpb, antonvlasenko, rajinsharwar, hellofromTonya.
Fixes #59195.

#22 @nosilver4u
4 weeks ago

Thanks for getting this sorted everyone!

Note: See TracTickets for help on using tickets.