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

support PHP Enums #16

Closed
wants to merge 5 commits into from
Closed

support PHP Enums #16

wants to merge 5 commits into from

Conversation

patrocle
Copy link

support PHP Enums hasn't been merged

@patrocle
Copy link
Author

 if (
                $expected === '*'
                || $value instanceof \UnitEnum && ($value->name === $expected) // <--- if false here
                || $expected === 'true' && $value === true
                || $expected === 'false' && $value === false
                || is_numeric($expected) && Str::contains($expected, '.') && $value === (float) $expected
                || is_numeric($expected) && $value === (int) $expected
                || (string) $value === $expected // <--- then it checks here
            ) {
                $this->fireModelEvent($change, false);
            }

if $value is an instance of \UnitEnum but $value->name different from $expected then it will check if (string) $value equals $expected, but we can't cast (string) $value from a Enum

@jpkleemans
Copy link
Owner

Hi, thanks for your PR! Sorry for my late reply.
Could you add a unit test that illustrates the problem, so the unit test shows that your code fixes that problem?

@patrocle
Copy link
Author

Hi, I update the test it_works_with_enumerated_casts to show that when we update an enum attribute that doesn't have an event match with $value->name === $expected, it try to cast to (string) $value === $expected
Thanks

Operations priority might differ from what you expect: please wrap needed with '(...)'
@obrunsmann
Copy link

Any chance to get this merged? Solved exception on my side perfectly fine - thanks @patrocle

@obrunsmann
Copy link

@jpkleemans ? 🥴🥴

@patrocle
Copy link
Author

@jpkleemans Hi, can you merge this ? Thanks

@jpkleemans
Copy link
Owner

@patrocle thanks for adding the test. But it looks like it fails?

image

Could you have a look at it?

DennyLoko added a commit to gpmpart/attribute-events that referenced this pull request Sep 29, 2023
This was necessray because some types cannot be converted to string,
and it was throwing an exception when casting an Enum.

This work was done based on the PR opened by @patrocle on
jpkleemans#16
@jpkleemans
Copy link
Owner

Fixed in #18

@jpkleemans jpkleemans closed this Dec 14, 2023
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