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

Multiplier::validate(): Filter out non-validatable components #103

Merged
merged 3 commits into from
Jan 17, 2025

Conversation

jtojnar
Copy link
Collaborator

@jtojnar jtojnar commented Mar 21, 2024

45ed76a started filtering components to Controls, to appease PHPStan, since Container::validate() only accepts Control[]. But Multiplier does not actually have Controls as direct children (other than the ‘Add’ Submitters), so it would stop validating and filtering multiplied controls.

a5a7348 reverted that part but kept the incorrect phpdoc type cast.

Now, it works without the filter because Container::validate() already ignores non-validatable components but we should still respect its contract.

Let’s filter the components before passing them down. This will also allow us to drop the lying phpdoc type cast.

Depends on nette/forms#323 for PHPStan to pass.

@jtojnar jtojnar marked this pull request as draft March 21, 2024 00:18
@jtojnar jtojnar force-pushed the validate-filter branch from 6932abf to 81e3861 Compare May 14, 2024 21:48
@jtojnar jtojnar marked this pull request as ready for review May 14, 2024 21:50
@jtojnar jtojnar marked this pull request as draft May 14, 2024 21:53
@jtojnar
Copy link
Collaborator Author

jtojnar commented May 17, 2024

The PHPStan issue is caused by phpstan/phpstan-nette#141

@f3l1x
Copy link
Member

f3l1x commented Jan 10, 2025

Any progress in here?

@jtojnar
Copy link
Collaborator Author

jtojnar commented Jan 10, 2025

Still blocked on the PHPStan issue IIRC. Might need to put it to ignore list.

45ed76a started filtering components
to `Control`s, to appease PHPStan, since `Container::validate()` only
accepted `Control[]`. But `Multiplier` does not actually have `Control`s
as direct children (other than the ‘Add’ `Submitter`s), so it would stop
validating and filtering multiplied controls.

a5a7348 reverted that part but kept
the incorrect phpdoc type cast.

Now, it works without the filter because `Container::validate()` already
ignores non-validatable components but we should still respect its contract.

Let’s filter the components before passing them down. This will also
allow us to drop the lying phpdoc type cast.

nette/forms 3.2.2 updated its phpdoc param type to allow that:
nette/forms@6437671
Filters are used for converting an input value (usually a string)
into a domain specific PHP type and they are bound to the validation scope:
https://doc.nette.org/en/forms/validation#toc-modifying-input-values

45ed76a started filtering components
passed to `Container::validate()` to `Control`s, to appease PHPStan.
But `Multiplier` does not actually have `Control`s as direct children
(other than the ‘Add’ `Submitter`s), so it would stop validating and
filtering multiplied controls.

It has been since fixed in a5a7348,
and more properly in the parent commit but let’s add a test so this
does not happen again.

It can be reproduced by removing `|| $component instanceof Container`
from the parent commit.
It was changed to array in bde2503 to override incorrect return value from phpstan-nette’s stub.
But with upgrade to PHPStan 2.0 in 07a1009, it is no longer helping – we had to resort to ignoring the error in 9533530.
@jtojnar jtojnar marked this pull request as ready for review January 17, 2025 23:40
@jtojnar jtojnar merged commit 71b44f4 into master Jan 17, 2025
5 of 17 checks passed
@jtojnar jtojnar deleted the validate-filter branch January 17, 2025 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants