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

[Form] Refactor guessing of form options and added min and max guessing #9759

Closed
wants to merge 20 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@stefanosala
Copy link

stefanosala commented Dec 14, 2013

Q A
Bug fix? no
New feature? yes
BC breaks? FormTypeGuesserInterface change
Deprecations? max_length and pattern form options
Tests pass? yes
Fixed tickets #6732
License MIT

We worked on issue #6732 and we added automatically min and max attributes 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

@stefanosala

This comment has been minimized.

Copy link

stefanosala commented Dec 14, 2013

@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
)));

This comment has been minimized.

@cordoval

cordoval Dec 14, 2013

Contributor

thought ubot could detect this wrong indentation

This comment has been minimized.

@stefanosala

stefanosala Dec 14, 2013

Let's wait for him to come and check, I'll prepare a fix.

This comment has been minimized.

@stefanosala

stefanosala Dec 14, 2013

Apparently he is not so fast, I'll push my fix right now :)

@webmozart

This comment has been minimized.

Copy link
Contributor

webmozart commented Dec 17, 2013

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 $type was selected.

The difference to the current behavior would be that guessOptions() should always be used, even if a type is manually provided.

Do you want to implement this change?

@stefanosala

This comment has been minimized.

Copy link

stefanosala commented Dec 17, 2013

I definitely agree and I can work on it.

@stefanosala

This comment has been minimized.

Copy link

stefanosala commented Dec 22, 2013

I started working on it from the ValidatorTypeGuesser and the max_length option.
@bschussek can you please check if this is the direction you imagined?

@@ -42,6 +42,24 @@ public function guessType($class, $property)
/**
* {@inheritDoc}
*/
public function guessOptions($class, $property, $type)

This comment has been minimized.

@webmozart

webmozart Dec 28, 2013

Contributor

You should add the type hint ResolvedFormTypeInterface to the $type argument.

{
$options = array();

switch ($type) {

This comment has been minimized.

@webmozart

webmozart Dec 28, 2013

Contributor

Instead of the switch, you can do:

if ($type->getOptionsResolver()->isKnown('max_length')) {
@webmozart

This comment has been minimized.

Copy link
Contributor

webmozart commented Dec 28, 2013

Yes, looks good! :)

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Dec 31, 2013

@stewe I see that this PR is WIP; will you have time to finish it in the near future?

@stefanosala

This comment has been minimized.

Copy link

stefanosala commented Dec 31, 2013

Yes, I'm working on it. Hope to complete it before the end of the week.

@stefanosala

This comment has been minimized.

Copy link

stefanosala commented Jan 4, 2014

Done. Let's just recap a couple of things:

  • I deprecated the options max_length and pattern and moved them to the attr option;
  • I added the guessing of min and max attributes;

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.
Thank you!

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jan 4, 2014

@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)

This comment has been minimized.

@Tobion

Tobion Jan 5, 2014

Member

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.

*
* @return Guess\ValueGuess|null A guess for the field's maximum length
* @return array An array of guesses for the field's attributes

This comment has been minimized.

@Tobion

Tobion Jan 5, 2014

Member

Guess\ValueGuess[]

@stefanosala

This comment has been minimized.

Copy link

stefanosala commented Jan 5, 2014

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());

This comment has been minimized.

@stloyd

stloyd Jan 5, 2014

Contributor

assertNull() same below.

*/
public function guessMaxValueForConstraint(Constraint $constraint)
{
switch (get_class($constraint)) {

This comment has been minimized.

@stloyd

stloyd Jan 5, 2014

Contributor

Why switch()? And not i.e.:

if ($constraint instanceof Range && is_numeric($constraint->max)) {
    return new ValueGuess($constraint->max, Guess::HIGH_CONFIDENCE);
}
}
}
if (count($filteredAttributes)) {

This comment has been minimized.

@stloyd

stloyd Jan 5, 2014

Contributor

!empty($filteredAttributes)? As you just want to know there are some filtered attributes, and not how many of them.

foreach ($this->guessers as $guesser) {
$guessedAttributes = $guesser->guessAttributes($class, $property);
if (false === is_array($guessedAttributes)) {

This comment has been minimized.

@stloyd

stloyd Jan 5, 2014

Contributor

Just if (!is_array($guessedAttributes)) { is enough.

*/
public function guessMaxLength($class, $property);
public function guessAttributes($class, $property);

This comment has been minimized.

@stloyd

stloyd Jan 5, 2014

Contributor

Removal of guessMaxLength() + guessPattern() from interface is BC break.

This comment has been minimized.

@stefanosala

stefanosala Jan 5, 2014

Yes, I know. What do you suggest?

This comment has been minimized.

@wouterj

wouterj Jan 5, 2014

Member

afaik, it's not a BC break as it doesn't fail when you have an extra method in the interface (as long as you keep the 2 methods in the core classes as a BC layer).

The BC break is adding guessAttributes and guessRequired to the interface.

This comment has been minimized.

@stloyd

stloyd Jan 5, 2014

Contributor

IIRC old methods must be still in interface, but with @deprecated phpdoc with note (for details look around code, there is plenty of examples), adding new ones to interface is as BC break too.

This comment has been minimized.

@stefanosala

stefanosala Jan 5, 2014

Is that ok or should I remove the new method too?

@stefanosala

This comment has been minimized.

Copy link

stefanosala commented Jan 5, 2014

@stloyd @Tobion thanks for your review.

@@ -120,7 +138,12 @@ public function guessRequired($class, $property)
}
/**
* {@inheritDoc}

This comment has been minimized.

@Tobion

Tobion Jan 5, 2014

Member

Since you changed to @deprecated you can use @inheritdoc again.

This comment has been minimized.

@stefanosala

stefanosala Jan 5, 2014

Right, thanks!

@stefanosala

This comment has been minimized.

Copy link

stefanosala commented Jan 7, 2014

Ping?

@webmozart

This comment has been minimized.

Copy link
Contributor

webmozart commented Jan 7, 2014

Hi @stewe, thank you for the awesome PR! Some feedback:

  • This PR currently contains two different changes: The deprecation of the "max_length" and "pattern" options, and the change of the value guesser. Could you do the option deprecation in a separate PR? This PR will be quite small and quick to review and merge.
  • You added a new method guessAttributes(), but I suggested guessOptions() with a slightly different signature above. Could you change this? guessOptions() should then return an array with the "attr" option set.
@stefanosala

This comment has been minimized.

Copy link

stefanosala commented Jan 7, 2014

Hi @bschussek, thanks for you reply.

  • Yes, I will.
  • I'm sorry, I didn't write you back my consideration.

If guessOptions($class, $property, ResolvedFormTypeInterface $type) is the signature of the method, how can I:

  • get a ResolvedFormTypeInterface in FormFactory?
  • check that the ResolvedFormTypeInterface supports a particular attribute?

fabpot added a commit that referenced this pull request Mar 26, 2014

feature #10001 [Form] Deprecated max_length and pattern options (stef…
…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
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Mar 26, 2014

#10001 has been merged now, so this PR should probably be rebased.

@sstok

This comment has been minimized.

Copy link
Contributor

sstok commented Jul 27, 2014

@stefanosala

This comment has been minimized.

Copy link

stefanosala commented Aug 5, 2014

@sstok I could rebase this, but honestly I have no idea how to go on on this.

@stefanosala

This comment has been minimized.

Copy link

stefanosala commented Sep 25, 2014

I will close this in favour of #7868

@stefanosala stefanosala deleted the stefanosala:issue-6732 branch Sep 25, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment