php-enum icon indicating copy to clipboard operation
php-enum copied to clipboard

Added new type-safe API that allows validating a given value against a given `class-string<Enum>`

Open Ocramius opened this issue 4 years ago • 4 comments

This patch introduces two new methods:

  • Enum::isValidEnumValue()
  • Enum::assertValidEnumValue()

The Enum::isValidEnumValue() is still wonky due to https://github.com/vimeo/psalm/issues/5372, but overall, this allows for matching against other Enum sub-types, by validating that a given $value is effectively a subtype of T within an Enum<T>.

The previous approach did not work, because static method types are not coupled with the class they are put on (they are effectively namespaced functions, and not much else).

Detailed explanation of what was done is available commit-by-commit, but the idea is that it is impossible to validate T for a given Enum<T> when a static method is used to do so, because static methods, by definition, are not coupled to the class they live on.

Therefore, I had to be a bit more creative: Enum::assertValidEnumValue(ConcreteEnumClass::class, $value) is to be used instead.

TODOs:

  • [ ] discuss deprecation, and if they are needed at all
  • [ ] https://github.com/vimeo/psalm/issues/5372
  • [ ] discuss naming of new API

/cc @michaelpetri @guidobonuzzi @DavideBicego

Ocramius avatar Mar 11 '21 17:03 Ocramius

Hey! Just to be sure I understand, these 2 new methods are only useful for static analysis right?

If we can avoid that that would be great though (I'm not sold on deprecating current methods which are simpler to use).

How about:

public static function isValid($value, ?string $enumType = null)

The new parameter would be optional, and only those using static analysis would go through the trouble of providing the class name. Would that work?

mnapoli avatar Mar 13 '21 16:03 mnapoli

Heads up: spent a lot of time debugging issues around this today, and will certainly work more on the patch as soon as I can, but I think what I'll do for now is removing <T> from these existing methods, so we fix the immediate issue (will open a new PR for that).

Ocramius avatar Mar 18 '21 16:03 Ocramius

(I'm not sold on deprecating current methods which are simpler to use).

Agree, probably better to just leave them be.

The new parameter would be optional, and only those using static analysis would go through the trouble of providing the class name. Would that work?

That is an excellent question, and I do indeed need to try it out! :+1:

Ocramius avatar Mar 18 '21 16:03 Ocramius

I recently stumbled upon this as well. If there is something I can help with @Ocramius, let me know.

boesing avatar Jul 14 '21 13:07 boesing