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

Add support for model keys cast to backed enums in FormatModel #1286

Merged
merged 7 commits into from
Dec 30, 2022
Merged

Add support for model keys cast to backed enums in FormatModel #1286

merged 7 commits into from
Dec 30, 2022

Conversation

saadsidqui
Copy link
Contributor

As the title states, add support for model keys cast to backed enums in FormatModel. I have not scouted the whole codebase, there may be other places where a fix is needed to support backed enums for model keys.

I came across this bug while working on a project of mine. I had left this project alone for a couple of weeks before resuming it today. The issue didn't happen before, and I did a composer update when resuming, so this might be a regression. I, unfortunately, do not have the time to check more thoroughly.

Hope this helps. Thank you.

@saadsidqui
Copy link
Contributor Author

This will probably break support for PHP < 8.1. I'm not sure how compatibility is handled in this project, any help would be greatly appreciated.

@driesvints
Copy link
Member

We can't break apps on <8.1. 7.3 is the minimum version right now. You'll need to do a class_exists check I think.

@taylorotwell taylorotwell marked this pull request as draft December 27, 2022 16:37
@saadsidqui
Copy link
Contributor Author

Updated. The reasoning is, if the interface_exists() check fails it will short-circuit, and the rest of the if condition will not execute. Thus, this should maintain compatibility with PHP < 8.1.

@saadsidqui
Copy link
Contributor Author

I have just noticed also, it might be worthwhile to implement a similar check on the case at line 18. Let me know if that makes sense and I will refactor the whole thing to include all cases.

@driesvints
Copy link
Member

Line 18 seems fine to me.

@saadsidqui
Copy link
Contributor Author

I meant the block fulfilling the condition on line 18, specifically line 20 and 21. In that case, $model->getAttribute will also return the foreign/related key id cast to an enum, if such cast is set in the pivot instance. It makes sense given that $model->getKey on line 24 is just a shortcut for getAttribute.

In any case, I have provided the more complete solution that should take care of either case. LMK if you see any issues.

@saadsidqui saadsidqui marked this pull request as ready for review December 28, 2022 18:37
@taylorotwell taylorotwell merged commit 3791d31 into laravel:4.x Dec 30, 2022
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