Get WordPress

Make WordPress Core

Opened 9 months ago

Last modified 6 weeks ago

#59774 reopened defect (bug)

Undefined array key when using wp_list_pluck function

Reported by: iamarunchaitanyajami's profile iamarunchaitanyajami Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.7 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:

  1. Use the wp_list_pluck function on an array.
  2. Specify a key that may or may not exist within the array.
  3. 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)

class-wp-list-util.php​ (7.3 KB) - added by iamarunchaitanyajami 9 months ago.
59774.diff​ (3.4 KB) - added by david.binda 7 months ago.

Download all attachments as: .zip

Change History (36)

@iamarunchaitanyajami
9 months ago

This ticket was mentioned in ​PR #5596 on ​WordPress/wordpress-develop by ​@iamarunchaitanyajami.
9 months ago #1

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 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().

Trac ticket: https://core.trac.wordpress.org/ticket/59774

#2 in reply to: ↑ description @iamarunchaitanyajami
9 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:

  1. Use the wp_list_pluck function on an array.
  2. Specify a key that may or may not exist within the array.
  3. 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().

#3 @hellofromTonya
9 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 :)

#4 @helgatheviking
8 months ago

I am seeing this issue as well following the same steps. Following.

#5 @david.binda
7 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.

Last edited 7 months ago by david.binda (previous) (diff)

@david.binda
7 months ago

#6 @swissspidy
6 months ago

  • Component changed from Script Loader to General

This ticket was mentioned in ​Slack in #core by rajinsharwar. ​View the logs.
6 months ago

#8 @rajinsharwar
6 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 @hellofromTonya
6 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.
6 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 @hellofromTonya
6 months ago

  • Keywords commit added

​https://github.com/WordPress/wordpress-develop/pull/6166

The patch is ready for commit. Prepping shortly.

#12 @hellofromTonya
6 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.

#13 @hellofromTonya
6 months ago

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

In 57698:

General: Handle missing field in WP_List_Util::pluck().

Handles when the $field (i.e. key or property) is missing in one of the $input_list items by checking the key (array) or property (object) exists before using it for assignment.

Resolves the following bugs:

  • a PHP warning for undefined key|property.
  • null being set for that array or object within the returned list.

The changes resolve the issues in both WP_List_Util::pluck() (if invoked directly) and wp_list_pluck().

Also includes an additional test for the scenario where the wp_list_pluck() $index_key is not null, the $field is missing in one of the $input_list items.

Follow-up to [55423], [51663], [42527], [38928].

Props iamarunchaitanyajami, davidbinda, hellofromTonya, helgatheviking.
Fixes #59774.

​@hellofromTonya commented on ​PR #5596:
6 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.

#16 follow-up: @dd32
5 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.

Last edited 5 months ago by dd32 (previous) (diff)

This ticket was mentioned in ​Slack in #forums by dd32. ​View the logs.
5 months ago

#18 @dd32
5 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 handle null values, ie. only does an isset() check that doesn't return truthful for null fields.

#19 @swissspidy
5 months ago

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

This ticket was mentioned in ​PR #6180 on ​WordPress/wordpress-develop by ​@hellofromTonya.
5 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 handle null.
  • 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 @hellofromTonya
5 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() while get_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 @jamescollins
5 months ago

This change is negatively affecting WooCommerce core, specifically the Order meta data retrieval, and likely other data types.

​https://github.com/woocommerce/woocommerce/blob/707c555091503d30b49537b7074b3072b4a86a1c/plugins/woocommerce/includes/abstracts/abstract-wc-data.php#L367

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.
5 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:
5 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 @hellofromTonya
5 months ago

Thank you @jamescollins for reporting the regression and confirming the fix resolves it.

#26 @hellofromTonya
5 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 @swissspidy
5 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 @hellofromTonya
5 months ago

Awesome. Thanks for the confidence check @swissspidy :) I'll do the revert shortly and then adjust ticket.

#29 @hellofromTonya
5 months ago

In 57732:

General: Revert r57698 for WP_List_Util::pluck().

r57698 caused a regression for arrays of objects which have magic methods and dynamic properties. A fix is identified.

However, a deeper dive discovered additional scenarios which will require a different fix.

Reverting gives more time for resolving these scenarios and more soak time to discover if there are others.

Props dd32, jamescollins, swissspidy.
See #59774.

#30 @hellofromTonya
5 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.

​@dd32 commented on ​PR #6195:
5 months ago #31

The || isset( $value->$field ) conditional does not solve plucking the attributes dynamic property from WP_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.
3 months ago

#33 @oglekler
8 weeks 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 @Cybr
6 weeks 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.

Note: See TracTickets for help on using tickets.