Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[11.x] Preserve item type (array or object) when using Arr::select() or Collection::select() #50749

Closed
wants to merge 5 commits into from

Conversation

Icexist
Copy link

@Icexist Icexist commented Mar 25, 2024

10.44.0 added the Collection::select() and Arr::select() methods #49845 and 10.45.0 updated it to work with ArrayAccess #50072

However, these convert any item in either an array or the Collection into an array even if it is an object. See https://github.com/laravel/framework/blob/11.x/src/Illuminate/Collections/Arr.php#L532-L533

For example, if you have an array of objects and select a single column, it will become an array of arrays.

$obj1 = new \stdClass;
$obj1->color = 'blue';
$obj1->temperature = 'warm';

$obj2 = new \stdClass;
$obj2->color = 'green';
$obj2->temperature = 'cool';

$arr = [$obj1, $obj2];

$selected = Arr::select($arr, 'color');
/* 
[
    [ 'color' => 'blue' ],
    [ 'color' => 'green' ],
]
*/

This is unexpected behavior as the Collection returned from Query Builder has each row of data represented as a stdClass object. Using select on that Collection then converts all the stdClass objects to array types, breaking downstream code.

The proposed change would coerce the individual value of the Collection or Array back into an object if the value was originally an object.

@Icexist Icexist changed the title Preserve item type (array or object) when using Arr::select() or Collection::select() [11.x] Preserve item type (array or object) when using Arr::select() or Collection::select() Mar 25, 2024
@driesvints
Copy link
Member

Existing tests shouldn't be altered here and should still pass. Please add new tests for this.

@Icexist Icexist marked this pull request as draft March 25, 2024 14:57
@Icexist
Copy link
Author

Icexist commented Mar 25, 2024

The existing tests expect that the type returned for each value will be an array, so they'll fail unless modified to expect an object when passed an object.

Is there a better way to approach this?

@crynobone
Copy link
Member

Is there a better way to approach this?

You should only add new assertions instead of modifying existing assertions. This ensures all previous expected behavior remains the same even with this PR. Otherwise, this cannot be added to 10.x and instead needs to be proposed to master branch.

@driesvints
Copy link
Member

If this is modifying existing behaviour then it's best to be sent to master as @crynobone suggested.

@driesvints driesvints closed this Mar 26, 2024
@Icexist
Copy link
Author

Icexist commented Mar 26, 2024

Cool, thanks for the clarification! I'll rework this against master and re-send.

I originally sent this against 11.x because the addition of the select broke a macro in my application (that did the same thing) and I wasn't able to migrate to the new select methods because they didn't preserve the type of the items.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants