#44416 closed defect (bug) (fixed)
`compact()` will throw notice for undefined variables in PHP 7.3
Reported by: | desrosj | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 5.0 | Priority: | normal |
Severity: | normal | Version: | |
Component: | General | Keywords: | php73 fixed-5.0 |
Focuses: | Cc: |
Description (last modified by )
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)
Change History (35)
#4
@
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.
#5
@
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.
#7
@
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
#9
@
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
.
#13
@
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
@
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' ) ) ) );
#15
@
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.
#19
@
6 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.
#20
@
6 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()
.
#21
@
6 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
@
6 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
@
6 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.
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 passingarray_keys( $postarr )
to the call, so all indexes passed to the function may not be represented in variable form. The secondcompact()
call is not causing any issues because those variable names are manually defined in thecompact()
call and all exist.The result of the first
compact()
call is passed to thewp_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 thecompact()
call.I think there are a few options here.
Make sure all documented
$postarr
arguments get a defined variableThis 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.