Opened 13 months ago
Last modified 7 weeks ago
#59774 reopened defect (bug)
Undefined array key when using wp_list_pluck function
Reported by: | iamarunchaitanyajami | Owned by: | hellofromTonya |
---|---|---|---|
Milestone: | 6.8 | Priority: | normal |
Severity: | normal | Version: | 5.1 |
Component: | General | Keywords: | has-patch has-unit-tests changes-requested early |
Focuses: | Cc: |
Description
Bug Description:
Issue: When using the wp_list_pluck
function to extract values from an array, a PHP warning is triggered if the specified key is not found within the array. The warning message is as follows:
PHP Warning: Undefined array key "required" in /var/www/html/wp-includes/class-wp-list-util.php on line 171
Steps to Reproduce:
- Use the
wp_list_pluck
function on an array. - Specify a key that may or may not exist within the array.
- Observe the PHP warning generated when the key is not found.
Expected Behavior:
The wp_list_pluck
function should gracefully handle cases where the specified key is not present in the array and not trigger a PHP warning.
Actual Behavior:
The function generates a PHP warning with an "Undefined array key" message, which could lead to unnecessary log clutter and confusion.
Environment:
- WordPress version: 6.3 and Higher
- PHP version: >= 8
- Operating system: UBUNTU
Proposed Solution:
To handle this situation gracefully without errors, you can check if the key exists in the array before attempting to access it. You can use the isset() function to determine if the key exists. Here's how you can modify the code:
Code highlighting:
if (is_object($value)) {
$newlist[$key] = isset($value->$field) ? $value->$field : array();
} elseif (is_array($value)) {
$newlist[$key] = isset($value[$field]) ? $value[$field] : array();
} else {
_doing_it_wrong(
__METHOD__,
__('Values for the input array must be either objects or arrays.'),
'6.2.0'
);
}
In the modified code, we use isset() to check if the key $field exists in either the object or the array. If the key exists, it assigns the value to $newlist[$key]. If the key doesn't exist, it assigns empty array().
This modification will prevent the code from triggering an error when attempting to access non-existent keys in an array or object. Instead, it will gracefully assign empty array().
Attachments (2)
Change History (38)
This ticket was mentioned in PR #5596 on WordPress/wordpress-develop by @iamarunchaitanyajami.
13 months ago
#1
#2
in reply to:
↑ description
@
13 months ago
Instead of using isset we will be using coalescing operator ?? to check if the key $field exists.
So the updated code will be
Code highlighting:
if (is_object($value)) { $newlist[$key] = $value->$field ?? array(); } elseif (is_array($value)) { $newlist[$key] = $value[ $field ] ?? array(); } else { _doing_it_wrong( __METHOD__, __('Values for the input array must be either objects or arrays.'), '6.2.0' ); }
Replying to iamarunchaitanyajami:
Bug Description:
Issue: When using the
wp_list_pluck
function to extract values from an array, a PHP warning is triggered if the specified key is not found within the array. The warning message is as follows:
PHP Warning: Undefined array key "required" in /var/www/html/wp-includes/class-wp-list-util.php on line 171
Steps to Reproduce:
- Use the
wp_list_pluck
function on an array.- Specify a key that may or may not exist within the array.
- Observe the PHP warning generated when the key is not found.
Expected Behavior:
Thewp_list_pluck
function should gracefully handle cases where the specified key is not present in the array and not trigger a PHP warning.
Actual Behavior:
The function generates a PHP warning with an "Undefined array key" message, which could lead to unnecessary log clutter and confusion.
Environment:
- WordPress version: 6.3 and Higher
- PHP version: >= 8
- Operating system: UBUNTU
Proposed Solution:
To handle this situation gracefully without errors, you can check if the key exists in the array before attempting to access it. You can use the isset() function to determine if the key exists. Here's how you can modify the code:
Code highlighting:
if (is_object($value)) { $newlist[$key] = isset($value->$field) ? $value->$field : array(); } elseif (is_array($value)) { $newlist[$key] = isset($value[$field]) ? $value[$field] : array(); } else { _doing_it_wrong( __METHOD__, __('Values for the input array must be either objects or arrays.'), '6.2.0' ); }
In the modified code, we use isset() to check if the key $field exists in either the object or the array. If the key exists, it assigns the value to $newlist[$key]. If the key doesn't exist, it assigns empty array().
This modification will prevent the code from triggering an error when attempting to access non-existent keys in an array or object. Instead, it will gracefully assign empty array().
#3
@
13 months ago
- Keywords needs-unit-tests has-testing-info added
- Milestone changed from Awaiting Review to 6.5
- Version changed from trunk to 5.1
Hello @iamarunchaitanyajami,
Welcome to WordPress Core's Trac :) Thank you for opening this ticket.
I'm doing some ticket triage to help this ticket be in a ready state for consideration:
Version
is the WP version that introduced the code or issue. The code in question was introduced in [42527] via #16895 during WP 5.1.- Moving into 6.5 for consideration.
- Keywords:
- Adding
needs-unit-tests
as tests can help to show the issue, confirm it is resolved, and help with future regressions. - Adding
has-testing-info
as you shared the steps to reproduce in the description - thank you :)
- Adding
#5
@
10 months ago
Looking into the function and related PHPUnit tests, my understanding of the wp_list_pluck
implementation is that it should mimic the array_column
behaviour for both, arrays and objects.
And thus my assumption is that we should not add an arbitrary value, array()
, to a non-existing key as is being proposed, as the array_column
simply skips the entry with a missing key. Eg.:
<?php $input_list = array( array( '123' => '456' ), array( 'foo' => 'bar' ), array( 'foo' => 'baz' ), ); var_dump( array_column( $input_list, 'foo', null ) );
produces:
array(2) { [0]=> string(3) "bar" [1]=> string(3) "baz" }
I'm attaching another version of the patch as well as PHPUnit tests. While writting the PHPUnit tests, I have noticed another PHP Warning which would occur in case the $index_key
param of the wp_list_pluck
is not null
, but the $field
is missing in one of the $input_list
items. I have modified existing PHPUnit tests to cover such a situation.
This ticket was mentioned in Slack in #core by rajinsharwar. View the logs.
9 months ago
#8
@
9 months ago
- Keywords needs-unit-tests removed
Removing the "needs-unit-tests" tag, and keeping this in the milestone. Hopefully, we can get some feedback from the committers if this is good to be merged.
#9
@
9 months ago
- Keywords has-unit-tests added
- Owner set to hellofromTonya
- Status changed from new to reviewing
Patch: 59774.diff
As @davidbinda noted, the function should behave the same as array_column()
but also handle objects.
I can reproduce the reported issue. Here it is in action https://3v4l.org/RtbZJ#veol.
After applying the patch, it resolves the issue https://3v4l.org/EuHT5#veol
I'll port the patch over to a PR to let the automated CI jobs run. But then should be ready for commit. Self-assigning.
This ticket was mentioned in PR #6166 on WordPress/wordpress-develop by @hellofromTonya.
9 months ago
#10
Ports https://core.trac.wordpress.org/attachment/ticket/59774/59774.diff here to gain a full run against the automated CI jobs for commit consideration.
Trac ticket: https://core.trac.wordpress.org/ticket/59774
#11
@
9 months ago
- Keywords commit added
https://github.com/WordPress/wordpress-develop/pull/6166
The patch is ready for commit. Prepping shortly.
#12
@
9 months ago
Hmm wait. Upon a deeper look, this ticket is resolving 2 the PHP warning and setting null
for the item in the returned list.
When comparing the results of:
$input_list = array(
array( '123' => '456' ),
array( 'foo' => 'bar' ),
array( 'foo' => 'baz' ),
);
var_dump( array_column( $input_list, 'foo', null ) );
var_dump( wp_list_pluck( $input_list, 'foo', null ) );
array_column()
prints:
array(2) { [0]=> string(3) "bar" [1]=> string(3) "baz" }
but wp_list_pluck()
prints:
Warning: Undefined array key "foo" in ... array(3) { [0]=> NULL [1]=> string(3) "bar" [2]=> string(3) "baz" }
Notice the difference. The first element is null
. So the issue is more than the a PHP Warning, as the NULL
should not be there.
@hellofromTonya commented on PR #5596:
9 months ago
#14
Thank you for submitting this patch. As explained here in the ticket, assigning an empty array is not desired. I'm closing this PR in favor of this commit https://core.trac.wordpress.org/ticket/59774.
@hellofromTonya commented on PR #6166:
9 months ago
#15
Committed via https://core.trac.wordpress.org/changeset/57698.
#16
follow-up:
↓ 21
@
9 months ago
- Resolution fixed deleted
- Status changed from closed to reopened
This has caused a regression for arrays of objects which have magic methods for getting properties.
For example:
wp_list_pluck( [ get_user_by('ID', 1) ], 'user_email' );
returns an empty array()
while get_user_by('ID', 1)->user_email
returns a string.
This ticket was mentioned in Slack in #forums by dd32. View the logs.
9 months ago
#18
@
9 months ago
My initial thought is that something like this could be used:
if ( property_exists( $object, $field ) || isset( $object->$field ) )
That should work in the following scenario's:
- Property exists (may be null)
- Property is magic,
__get()
may return null,__isset()
properly handles null values (ie. does isset(), or checks a property exists in the child object, or has a static list of magic keys)
It would not work when:
- Property is magic,
__get()
returns null,__isset()
fails to handlenull
values, ie. only does an isset() check that doesn't return truthful for null fields.
This ticket was mentioned in PR #6180 on WordPress/wordpress-develop by @hellofromTonya.
9 months ago
#20
- Keywords has-patch added; needs-patch removed
DO NOT REVIEW OR COMMIT THIS PR.
Purpose: to better understand and demonstrate how wp_list_pluck()
used to and now should handle different object scenarios, such as:
- Class with both a
__get()
and__isset()
. - Class with a
__get()
but no__isset()
. - A
__isset()
that does not handlenull
. - Dynamic properties.
- "compat_fields" (compatible but no intended for public) properties.
The original code before r57698 is run via a global $GLOBALS['before_r57698']
.
Trac ticket: https://core.trac.wordpress.org/ticket/59774
#21
in reply to:
↑ 16
@
9 months ago
- Keywords needs-patch added; has-testing-info has-unit-tests has-patch removed
Replying to dd32:
This has caused a regression for arrays of objects which have magic methods for getting properties.
For example:
wp_list_pluck( [ get_user_by('ID', 1) ], 'user_email' );returns an empty
array()
whileget_user_by('ID', 1)->user_email
returns a string.
You're right @dd32. Using your examples:
var_dump( wp_list_pluck( [ get_user_by('ID', 1) ], 'user_email' ) ); var_dump( get_user_by('ID', 1)->user_email );
Results before r57698:
array(1) { [0]=> string(13) "test@test.com" } string(13) "test@test.com"
Results after r57698:
array(0) { } string(13) "test@test.com"
There may be other object handle scenarios that need consideration. I created this patch to help identify those scenarios and better understand how each used to work (before r57698) and how they should work.
The goal:
- Identify all of the missing object handling scenarios.
- Add tests for each.
- Figure out a solution for all.
If the above can happen before RC1, then a follow-up commit will be made. Else, I'd suggest reverting r57698 and moving the ticket to 6.6.
#22
@
9 months ago
This change is negatively affecting WooCommerce core, specifically the Order meta data retrieval, and likely other data types.
These are arrays of objects.
Changing
if ( property_exists( $value, $field ) ) {
to
if ( property_exists( $value, $field ) || isset( $value->$field ) ) {
As suggested by @dd32 in #comment:18, resolves the issue that we have observed.
Thanks to @om4csaba for helping me discover this.
This ticket was mentioned in PR #6195 on WordPress/wordpress-develop by @hellofromTonya.
9 months ago
#23
- Keywords has-patch has-unit-tests added; needs-patch removed
r57698 introduced a regression with objects with magic methods and/or dynamic properties.
This PR seeks to resolve the resolve.
- [X] Add tests for different combinations of classes.
- [X] Add the
|| isset()
conditional fix which solves most of the issues. - [ ] Figure out how to solve scenarios where the
__get()
magically sets a dynamic property but__isset()
does not detect it.
WP_Block::$attributes
is an example for the last bullet task.
Trac ticket: https://core.trac.wordpress.org/ticket/59774
@hellofromTonya commented on PR #6195:
9 months ago
#24
The || isset( $value->$field )
conditional does not solve plucking the attributes
dynamic property from WP_Block
.
Consider the test code:
$block = new WP_Block( $args ); $input_list[] = $block; $this->assertTrue( isset( $block->attributes ), 'isset() should return true as the WP_Block::$attributes dynamic property is magically set in __get()' );
The attributes
dynamic property is magically set in the __get()
method. If you var_dump()
it, it will return the expected value. But isset()
returns false
. This makes sense - see it in action https://3v4l.org/SHAYh.
#25
@
9 months ago
Thank you @jamescollins for reporting the regression and confirming the fix resolves it.
#26
@
9 months ago
- Keywords changes-requested added
PR 6195 includes the fix and various Core classes to test the different scenarios of method magics and dynamic properties.
There are instances in Core where the || isset( $value->$field )
approach does not work. For example, see the issue with isset()
and WP_Block
's attributes
dynamic property. isset()
fails.
I think the solution needs more time. While a solution might be possible before RC1 next week, I'm also thinking a longer soak time is needed to discover unknown scenarios and impacts.
My suggestion:
- Revert r57698.
- Move the ticket to 6.6 and mark for
early
.
It can be recommitted as early as when 6.5 is branched and trunk
opens for 6.6 Alpha.
@swissspidy do you agree?
#27
@
9 months ago
I've been following this ticket a little bit. The suggested approach to retry this early in 6.6 makes sense to me, given the discovered edge cases.
#28
@
9 months ago
Awesome. Thanks for the confidence check @swissspidy :) I'll do the revert shortly and then adjust ticket.
#30
@
9 months ago
- Keywords early added
- Milestone changed from 6.5 to 6.6
Thank you everyone for working through the regression. r57732 reverts the changes that caused the regression(s) and will not ship in 6.5.
I'm marking this ticket for 6.6 early
alpha with the intent to
- (a) commit a full solution to the known scenarios and
- (b) give a long soak time for feedback should there be more scenarios to consider.
9 months ago
#31
The
|| isset( $value->$field )
conditional does not solve plucking theattributes
dynamic property fromWP_Block
.
One could suggest this is a fault of the class not implemening __isset()
.
One could however suggest that wp_list_pluck()
and friends is not intended on being run on a set where the property is not set, and that no checking is needed in the first place.
This ticket was mentioned in Slack in #core by nhrrob. View the logs.
6 months ago
#33
@
6 months ago
- Milestone changed from 6.6 to 6.7
This is an early ticket and at is too late for it in this cycle, so, I am moving it to the next milestone.
#34
@
5 months ago
Should we not deprecate wp_list_pluck()
altogether and encourage using the much faster array_column()
?
That function supports extracting from objects since PHP 7.0, and WP requires 7.4 or greater.
In this PR, we use coalescing operator
??
to check if the key$field
exists in either the object or the array. If the key exists, it assigns the value to$newlist[$key]
. If the key doesn't exist, it assigns emptyarray()
.This modification will prevent the code from triggering an error when attempting to access non-existent keys in an array or object. Instead, it will gracefully assign empty
array()
.Trac ticket: https://core.trac.wordpress.org/ticket/59774