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 IntBackedEnum #33

Open
DaanBiesterbos opened this issue Aug 20, 2024 · 3 comments
Open

Add support for IntBackedEnum #33

DaanBiesterbos opened this issue Aug 20, 2024 · 3 comments

Comments

@DaanBiesterbos
Copy link

Right now the order and position within the enum determine the result. This introduces some problems.
Bitmasks are often used to check permissions etc, therefore the minor risks should be taken more seriously I believe.

  • Very sensitive to potential dangerous bugs when someone is not aware of the implications and makes a change to the enum. Or "small" codestyle fixes done to order enum cases alphabetically may have grave consequences when gone unnoticed.
  • I cannot migrate pre-existing bitmasks to enums, since some values were removed over time. I might have constants like this:
public const MASK_PERMISSION_1 = 1 << 1;
public const MASK_PERMISSION_2 = 1 << 3;
public const MASK_PERMISSION_3 = 1 << 4;

Right now I would have to add placeholders to use this bundle without changing the bitmask itself.

enum Permissions {
     case Permission1;
     case Missing1;
     case Permission2;
     case Permission3;
}

Obviously, this is not what I need, even more so when more values are missing.

Proposed solution:

Add support for int backed enums. Optionally add an argument to the EnumBitmask constructor.

For example:

enum Permissions: int {
    case Permission1 = 1 << 1;
    case Permission2 = 1 << 3;
    case Permission3 = 1 << 4;
}

$bitmask = new EnumBitmask(enum: Permissions::class, mask: 0, maskValues: true);

For now I'll just implement my own EnumBitmask because I cannot wait for this change.
If you agree to the solution I don't mind to create a pull request for it.

@yaroslavche
Copy link
Owner

@DaanBiesterbos thank you for bringing this issue to my attention. You’ve raised some valid concerns regarding the current implementation’s dependency on enum order and the challenges with migrating pre-existing bitmasks.

I agree that relying on the order within the enum introduces potential risks, especially in cases where permissions or other critical flags are involved. The idea of adding placeholders to maintain the correct bitmask values is indeed not an ideal solution.

Your proposed solution of supporting int-backed enums makes sense. It would provide more flexibility and mitigate the risks associated with the enum order, allowing for a safer and more predictable implementation. I believe this enhancement would make the library more robust and easier to integrate with existing codebases.

If you're willing to create a pull request, I would greatly appreciate it. At the moment, I’m dealing with some time constraints, but I’ll make sure to review the PR as soon as I have the opportunity.

Thanks again for your valuable input and contribution!

@DaanBiesterbos
Copy link
Author

DaanBiesterbos commented Aug 25, 2024

Sure, no problem! :)

What I ended up doing was to create a new class. It was difficult to add a constructor argument because the bitmask is instantiated in places where I don't know the value of the new argument. Since we only need my solution I could simply check if the enum is an int backed enum, and if so, use the value. But that won't do, because it would not be backward compatible.

To keep the bundle backward compatible I would say it is best to make a subclass (IntBackedEnumBitmask) and perhaps an abstract class to make it easier for developer to implement their own implementation.

I did find one quirky little thing, which is that the is_a() and is_subclass_of() functions don't seem to work for enums and the IntBackedEnum interface. Possibly the interface is just a stub and is not actually exposed to user land.

I had to use reflection to validate that a given FQCN is indeed an int backed enum. Not the prettiest but that's what I could come up with.

$reflection = new \ReflectionEnum($enumClass);
$isIntBacked = $reflection->isBacked() && $reflection->getBackingType()->getName() === 'int';

Do you overall agree to this approach? Using a subclass and the reflection solution to validate the FQCN argument?

@yaroslavche
Copy link
Owner

@DaanBiesterbos Thank you for sharing your approach. I appreciate the effort to maintain backward compatibility, but I have some concerns about using reflection in this case.

While reflection can be powerful, I believe it's best to avoid it when simpler solutions exist. Instead, I would suggest first checking if the enum is a non-empty BackedEnum, and then verifying the type of its first value. In this case, it's possible to add a boolean constructor parameter with a default value of false.

yaroslavche added a commit that referenced this issue Sep 1, 2024
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

No branches or pull requests

2 participants