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

FormTypeGuesser overrides FormBuilder options #19871

Closed
miguelsimoes opened this issue Sep 6, 2016 · 2 comments
Closed

FormTypeGuesser overrides FormBuilder options #19871

miguelsimoes opened this issue Sep 6, 2016 · 2 comments

Comments

@miguelsimoes
Copy link

At the Symfony\Component\Form\FormFactory code of release 2.8.x, line 155 we have:

if ($typeGuess) {
    $options = array_merge($typeGuess->getOptions(), $options);
}

This line breaks the generation of the options being added by a custom TypeGuesser as the below example shows. The suggestion is to use array_merge_recursive instead.

Example:
Assume TypeGuesser::guessMaxLength() returns a max_length to be added to the input, and then, on the TypeGuess::guessType() it is returned another attribute to be added to the input (for example: a data-* thing for javascript or even a class attribute).

What happens is shown on the next "dumps".

$typeGuess->getOptions() returns:

array (size=2)
  'label' => string 'Name' (length=4)
  'attr' => 
    array (size=1)
      'class' => string 'cenas' (length=5)

$options is populated with:

array (size=4)
  'required' => boolean true
  'attr' => 
    array (size=1)
      'maxlength' => int 50
  'disabled' => boolean false
  'constraints' => 
    array (size=2)
      0 => 
        object(Symfony\Component\Validator\Constraints\NotBlank)[573]
          public 'message' => string 'This value should not be blank.' (length=31)
          public 'payload' => null
      1 => 
        object(Symfony\Component\Validator\Constraints\Length)[574]
          public 'maxMessage' => string 'This value is too long. It should have {{ limit }} character or less.|This value is too long. It should have {{ limit }} characters or less.' (length=140)
          public 'minMessage' => string 'This value is too short. It should have {{ limit }} character or more.|This value is too short. It should have {{ limit }} characters or more.' (length=142)
          public 'exactMessage' => string 'This value should have exactly {{ limit }} character.|This value should have exactly {{ limit }} characters.' (length=108)
          public 'charsetMessage' => string 'This value does not match the expected {{ charset }} charset.' (length=61)
          public 'max' => int 50
          public 'min' => null
          public 'charset' => string 'UTF-8' (length=5)
          public 'payload' => null

When the merge is executed, we get:

array (size=5)
  'label' => string 'Name' (length=4)
  'attr' => 
    array (size=1)
      'maxlength' => int 50
  'required' => boolean true
  'disabled' => boolean false
  'constraints' => 
    array (size=2)
      0 => 
        object(Symfony\Component\Validator\Constraints\NotBlank)[573]
          public 'message' => string 'This value should not be blank.' (length=31)
          public 'payload' => null
      1 => 
        object(Symfony\Component\Validator\Constraints\Length)[574]
          public 'maxMessage' => string 'This value is too long. It should have {{ limit }} character or less.|This value is too long. It should have {{ limit }} characters or less.' (length=140)
          public 'minMessage' => string 'This value is too short. It should have {{ limit }} character or more.|This value is too short. It should have {{ limit }} characters or more.' (length=142)
          public 'exactMessage' => string 'This value should have exactly {{ limit }} character.|This value should have exactly {{ limit }} characters.' (length=108)
          public 'charsetMessage' => string 'This value does not match the expected {{ charset }} charset.' (length=61)
          public 'max' => int 50
          public 'min' => null
          public 'charset' => string 'UTF-8' (length=5)
          public 'payload' => null

Instead of the expected result:

array (size=5)
  'label' => string 'Name' (length=4)
  'attr' => 
    array (size=2)
      'class' => string 'cenas' (length=5)
      'maxlength' => int 50
  'required' => boolean true
  'disabled' => boolean false
  'constraints' => 
    array (size=2)
      0 => 
        object(Symfony\Component\Validator\Constraints\NotBlank)[573]
          public 'message' => string 'This value should not be blank.' (length=31)
          public 'payload' => null
      1 => 
        object(Symfony\Component\Validator\Constraints\Length)[574]
          public 'maxMessage' => string 'This value is too long. It should have {{ limit }} character or less.|This value is too long. It should have {{ limit }} characters or less.' (length=140)
          public 'minMessage' => string 'This value is too short. It should have {{ limit }} character or more.|This value is too short. It should have {{ limit }} characters or more.' (length=142)
          public 'exactMessage' => string 'This value should have exactly {{ limit }} character.|This value should have exactly {{ limit }} characters.' (length=108)
          public 'charsetMessage' => string 'This value does not match the expected {{ charset }} charset.' (length=61)
          public 'max' => int 50
          public 'min' => null
          public 'charset' => string 'UTF-8' (length=5)
          public 'payload' => null

Notice that on the expected result we have 2 elements on the 'attr' index of the options, but with the current code we only get 1 element (the max_length).

@xabbuh
Copy link
Member

xabbuh commented Jan 27, 2017

@miguelsimoes Would you like to create a pull request with your suggestion?

@HeahDude
Copy link
Contributor

The title of the issue is wrong, having passed options overriding the guessed one is the expected behavior and what happens in the result above, but 👍 for using array_merge_recursive.

@xabbuh xabbuh removed the Unconfirmed label Mar 5, 2017
fabpot added a commit that referenced this issue Jun 3, 2017
…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
@fabpot fabpot closed this as completed Jun 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants