-
-
Notifications
You must be signed in to change notification settings - Fork 28
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
Improve consumer type inference with InputFilter templates #91
Conversation
Adds a template to InputFilterInterface to define the array shape of filtered values Signed-off-by: George Steel <george@net-glue.co.uk>
Solves a type collision where `add` accepts InputInterface or InputFilterInterface but does not check that the merge targets are compatible. Signed-off-by: George Steel <george@net-glue.co.uk>
Psalm does not really work on the basis of template defaults, they are a restriction. This causes problems with nested input filters and collections Signed-off-by: George Steel <george@net-glue.co.uk>
Signed-off-by: George Steel <george@net-glue.co.uk>
…llections Signed-off-by: George Steel <george@net-glue.co.uk>
Signed-off-by: George Steel <george@net-glue.co.uk>
Signed-off-by: George Steel <george@net-glue.co.uk>
Signed-off-by: George Steel <george@net-glue.co.uk>
Signed-off-by: George Steel <george@net-glue.co.uk>
2711b82
to
50a46ec
Compare
… validator chain became iterable Signed-off-by: George Steel <george@net-glue.co.uk>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the fact that you (a) added more tests to cover more scenarios, and (b) added the static analysis classes to validate the templates!
One big request, however: I think it would be fantastic to add some info to the docs about the Psalm shapes, and some examples of how they are used (these could be pulled from the static analysis cases you provide, or even just link to them). This will make it clear that we offer the feature; otherwise, it will only be discoverd by those diving into the source code.
if ( | ||
isset($this->inputs[$name]) | ||
&& $this->inputs[$name] instanceof InputInterface | ||
&& $input instanceof InputInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this condition added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InputFilter::add()
accepts InputInterface|InputFilterInterface
- The condition was always there, asserting the left side was InputInterface
but there was nothing to check the right side was the correct type - There's a test to cover it - effectively, we had the possibility of types smashing together with InputInterface::merge(InputFilterInterface)
- I considered changing the original condition to just check the type equality but that would have broken BC - i.e. the existing behaviour would be a replace if the type was an InputFilterInterface
Signed-off-by: George Steel <george@net-glue.co.uk>
Added a first stab at documentation @weierophinney |
Signed-off-by: George Steel <george@net-glue.co.uk>
Perfect! I'm approving now, but only because I don't fully understand Marco's concern (though your explanation makes sense to me). |
Thanks @gsteel! |
Description
Adds a template to InputFilterInterface to define the array shape of filtered values
The plan here is better inference when calling
InputFilterInterface::getValues()
. It might cause a lot of noise for users because psalm requires a template declaration for all inheritors, that said, it'd be useful to those who care.Once merged, users will be able to define an input filter such as:
… and psalm will infer the types for whatever comes out of
$inputFilter->getValues()
such as:As part of wrestling with Psalm on this patch, it's worth mentioning that all input names can now be
array-key
as opposed tostring
. This is not really related to the patch but something I had to address becauseCollectionInputFilter
is effectively a nested list, it's actually a requirement to be able to fetch an input with something like$inputFilter->get(1)
This work has also uncovered at least one or two subtle bugs or ambiguous behaviour and fixes an issue where
InputFilter::add
will attempt to callInput::merge
with anInputFilter
as an argument