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.

Sign up for GitHub

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

Jump to bottom

Improve consumer type inference with InputFilter templates #91

Merged
merged 12 commits into from
Jul 14, 2023
Merged

Improve consumer type inference with InputFilter templates #91

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
cfa8907
Improve consumer type inference with InputFilter templates
gsteel May 24, 2023
d79f252
fix: Inputs and InputFilters cannot be merged
gsteel Jul 14, 2023
9e83e6b
Remove the 'default' types for TFilteredValues
gsteel Jul 14, 2023
948eede
Minor psalm fixes
gsteel Jul 14, 2023
d1f875b
Input names can feasibly be an array-key type and are integers for co…
gsteel Jul 14, 2023
3978faf
Add a collection input filter to static analysis tests
gsteel Jul 14, 2023
4b52ac8
Clarify existing behaviour with an empty string for an input name
gsteel Jul 14, 2023
410e504
Additional tests to cover input merging behaviour
gsteel Jul 14, 2023
50a46ec
Refresh baseline
gsteel Jul 14, 2023
645131e
Bump filter and validator minimum versions where the filter chain and…
gsteel Jul 14, 2023
df3a17d
Add cookbook documentation on using array shapes with InputFilter
gsteel Jul 14, 2023
4af687c
qa: remove trailing spaces in README
gsteel Jul 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,10 @@
},
"require": {
"php": "~8.1.0 || ~8.2.0",
"laminas/laminas-filter": "^2.13",
"laminas/laminas-filter": "^2.19",
"laminas/laminas-servicemanager": "^3.21.0",
"laminas/laminas-stdlib": "^3.0",
"laminas/laminas-validator": "^2.15"
"laminas/laminas-validator": "^2.25"
},
"require-dev": {
"ext-json": "*",
Expand Down
14 changes: 7 additions & 7 deletions composer.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

46 changes: 11 additions & 35 deletions psalm-baseline.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,18 +26,12 @@
</NonInvariantDocblockPropertyType>
</file>
<file src="src/BaseInputFilter.php">
<DocblockTypeContradiction>
<code><![CDATA[$input instanceof InputInterface && (empty($name) || is_int($name))]]></code>
</DocblockTypeContradiction>
<InvalidReturnStatement>
<code>$messages</code>
<code>$values</code>
</InvalidReturnStatement>
<InvalidReturnType>
<code><![CDATA[array<string, array<array-key, string>>]]></code>
<code>TFilteredValues</code>
Ocramius marked this conversation as resolved.
Show resolved Hide resolved
</InvalidReturnType>
<MixedArgument>
<code>$input</code>
</MixedArgument>
<MixedArgumentTypeCoercion>
<code>$inputs</code>
</MixedArgumentTypeCoercion>
Expand All @@ -49,21 +43,14 @@
<MixedPropertyTypeCoercion>
<code>$inputs</code>
</MixedPropertyTypeCoercion>
<PossiblyInvalidArgument>
<code>$input</code>
</PossiblyInvalidArgument>
<PossiblyNullArrayOffset>
<code><![CDATA[$this->inputs]]></code>
</PossiblyNullArrayOffset>
<RedundantCastGivenDocblockType>
<code>(string) $name</code>
</RedundantCastGivenDocblockType>
<TooManyArguments>
<code>isValid</code>
<code>isValid</code>
</TooManyArguments>
<UnusedPsalmSuppress>
<code>DocblockTypeContradiction</code>
<code>RedundantConditionGivenDocblockType</code>
<code>RedundantConditionGivenDocblockType</code>
</UnusedPsalmSuppress>
Expand All @@ -73,10 +60,6 @@
<code>$name</code>
<code>$name</code>
</ImplementedParamTypeMismatch>
<ImplementedReturnTypeMismatch>
<code><![CDATA[array<array-key, array<string, array<array-key, string>>>]]></code>
<code><![CDATA[array<array-key, array<string, array<array-key, string>>>]]></code>
</ImplementedReturnTypeMismatch>
<InvalidArgument>
<code>$data</code>
</InvalidArgument>
Expand All @@ -87,16 +70,21 @@
<code><![CDATA[$this->invalidInputs]]></code>
<code><![CDATA[$this->validInputs]]></code>
</InvalidPropertyAssignmentValue>
<LessSpecificImplementedReturnType>
<code><![CDATA[array<array-key, array>]]></code>
<code><![CDATA[array<array-key, array>]]></code>
</LessSpecificImplementedReturnType>
<InvalidReturnStatement>
<code><![CDATA[$this->collectionValues]]></code>
</InvalidReturnStatement>
<InvalidReturnType>
<code>TFilteredValues</code>
</InvalidReturnType>
<MixedArgument>
<code>$data</code>
</MixedArgument>
<MixedAssignment>
<code>$data</code>
</MixedAssignment>
<MixedPropertyTypeCoercion>
<code><![CDATA[$this->collectionValues]]></code>
</MixedPropertyTypeCoercion>
<PossiblyUnusedReturnValue>
<code>array[]</code>
<code>array[]</code>
Expand Down Expand Up @@ -136,7 +124,6 @@
<MixedArgumentTypeCoercion>
<code>$filter</code>
<code>$inputSpecification</code>
<code>$key</code>
</MixedArgumentTypeCoercion>
<MixedArrayAccess>
<code><![CDATA[$filter['name']]]></code>
Expand Down Expand Up @@ -359,12 +346,6 @@
<code>ModuleManager</code>
</UndefinedDocblockClass>
</file>
<file src="src/OptionalInputFilter.php">
<ImplementedReturnTypeMismatch>
<code><![CDATA[array<string, mixed>|null]]></code>
<code><![CDATA[array<string, mixed>|null]]></code>
</ImplementedReturnTypeMismatch>
</file>
<file src="test/ArrayInputTest.php">
<ArgumentTypeCoercion>
<code>$valueMap</code>
Expand Down Expand Up @@ -465,12 +446,8 @@
<code>$getMessages</code>
<code>$msg</code>
<code>$msg</code>
<code>$name</code>
</MixedArgumentTypeCoercion>
<MixedArrayAccess>
<code><![CDATA[$filter1->getValues()['nested']['nestedField1']]]></code>
<code><![CDATA[$filter1->getValues()['nested']['nestedField1']]]></code>
<code><![CDATA[$filter1->getValues()['nested']['nestedField1']]]></code>
<code>$inputTypeData[1]</code>
<code>$inputTypeData[2]</code>
</MixedArrayAccess>
Expand Down Expand Up @@ -526,7 +503,6 @@
<PossiblyUndefinedMethod>
<code>getName</code>
<code>getName</code>
<code>isRequired</code>
</PossiblyUndefinedMethod>
<PossiblyUnusedMethod>
<code>addMethodArgumentsProvider</code>
Expand Down
59 changes: 33 additions & 26 deletions src/BaseInputFilter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@
use function is_int;
use function sprintf;

/**
* @template TFilteredValues
* @implements InputFilterInterface<TFilteredValues>
*/
class BaseInputFilter implements
InputFilterInterface,
UnknownInputsCapableInterface,
Expand All @@ -35,16 +39,16 @@ class BaseInputFilter implements
/** @var array<array-key, mixed> */
protected $unfilteredData = [];

/** @var array<string, InputInterface|InputFilterInterface> */
/** @var array<array-key, InputInterface|InputFilterInterface> */
protected $inputs = [];

/** @var array<string, InputInterface|InputFilterInterface>|null */
/** @var array<array-key, InputInterface|InputFilterInterface>|null */
protected $invalidInputs;

/** @var null|array<array-key, string> Input names */
protected $validationGroup;

/** @var array<string, InputInterface|InputFilterInterface>|null */
/** @var array<array-key, InputInterface|InputFilterInterface>|null */
protected $validInputs;

/**
Expand Down Expand Up @@ -73,15 +77,15 @@ public function count()
/**
* Add an input to the input filter
*
* @param InputInterface|InputFilterInterface $input
* @param null|string $name Name used to retrieve this input
* @throws Exception\InvalidArgumentException
* @param InputInterface|InputFilterInterface $input
* @param null|string|int $name Name used to retrieve this input. Can be an integer for collections
* @return $this
* @psalm-suppress MoreSpecificImplementedParamType
* @throws Exception\InvalidArgumentException
*/
public function add($input, $name = null)
{
/** @psalm-suppress RedundantConditionGivenDocblockType,DocblockTypeContradiction */
/** @psalm-suppress RedundantConditionGivenDocblockType */
if (! $input instanceof InputInterface && ! $input instanceof InputFilterInterface) {
throw new Exception\InvalidArgumentException(sprintf(
'%s expects an instance of %s or %s as its first argument; received "%s"',
Expand All @@ -92,12 +96,15 @@ public function add($input, $name = null)
));
}

/** @psalm-suppress TypeDoesNotContainType */
if ($input instanceof InputInterface && (empty($name) || is_int($name))) {
$name = $input->getName();
}

if (isset($this->inputs[$name]) && $this->inputs[$name] instanceof InputInterface) {
if (
isset($this->inputs[$name])
&& $this->inputs[$name] instanceof InputInterface
&& $input instanceof InputInterface
Copy link
Member

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?

Copy link
Member Author

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

) {
// The element already exists, so merge the config. Please note
// that this merges the new input into the original.
$original = $this->inputs[$name];
Expand All @@ -112,8 +119,8 @@ public function add($input, $name = null)
/**
* Replace a named input
*
* @param mixed $input Any of the input types allowed on add() method.
* @param string $name Name of the input to replace
* @param InputInterface|InputFilterInterface $input
* @param array-key $name Name of the input to replace
* @throws Exception\InvalidArgumentException If input to replace not exists.
* @return self
*/
Expand All @@ -136,7 +143,7 @@ public function replace($input, $name)
/**
* Retrieve a named input
*
* @param string $name
* @param array-key $name
* @throws Exception\InvalidArgumentException
* @return InputInterface|InputFilterInterface
*/
Expand All @@ -155,7 +162,7 @@ public function get($name)
/**
* Test if an input or input filter by the given name is attached
*
* @param string $name
* @param array-key $name
* @return bool
*/
public function has($name)
Expand All @@ -166,7 +173,7 @@ public function has($name)
/**
* Remove a named input
*
* @param string $name
* @param array-key $name
* @return InputFilterInterface
*/
public function remove($name)
Expand Down Expand Up @@ -233,7 +240,7 @@ public function isValid($context = null)
/**
* Validate a set of inputs against the current data
*
* @param array<array-key, string> $inputs Array of input names.
* @param array<array-key, array-key> $inputs Array of input names.
* @param array<array-key, mixed> $data
* @param mixed|null $context
* @return bool
Expand Down Expand Up @@ -301,7 +308,7 @@ protected function validateInputs(array $inputs, array $data = [], $context = nu
* Implementations should allow passing a single array value, or multiple arguments,
* each specifying a single input.
*
* @param string|array<array-key, string> $name
* @param array-key|array<array-key, array-key> $name
* @throws Exception\InvalidArgumentException
* @return InputFilterInterface
*/
Expand All @@ -320,14 +327,14 @@ public function setValidationGroup($name)
if (is_array($name)) {
$inputs = [];
foreach ($name as $key => $value) {
if (! $this->has((string) $key)) {
if (! $this->has($key)) {
$inputs[] = $value;
continue;
}

$inputs[] = $key;

$input = $this->inputs[(string) $key];
$input = $this->inputs[$key];
if ($input instanceof InputFilterInterface) {
// Recursively populate validation groups for sub input filters
$input->setValidationGroup($value);
Expand All @@ -351,7 +358,7 @@ public function setValidationGroup($name)
* Implementations should return an associative array of name/input pairs
* that failed validation.
*
* @return array<string, InputInterface|InputFilterInterface>
* @return array<array-key, InputInterface|InputFilterInterface>
*/
public function getInvalidInput()
{
Expand All @@ -364,7 +371,7 @@ public function getInvalidInput()
* Implementations should return an associative array of name/input pairs
* that passed validation.
*
* @return array<string, InputInterface|InputFilterInterface>
* @return array<array-key, InputInterface|InputFilterInterface>
*/
public function getValidInput()
{
Expand All @@ -374,7 +381,7 @@ public function getValidInput()
/**
* Retrieve a value from a named input
*
* @param string $name
* @param array-key $name
* @throws Exception\InvalidArgumentException
* @return mixed
*/
Expand Down Expand Up @@ -402,7 +409,7 @@ public function getValue($name)
* List should be an associative array, with the values filtered. If
* validation failed, this should raise an exception.
*
* @return array<string, mixed>
* @return TFilteredValues
*/
public function getValues()
{
Expand All @@ -424,7 +431,7 @@ public function getValues()
/**
* Retrieve a raw (unfiltered) value from a named input
*
* @param string $name
* @param array-key $name
* @throws Exception\InvalidArgumentException
* @return mixed
*/
Expand Down Expand Up @@ -473,7 +480,7 @@ public function getRawValues()
* Should return an associative array of named input/message list pairs.
* Pairs should only be returned for inputs that failed validation.
*
* @return array<string, array<array-key, string>>
* @return array<array-key, array<array-key, string|array>>
*/
public function getMessages()
{
Expand All @@ -488,7 +495,7 @@ public function getMessages()
/**
* Ensure all names of a validation group exist as input in the filter
*
* @param array<array-key, string> $inputs Input names
* @param array<array-key, array-key> $inputs Input names
* @return void
* @throws Exception\InvalidArgumentException
*/
Expand Down Expand Up @@ -597,7 +604,7 @@ public function getUnknown()
/**
* Get an array of all inputs
*
* @return array<string, InputInterface|InputFilterInterface>
* @return array<array-key, InputInterface|InputFilterInterface>
*/
public function getInputs()
{
Expand Down
Loading