[Form] Mix attr option between guessed options and user options #22936
Conversation
| Q | A |
|---|---|
| Branch? | 2.7 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #19871 |
| License | MIT |
| @@ -126,7 +126,7 @@ public function createBuilderForProperty($class, $property, $data = null, array | |||
|
|
|||
| // user options may override guessed options | |||
| if ($typeGuess) { | |||
| $options = array_merge($typeGuess->getOptions(), $options); | |||
| $options = array_replace_recursive($typeGuess->getOptions(), $options); | |||
There was a problem hiding this comment.
I am not sure if it is a good idea to apply this "globally". For the attr option, doing a recursive merge does work. But this may not be the case for all other options where the usage of some keys may be mutually exclusive.
There was a problem hiding this comment.
What we can do: First merge all guess options recursively and then do the merge here as before.
There was a problem hiding this comment.
But this may not be the case for all other options where the usage of some keys may be mutually exclusive.
Sorry :/ I don't see what I missing here, would you show me an example, please? I've done a simple test (https://3v4l.org/5FvnM) but I don't see what is wrong with array_replace_recursive() function :/
There was a problem hiding this comment.
We don't know if it is allowed to use both keys baz and bar at the same time for the foo option.
There was a problem hiding this comment.
I suggest to change the code as follows:
$guessedOptions = array();
if (null !== $pattern) {
$guessedOptions = array_replace_recursive(array('attr' => array('pattern' => $pattern)), $guessedOptions);
}
if (null !== $maxLength) {
$guessedOptions = array_replace_recursive(array('attr' => array('maxlength' => $maxLength)), $guessedOptions);
}
if ($requiredGuess) {
$guessedOptions = array_merge(array('required' => $requiredGuess->getValue()), $guessedOptions);
}
if ($typeGuess) {
$guessedOptions = array_replace_recursive($typeGuess->getOptions(), $guessedOptions);
}
// user options may override guessed options
$options = array_merge($guessedOptions, $options);There was a problem hiding this comment.
Following the first assumption:
But this may not be the case for all other options where the usage of some keys may be mutually exclusive.
Still we can pass false to the user option and it not will be displayed, i.e.:
$form->add('foo', null, array('foo' => array('baz' => false, 'bar' => 'foobar')));
//...
$customGuessedOptions = array('foo' => array('baz' => 'foobaz'));There was a problem hiding this comment.
My comment above should have used array_replace_recursive() for the type guess too (I updated it).
There was a problem hiding this comment.
Losing the maxlength attribute when passing the attr option is expected IMO as you opted to be more clever than the guessers for the attr option.
There was a problem hiding this comment.
Losing the maxlength attribute when passing the attr option is expected IMO [...]
It's not a BC break? before this PR it's not true and the issue description proposes mix attr options also between guessed options and populated options, so now the one that is lost is me :)
There was a problem hiding this comment.
Losing the maxlength attribute when passing the attr option is expected
It should not as it could trigger an unwanted deprecation.
|
I've updated the solution mixing Status: Needs Review |
|
Thank you @yceruto. |
…tions (yceruto) This PR was merged into the 2.7 branch. Discussion ---------- [Form] Mix attr option between guessed options and user options | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #19871 | License | MIT Commits ------- 84f5de9 mix attr options between type-guess options and user options