Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#44416 closed defect (bug) (fixed)

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

Reported by: desrosj's profile desrosj Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: General Keywords: php73 fixed-5.0
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`

Attachments (6)

44416.diff (4.1 KB) - added by desrosj 6 years ago.
44416-simple.diff (7.1 KB) - added by desrosj 6 years ago.
Improved patch for original approach
44416-wp_insert_post-rework.diff (31.4 KB) - added by desrosj 6 years ago.
More complex rework of wp_insert_post()
44416.2.diff (10.2 KB) - added by desrosj 5 years ago.
44416.3.diff (937 bytes) - added by desrosj 5 years ago.
44416.3.2.diff (1.3 KB) - added by desrosj 5 years ago.

Download all attachments as: .zip

Change History (35)

#1 @desrosj
6 years ago

  • Description modified (diff)

#2 @SergeyBiryukov
6 years ago

  • Milestone changed from Future Release to 5.0

#3 @desrosj
6 years 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
6 years 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.

@desrosj
6 years ago

#5 @desrosj
6 years ago

  • Keywords has-patch needs-refresh added; needs-patch removed

44416.diff ensures that there are variable declarations for each value being included in a compact() call with a few exceptions. The main remaining notice is for menu_order. menu_order is checked for isset() not empty() because it can be a 0. Fixing this notice would require more changes.

It also changes wp_insert_post() to use array_keys() on the list of defaults instead of the passed $postarr, adds ID in (the variable is $post_ID not $ID), and removes two field that are not present ($context and $filter). This shouldn't change anything and should be backwards compatible because the missing variables were not being compacted anyway.

This is a more conservative approach, but not sure that I love it. All tests are passing, but I have not checked if this is due to an absence of test cases that would catch breaking changes, or because the tests actually continue to pass.

#6 @pross
6 years ago

Same issue php 7.4 (master) tested patch works fine.

#7 @SergeyBiryukov
6 years ago

  • Milestone changed from 5.0 to 4.9.9
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

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


6 years ago

@desrosj
6 years ago

Improved patch for original approach

@desrosj
6 years ago

More complex rework of wp_insert_post()

#9 @desrosj
6 years ago

  • Keywords needs-testing added; needs-refresh removed

Here is a breakdown of the two patches I just added:

44416-simple.diff is a more complete refresh of the original approach, which changes less code and switches compact() to use array_keys() on the default list provided to wp_parse_args(), ensuring no custom keys passed cause notices.

Here is a build in Travis of this approach showing all warnings fixed except the one caused by #45018.

44416-wp_insert_post-rework.diff is a more complex rewrite of wp_insert_post() that no longer creates a variable for each individual post property (many of which were created just for the sake of being present for compact()) and instead performs escaping and sanitization and value correction to the original $postarr that is passed to the function.

Here is a build in Travis of this approach showing all warnings fixed except the one caused by #45018.

Notes:

  • Both patches apply identical fixes to all of the other files.
  • The unit tests still need to be checked to verify the code being changed in the patches has proper tests, or that the tests are still passing due to a lack of tests.

I definitely like the simpler version of the patch for a minor release, but think that the more complex patch could be considered for 5.0.

#10 @pento
6 years ago

  • Milestone changed from 4.9.9 to 5.0

#11 @desrosj
6 years ago

  • Keywords php73 added

#12 @jorbin
6 years ago

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

In 43819:

php7.3 compatibility: Fix compact throwing notices

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

This fixes all unit tested code that uses compact.

Props desrosj.
Fixes #44416.

#13 @jorbin
6 years ago

  • Keywords fixed-5.0 added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Thanks @desrosj. I used your simple patch as it included the least amount of code being changed to fix this problem.

#14 @SergeyBiryukov
6 years ago

I'd still like to consider 44416-wp_insert_post-rework.diff for 5.1 (maybe in a new ticket).

This seems a bit too clever to grasp at a glance, and should be simplified per the coding standards:

$new_postarr = array_merge(
	array(
 		'ID' => $post_ID,
	),
	compact( array_diff( array_keys( $defaults ), array( 'context', 'filter' ) ) )
);
Last edited 6 years ago by SergeyBiryukov (previous) (diff)

#15 @desrosj
6 years ago

  • Keywords needs-patch added; has-patch removed

Seems like I missed a few. The error caused in #45018 was terminating the build after the first phpunit command was run. This caused multisite and AJAX tests to not be run. While working on fixing #45018, two more instances of showed up.

In WP_Site_Query, $groupby, and $limits can also be undefined when compact() is called.

To see the new warnings, you can check out this test build.

Last edited 6 years ago by desrosj (previous) (diff)

#16 @desrosj
6 years ago

  • Keywords fixed-5.0 removed

#17 @jorbin
6 years ago

In 43832:

php7.3 compatibility: Fix compact throwing notices for multisite

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

By initializing these variables, they can be compacted.

Previously [43819].
See #44416.
Props desrosj.

#18 @pento
6 years ago

  • Keywords fixed-5.0 added

@desrosj
5 years ago

#19 @desrosj
5 years ago

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

PHP 7.3 is scheduled to be released on December 6th. If 5.0 is not released prior to this date, a 4.9.9 may be released as previously discussed.

44416.2.diff is a patch to backport [43819] and [43832] to the 4.9 branch.

Here is a build on Travis showing all of the issues with compact are addressed. Note: this build also includes a backport of [43899], which prevents the entire test suite from running successfully to prove this patch is working.

Last edited 5 years ago by desrosj (previous) (diff)

#20 @desrosj
5 years ago

  • Keywords needs-patch added; fixed-5.0 has-patch removed

Found a new instance of this being an issue in Core. It's possible that several variables in wp_edit_posts_query() are not set prior to calling compact().

@desrosj
5 years ago

@desrosj
5 years ago

#21 @desrosj
5 years ago

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

More details on my last comment. You can see these notices by going to any wp-admin/edit.php screen.

Also, there is another notice triggered on the Add New Plugin page caused by $version being undefined.

44416.3.2.diff fixes both of these.

#22 @pento
5 years ago

  • Keywords fixed-5.0 added; has-patch needs-testing removed

As these are just notices, let's leave them for 5.0.1.

@desrosj: Would you mind creating a follow up ticket for this?

#24 @conner_bw
5 years ago

While you were java-scripting...

http://php.net/downloads.php#v7.3.0

As these are just notices, let's leave them for 5.0.1.

The notices break unit tests, continuous integration workflows, hence the whole point of (me) tracking this ticket.

#25 @desrosj
5 years ago

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

In 44166:

PHP7.3 compatibility: Fix compact throwing notices.

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.

Props jorbin, desrosj.

Merges [43819] and [43832] to trunk.

Fixes #44416.

#26 @desrosj
5 years ago

In 44185:

PHP 7.3 Compatibility: Fix compact related notices.

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. This fixes a few more instances of unset variables in the WordPress admin.

The full RFC can be viewed here: https://wiki.php.net/rfc/compact.

See #44416.
Fixes #45483.

#27 @desrosj
5 years ago

In 44186:

Networks and Sites: Fix incorrect variable location.

This fixes an issue introduced in [44166] where the $groupby variable was inserted too low in the get_site_ids() function while merging [43832] into trunk. The merged location did not account for a new conditional statement that existed only in trunk, and would have resulted in values assigned to $groupby being erased in certain scenarios.

Props spacedmonkey.

See #44416.
Fixes #45582.

#28 @desrosj
5 years ago

In 44297:

PHP 7.3 Compatibility: Fix compact related notices.

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. This fixes a few more instances of unset variables in the WordPress admin.

The full RFC can be viewed here: https://wiki.php.net/rfc/compact.

See #44416.

Merges [44185] into trunk.

Fixes #45483.

#29 @jorbin
5 years ago

#47560 was marked as a duplicate.

Note: See TracTickets for help on using tickets.