[Form] Refactor guessing of form options and added min and max guessing #9759
Conversation
|
@bschussek: It can work also for number type, but at the moment it is being rendered as "text" input type. Is there a specific reason beside the comment "type="number" doesn't work with floats"? |
| 'integer', | ||
| array(), | ||
| Guess::HIGH_CONFIDENCE | ||
| ))); |
There was a problem hiding this comment.
thought ubot could detect this wrong indentation
There was a problem hiding this comment.
Let's wait for him to come and check, I'll prepare a fix.
There was a problem hiding this comment.
Apparently he is not so fast, I'll push my fix right now :)
|
I don't like the original approach that I chose for the type guessers too much anymore. I think instead of adding more methods to that interface, we should try to simplify it: public function guessType($class, $property);
public function guessOptions($class, $property, $type);The first method would be basically the same as now, except that it would return only the type, and not the options. (BC should be kept somehow at this point) The second method would then return the options for a class property, given that type The difference to the current behavior would be that Do you want to implement this change? |
|
I definitely agree and I can work on it. |
|
I started working on it from the |
| @@ -42,6 +42,24 @@ public function guessType($class, $property) | |||
| /** | |||
| * {@inheritDoc} | |||
| */ | |||
| public function guessOptions($class, $property, $type) | |||
There was a problem hiding this comment.
You should add the type hint ResolvedFormTypeInterface to the $type argument.
|
Yes, looks good! :) |
|
@stewe I see that this PR is |
|
Yes, I'm working on it. Hope to complete it before the end of the week. |
|
Done. Let's just recap a couple of things:
Please review my commits and let me know if I need to adjust or add something and let me know if I need to squash the commits. |
|
@bschussek Can you review this one please? |
| @@ -122,7 +140,7 @@ public function guessRequired($class, $property) | |||
| /** | |||
| * {@inheritDoc} | |||
| */ | |||
| public function guessMaxLength($class, $property) | |||
| protected function guessMaxLength($class, $property) | |||
There was a problem hiding this comment.
why protected? As far as I see, it's not used for inheritance. So it should either be private or public (for BC).
Applies to all of these changes.
|
Wait, tests are failing, let me fix them. |
| $this->assertNotNull($value); | ||
| $this->assertNull($value->getValue()); | ||
| $this->assertArrayHasKey('maxlength', $attributes); | ||
| $this->assertEquals(null, $attributes['maxlength']->getValue()); |
| @@ -120,7 +138,12 @@ public function guessRequired($class, $property) | |||
| } | |||
|
|
|||
| /** | |||
| * {@inheritDoc} | |||
There was a problem hiding this comment.
Since you changed to @deprecated you can use @inheritdoc again.
|
Ping? |
|
Hi @stewe, thank you for the awesome PR! Some feedback:
|
|
Hi @bschussek, thanks for you reply.
If
|
…anosala) This PR was squashed before being merged into the 2.5-dev branch (closes #10001). Discussion ---------- [Form] Deprecated max_length and pattern options Split of issue #9759 | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | `max_length` and `pattern` in form options | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | symfony/symfony-docs#3461 Commits ------- 52c07c7 Deprecated max_length and pattern options
|
#10001 has been merged now, so this PR should probably be rebased. |
…form types supported attributes
|
@stefanosala ping |
|
@sstok I could rebase this, but honestly I have no idea how to go on on this. |
|
I will close this in favour of #7868 |
We worked on issue #6732 and we added automatically
minandmaxattributes to integer type based on Range validator.It should work also for numeric type, but at the moment it is being rendered as "text" input type. Is there a specific reason beside the comment "type="number" doesn't work with floats"?
We are now working on implementing a guess{Max|Min}Value on Doctrine and Propel guesser.
Credits: @dlondero