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

Enums are not compatible with Collection::value() #53676

Closed
CasEbb opened this issue Nov 26, 2024 · 3 comments · Fixed by #53777
Closed

Enums are not compatible with Collection::value() #53676

CasEbb opened this issue Nov 26, 2024 · 3 comments · Fixed by #53777

Comments

@CasEbb
Copy link

CasEbb commented Nov 26, 2024

Laravel Version

11.34.0

PHP Version

8.4.1

Database Driver & Version

No response

Description

When trying to retrieve a value with Collection::value() and the expected value is an enum it is skipped.

Steps To Reproduce

<?php

enum MyEnum {
    case MyCase;
}

collect([
    ['id' => 1, 'type' => MyEnum::MyCase],
])->value('type'); // == null

collect([
    ['id' => 1, 'type' => MyEnum::MyCase],
    ['id' => 2, 'type' => 'b'],
])->value('type'); // == "b"
Copy link

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

@CasEbb
Copy link
Author

CasEbb commented Nov 27, 2024

While trying to draft a pull request I ran into the following.

I delved a little deeper into it and it looks like the issue lies in operatorForWhere. value calls firstWhere with only 1 argument (the key, and no value) which ultimately uses operatorForWhere, also with 1 argument. When operatorForWhere is called with just 1 argument it tries to search for any truthy value.

if (func_num_args() === 1) {
$value = true;
$operator = '=';
}

The closure that operatorForWhere returns checks whether the retrieved value and the comparison value differ in being objects. If it does, a simple truth check occurs whether the requested operation is an inequality.

if (count($strings) < 2 && count(array_filter([$retrieved, $value], 'is_object')) == 1) {
return in_array($operator, ['!=', '<>', '!==']);
}

Enums are objects, true is not, so this check will return false. We could change this check to exclude enums:

if (count($strings) < 2 && count(array_filter([$retrieved, $value], function ($v) {
    return is_object($v) && ! $v instanceof UnitEnum;
})) == 1) {
    return in_array($operator, ['!=', '<>', '!==']);
}

However, we run into a strange issue now. operatorForWhere now returns the result of $retrieved == $value. $retrieved is our enum, $value is true. The result of this comparison is false! This might actually be a bug in PHP (see: php/php-src#16954).

Any ideas on how to proceed from here?

@joelbladt
Copy link

If I have understood correctly, you want to receive all results from the collection as an array and the enums should not be skipped. In your second example, the enum is skipped because the value is null.

/**
* Get a single key's value from the first matching item in the collection.
*
* @template TValueDefault
*
* @param string $key
* @param TValueDefault|(\Closure(): TValueDefault) $default
* @return TValue|TValueDefault
*/
public function value($key, $default = null)
{
if ($value = $this->firstWhere($key)) {
return data_get($value, $key, $default);
}
return value($default);
}

$strings = array_filter([$retrieved, $value], function ($value) {
return is_string($value) || (is_object($value) && method_exists($value, '__toString'));
});

This check fails. Simply because method_exists('__toString') cannot be satisfied. In your previous check, you have no values that return the result true. Therefore, the result is also null .

If you want to get all results back as an array, try that:

collect([
    ['id' => 1, 'type' => MyEnum::MyCase],
])->select('type')->toArray();
    
collect([
    ['id' => 1, 'type' => MyEnum::MyCase],
    ['id' => 2, 'type' => 'b'],
])->select('type')->toArray();

/**
 * Result:
 * 
 * array(1) {
 *   [0]=> array(1) {
 *     ["type"]=> enum(App\Enums\MyEnum::MyCase)
 *   }
 * }
 * 
 * array(2) {
 *   [0]=> array(1) {
 *     ["type"]=> enum(App\Enums\MyEnum::MyCase)
 *   }
 *   [1]=> array(1) {
 *     ["type"]=> string(1) "b"
 *   }
 * }
 */

crynobone added a commit that referenced this issue Dec 6, 2024
taylorotwell pushed a commit that referenced this issue Dec 6, 2024
…nd `value()` (#53777)

* [11.x] Improves `Collection` support for enums using `firstWhere()` and `value()`

Fixes #53676

Signed-off-by: Mior Muhammad Zaki <[email protected]>

* Apply fixes from StyleCI

* wip

Signed-off-by: Mior Muhammad Zaki <[email protected]>

* wip

Signed-off-by: Mior Muhammad Zaki <[email protected]>

---------

Signed-off-by: Mior Muhammad Zaki <[email protected]>
Co-authored-by: StyleCI Bot <[email protected]>
browner12 pushed a commit to browner12/framework that referenced this issue Dec 10, 2024
…nd `value()` (laravel#53777)

* [11.x] Improves `Collection` support for enums using `firstWhere()` and `value()`

Fixes laravel#53676

Signed-off-by: Mior Muhammad Zaki <[email protected]>

* Apply fixes from StyleCI

* wip

Signed-off-by: Mior Muhammad Zaki <[email protected]>

* wip

Signed-off-by: Mior Muhammad Zaki <[email protected]>

---------

Signed-off-by: Mior Muhammad Zaki <[email protected]>
Co-authored-by: StyleCI Bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants