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

[Feature] Add support for casting pure enums to integers instead of strings #53358

Draft
wants to merge 2 commits into
base: 11.x
Choose a base branch
from

Conversation

shaheenfawzy
Copy link

[Feature] Add support for casting pure enums to integers instead of strings

This PR modifies the default behavior of storing pure enum values as strings to storing them as integers. The motivation behind this change is that, in most cases I use enums as casts I use integer backed enums —which typically requires maintaining specific value for each enum case— because int columns are more effiecent in storing and indexing, and don't care what is the value of each enum. This PR takes care of giving each case a unique value and it was inspired by implicit enum values in C++.

Cons

  1. Changing the order of the enum cases will give you wrong results as the value of each case is determined by order of the case in the array returned from Enum::cases() method which also depends on the order of the cases in the enum.

Changes Made

  • Updated the getStorableEnumValue method in the HasAttributes trait to return an integer instead of a string.
  • Modified the getEnumCaseFromValue method in the HasAttributes trait to interpret enum values as integers or strings for old values.

Compatibility

This change is expected to be backward-compatible, If you can think of a scenario where this might introduce a breaking change, please let me know.

Tests

  • Tests have not yet been added, but I plan to add them to cover the new behavior.

Example

Here’s an example to illustrate the change:

Before:

Using a pure enum cast in the model:

enum Status {
    case Pending;
    case Approved;
}
// This would store the case name in the db as a string "Pending".
$model->update(['status' => Status::Pending]);
After:

With this PR, using the same pure enum cast will store integer values instead:

// This would store case integer index in Status::cases() array which is 0, (where `PENDING` maps to 0, `APPROVED` to 1, and so on)
$model->update(['status' => Status::Pending]);

Notes

  1. I opted for using the implementation of enum_value() helper method instead of using the method it self because this helper is used throughout the project and I don't know whether this change will cause errors or not.

This change makes it easier to use pure enums in the casts without needing to define explicit integer-backed values for each case.

@shaheenfawzy shaheenfawzy changed the title Initial implementation [Feature] Add support for casting pure enums to integers instead of strings Oct 31, 2024
@shaheenfawzy shaheenfawzy marked this pull request as draft October 31, 2024 11:39
@dennisprudlo
Copy link
Contributor

The con would be a really really big con for me. I don't think devs would expect to break their applications by reordering their enum cases.

Also, if I'm not mistaken and assuming devs won't change the order, reading strings from the DB will still work but writing back will store an integer (or rather a digit since the DB column still is a varchar for example). However, I think this is a huge breaking change. Other apps may use the same DB or an API may return the values from the DB. Now they get "1" instead of "admin".

@shaheenfawzy
Copy link
Author

@dennisprudlo I do agree with you on the first point, but how often actually the order of enum cases are changed, and also the storage of different types in the same column is a big disadvantage in the case of using the same db or an api outside the Laravel application, I don't have a solution for these but I'm open to suggestions.

@henzeb
Copy link
Contributor

henzeb commented Nov 5, 2024

If you want to store an enum as an integer, I suggest using integer backed enums. Reordering would make it less likely one changes the values...

@henzeb
Copy link
Contributor

henzeb commented Nov 5, 2024

@dennisprudlo I do agree with you on the first point, but how often actually the order of enum cases are changed, and also the storage of different types in the same column is a big disadvantage in the case of using the same db or an api outside the Laravel application, I don't have a solution for these but I'm open to suggestions.

I sometimes add a case and place it in alphabetical order or in order of significance. Read, Execute would become Read Write Execute if I added the Write case functionality later on.

@shaheenfawzy
Copy link
Author

Maybe a better idea is to make an interface called CastsToIntegers and implement it in enums where I would like to apply this behaviour

@henzeb
Copy link
Contributor

henzeb commented Nov 8, 2024

Maybe a better idea is to make an interface called CastsToIntegers and implement it in enums where I would like to apply this behaviour

That is when you choose for integer backed enums ...

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