Sync#2
Open
namo-R wants to merge 51 commits into
Open
Conversation
PHP 8.2 Declaration of Contributte\FormMultiplier\Multiplier::setValues($values, bool $erase = false): Contributte\FormMultiplier\Multiplier must be compatible with Nette\Forms\Container::setValues(object|array $data, bool $erase = false): static
`Container::getComponents()` returns array when `$deep` is false (by default): nette/component-model@7f613ee The method also no longer takes any arguments: nette/component-model@4e0946a
Follow up to 39725d3
This fixes the following PHPStan errors: 280 Method Contributte\FormMultiplier\Multiplier::getValues() has parameter $returnType with no type specified. 290 No error to ignore is reported on line 290. Let’s also deprecate passing bool `$returnType` deprecated like `nette/forms` 3.2.0 does: nette/forms@0a812bd Also use Array constant made public in 3.1.12: nette/forms@0be7b3d
nette/component-model 3.1.0 made the type annotations more precise, correctly declaring that `null` will not be returned when `$throw` argument is `true` (default): nette/component-model@fbab7bc As a result PHPStan started to complain: Expression on left side of ?? is not nullable.
Follow up to 7445d0d
There are no acceptance tests and `make coverage` would fail with:
Code coverage file /home/runner/work/forms-multiplier/forms-multiplier/tests/_output/c3tmp/codecoverage.serialized does not exist
`make coverage` would fail with:
No code coverage driver available
Sounds like it might have been broken by xdebug 2 → 3 bump:
https://xdebug.org/docs/upgrade_guide#New-Concepts
Cargo culted from contributte/invoice@ca51236
nette/forms@e227d87 added an extra argument. While at it, let’s also narrow the `$values` argument type to match `Container::setValues`. This change remains compatible with nette/forms 3.2.2.
This was accidentally deleted in 1d9a356
nette/forms@0cd5069 added more types but that is overly broad for our usage pattern. We need to cast it to proper value until the return type is clarified upstream. Also use `$this->getForm()` instead of `$this->form` since the former is already checked not to be `null` by `$this->isFormSubmitted()`.
This was accidentally broken in 45cf2a8
This partially reverts commit 39725d3. The commit introduced a regression in `testGroupManualRenderWithButtons`, changed the internal architecture in a way that made it harder to reason about (the Multiplier inconsistently took over some responsibilities of ComponentResolver). Additionally, it applied the following changes that are unrelated to the purported fix and not really necessary: - Latte 2 macros are trivial to migrate and unclearly named, no need to keep them for BC. - The onSuccess handlers were already fixed in ed96ba0. - `testGroupManualRenderWithButtons` look like remnants of debugging effort, they do not fix the test. We are only keeping the following: - Test for nested support (to be implemented later). - Fix for `assertMatchesRegularExpression` deprecation in PHPUnit. For reference, the reverted change is the following commit that was squashed into 39725d3, except for the change to `testSendNested`: 2c33de2
In bde2503, we started to require component-model 3.1.0, which requires PHP 8.1. As a result, CI can no longer build on PHP 8.0. Let’s bump the PHP requirement to match.
nette/forms 3.1.15 clarified that the `Container::getForm` method can only return `null` when passed `throw: false`: nette/forms@ee43bfc With PHPStan 2.0 this would be noticed and result in an error: 384 Strict comparison using !== between Nette\Forms\Form and null will always evaluate to true. 🪪 notIdentical.alwaysTrue Let’s use non-throwing variant and also add the PHPDoc to be explicit.
This is a phpstan-nette bug.
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.
This was introduced in a2ccdcf (#83) but has not been fixed so far. When a group is created inside the modifier, clicking the create button will cause one more fieldset to be created than there are containers: ```html <form action="/" method="post" id="frm-form"> <fieldset> <legend>Team member #1</legend> <table> <tr> <th><label for="frm-form-members-0-name">Name</label></th> <td><input type="text" name="members[0][name]" id="frm-form-members-0-name" class="text"></td> </tr> </table> </fieldset> <fieldset> <legend>Team member #2</legend> <table> <tr> <th><label for="frm-form-members-1-name">Name</label></th> <td><input type="text" name="members[1][name]" id="frm-form-members-1-name" class="text"></td> </tr> </table> </fieldset> <fieldset> <legend>Team member #3</legend> </fieldset> <table> <tr> <th></th> <td><input type="submit" name="members[multiplier_creator]" class=" button" value="add" formnovalidate data-nette-validation-scope='["members"]'></td> </tr> </table> <input type="hidden" name="_do" value="form-submit"> </form> ``` ``` Test tests/unit/CreateButtonTest.php:testFieldsets After adding a container, there should be two fieldsets. Failed asserting that actual size 3 matches expected size 2. ```
The nested multipliers were fixed in 39725d3 but the change broke other features so it was reverted in 71cc347 (keeping the test change since it was what we want). See #92 for some more details. Another regression test was introduced in 1e6e3b2 to ensure the nested multiplier support is correctly implemented. However, proper implementation has not manifested so far so let’s mark the tests as incomplete until then.
…erty
PHPStan 2.1.8 started to complain about `property.protected`:
Access to protected property Nette\Forms\Container::$currentGroup.
See phpstan/phpstan#13123
Fortunately, Nette provides a public method we can use instead.
Though they are currently broken by something else so marking them as ignored.
This would have caused an exception anyway since the `removeButton` property would have been `null`:
[Error] Call to a member function create() on null
This was fixed in 9e0847c but there was no test. Also add a comment since the code is non-obvious.
Currently, it is allowed so mark the test as ignored.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.