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

[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`

[desrosj]

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.

[ayeshrajans]

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]

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.

[pross]

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

[desrosj]

Improved patch for original approach

[desrosj]

More complex rework of wp_insert_post()

[desrosj]

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.

[jorbin]

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.

[jorbin]

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

[SergeyBiryukov]

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' ) ) )
);

[desrosj]

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.

[jorbin]

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.

[desrosj]

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.

[desrosj]

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]

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.

[pento]

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

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

[desrosj]

Done: #45483.

[conner_bw]

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.

[desrosj]

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.

[desrosj]

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.

[desrosj]

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.

[desrosj]

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.

[jorbin]

#47560 was marked as a duplicate.