WordPress.org

Make WordPress Core

Opened 4 weeks ago

Last modified 4 weeks ago

#44416 new defect (bug)

`compact()` will throw notice for undefined variables in PHP 7.3

Reported by: desrosj Owned by:
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: General Keywords: needs-patch
Focuses: Cc:

Description (last modified by desrosj)

PHP 7.3 has released Alpha 1 version. While this is still an early development version, one of the accepted and committed changes is exposing some undefined variables.

In PHP 7.3, the compact() function has been changed to issue an E_NOTICE level error if a passed string refers to an unset variable. In previous versions of PHP, this notice was silently skipped. The full RFC can be viewed here: https://wiki.php.net/rfc/compact

While it may be early to fix PHP 7.3 compatibility issues, this one exposes variables that are potentially being handled incorrectly and should be investigated.

The main offenders appear to be:

  • wp_insert_post()
  • wp_xmlrpc_server->mw_newPost();
  • WP_Comment_Query->get_comment_ids()

To see the warnings thrown, look at the nightly PHP job on any recent build for `master`

Change History (4)

#1 @desrosj
4 weeks ago

  • Description modified (diff)

#2 @SergeyBiryukov
4 weeks ago

  • Milestone changed from Future Release to 5.0

#3 @desrosj
4 weeks ago

Dove into wp_insert_post(). Notices being thrown for the following variables not existing:

  • $filter
  • $context
  • $ID ($post_ID is used instead)
  • $file
  • $post_mime_type
  • $category
  • $comment_count
  • $ancestors
  • $page_template
  • $tags_input

The first compact() call is causing all of the issues. This is because it is passing array_keys( $postarr ) to the call, so all indexes passed to the function may not be represented in variable form. The second compact() call is not causing any issues because those variable names are manually defined in the compact() call and all exist.

The result of the first compact() call is passed to the wp_insert_post_parent filter, which attempts to prevent hierarchy loops. Core has one function that hooks into this filter, but it does not even use the result of the compact() call.

I think there are a few options here.

Make sure all documented $postarr arguments get a defined variable

This would fix the notice for all core values, but it would not fix issues where a plugin or theme is passing a custom value to $postarr. While some variables can just be moved above this first call ($post_mime_type, for example), it would also require defining new variables only for the purpose of avoiding a PHP notice, which makes me sway away from this option.

Manually define the variables to compact in the third argument of wp_insert_post_parent

This would ensure that PHP notices are never thrown (similar to the second call to compact() further down). But, it could cause a backwards compatibility issue. A value being used by a filter could start to be excluded. This is not a very well known or used filter, so communication in a Make post ahead of time could help prevent issues with this change. The filter could also be deprecated in favor of a new one. But this would not fix the notices.

#4 @ayeshrajans
4 weeks ago

Re: wp_insert_post(): This function practically extract()'s the variables off the $postarr array, only to be converted into an array right before applying the hook. Perhaps we can keep a separate array to hold these variables and avoid the compact() call altogether.

This function is a giant ~600 line function and I'm afraid to modify it this much without waking up cthulhu.

Note: See TracTickets for help on using tickets.