add handling for enum as haystack #168
Conversation
Signed-off-by: Reinfi <[email protected]>
Signed-off-by: Reinfi <[email protected]>
There was a problem hiding this comment.
Ok, from a preliminary observation, I think we should leave the haystack alone, and just populate it from a new option containing the enum.
Changing the haystack itself is probably more trouble, especially for BC.
This also means having a new private setEnum() or such 🤔
Signed-off-by: Reinfi <[email protected]>
|
Still Psalm will fail, but I have not really an idea how to handle that because I had nothing todo with psalm so far 🙈 |
There was a problem hiding this comment.
Overall, approach seems OK 👍
What I'm thinking about is how to get static analysis "happy" with this :D
Still Psalm will fail, but I have not really an idea how to handle that because I had nothing todo with psalm so far 🙈
We could use a .stub file with Psalm, for now, perhaps: we'd declare BackedEnum there, then drop the stub once we drop PHP 8.0 support 🤔
/cc @gsteel do you have any better ideas?
| */ | ||
| public function setEnum(string $enum): self | ||
| { | ||
| if (! is_subclass_of($enum, UnitEnum::class)) { |
There was a problem hiding this comment.
I think we can skip this particular check: class-string<UnitEnum> already verifies this, and code written in newer codebases generally respects types in a much cleaner way
| * | ||
| * @var class-string<UnitEnum>|null | ||
| */ | ||
| protected $enum; |
There was a problem hiding this comment.
Let's make this private before it spreads :D
| @@ -169,6 +204,35 @@ public function setRecursive($recursive) | |||
| */ | |||
| public function isValid($value) | |||
| { | |||
| $enum = $this->getEnum(); | |||
There was a problem hiding this comment.
Since we make $this->enum private, we can directly access it here: no need for a method call
| @@ -169,6 +204,35 @@ public function setRecursive($recursive) | |||
| */ | |||
| public function isValid($value) | |||
| { | |||
| $enum = $this->getEnum(); | |||
| if (is_string($enum)) { | |||
There was a problem hiding this comment.
We should just check for !== null
| public function setEnum(string $enum): self | ||
| { | ||
| if (! is_subclass_of($enum, UnitEnum::class)) { | ||
| throw new Exception\RuntimeException('enum has invalid type'); | ||
| } | ||
|
|
||
| $this->enum = $enum; | ||
| return $this; | ||
| } |
There was a problem hiding this comment.
I wonder if this method should call setHaystack(): that would remove the need for touching the isValid() implementation
|
@Ocramius one things we need to keep in mind is that filters and stuff are usually passed as attributes and/or annotations. I am working with these Interfaces for years now and I have never called setters directly. They are always called from provided factories from within this component and thus I only have to use the attributes/annotations which allow devs to declare a spec which then is processed. Thats why the abstract filter is used because options are being passed as array so that they can be passed to the plugin manager. thats why the "build" method has that optional options array. imho, as long as we do not provide an alternative to that specification, we should not try to avoid the Standard which was declared years back. so I think what would be fine is that we provide a dedicated factory for the ToEnum filter @reinfi, yes. That should work - might be worth testing that in a real project before merging this tho. 😬 |
Description
See issue #167.
Suggestion was to allow the haystack to be an enum. Otherwise a new validator for just the case of enums would be suitable.
I am a bit unsure about the testcase and I hope it would work with the build pipeline, but I will see it afterwards.
And psalm is not working because it misses the classes on PHP 8.0.
Happy to hear your feedback.