[Form] add guess pattern #3927

Merged
merged 0 commits into from Apr 23, 2012

Projects

None yet

8 participants

@ruian

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #3766
Todo: -

@stloyd

You need to revert change of permissions.

@stof stof commented on an outdated diff Apr 13, 2012
...mfony/Bridge/Doctrine/Form/DoctrineOrmTypeGuesser.php
@@ -16,6 +16,7 @@
@stof
stof Apr 13, 2012

please revert all permission changes

@stof stof commented on an outdated diff Apr 13, 2012
...mfony/Bridge/Doctrine/Form/DoctrineOrmTypeGuesser.php
@@ -16,6 +16,7 @@
use Symfony\Component\Form\Guess\Guess;
use Symfony\Component\Form\Guess\TypeGuess;
use Symfony\Component\Form\Guess\ValueGuess;
+use Symfony\Component\Form\Guess\PatternGuess;
@stof
stof Apr 13, 2012

this class is not used

@stof stof commented on an outdated diff Apr 13, 2012
src/Symfony/Bridge/Propel1/Form/PropelTypeGuesser.php
@@ -15,6 +15,7 @@
use Symfony\Component\Form\Guess\Guess;
use Symfony\Component\Form\Guess\TypeGuess;
use Symfony\Component\Form\Guess\ValueGuess;
+use Symfony\Component\Form\Guess\PatternGuess;
@stof
stof Apr 13, 2012

unused here too

@stloyd stloyd commented on an outdated diff Apr 13, 2012
...ent/Form/Extension/Validator/ValidatorTypeGuesser.php
@@ -233,6 +246,21 @@ public function guessMinLengthForConstraint(Constraint $constraint)
}
/**
+ * Guesses a field's pattern based on the given constraint
+ *
+ * @param Constraint $constraint The constraint to guess for
+ *
+ * @return Guess The guess for the pattern
+ */
+ public function guessPatternForConstraint(Constraint $constraint)
+ {
+ switch (get_class($constraint)) {
@stloyd
stloyd Apr 13, 2012

switch ? For one case ?

@vicb

Some file permissions have been changed in your commit, they should be reverted

@stloyd stloyd commented on an outdated diff Apr 13, 2012
src/Symfony/Component/Form/FormTypeGuesserChain.php
@@ -68,6 +68,13 @@ public function guessMinLength($class, $property)
});
}
+ public function guessPattern($class, $property)
@stloyd
stloyd Apr 13, 2012

You should add {@inheritDoc}.

@stloyd stloyd and 4 others commented on an outdated diff Apr 13, 2012
src/Symfony/Component/Form/Tests/FormFactoryTest.php
- $author->firstName = 'John';
- $author->setLastName('Smith');
-
- $this->assertEquals($author, $form->getData());
+ public function testFieldTypeCreatesDefaultValueForEmptyDataOption()
+ {
+ $factory = new FormFactory(array(new \Symfony\Component\Form\Extension\Core\CoreExtension()));
+
+ $form = $factory->createNamedBuilder(new AuthorType(), 'author')->getForm();
+ $form->bind(array('firstName' => 'John', 'lastName' => 'Smith'));
+
+ $author = new Author();
+ $author->firstName = 'John';
+ $author->setLastName('Smith');
+
+ $this->assertEquals($author, $form->getData());
@stloyd
stloyd Apr 13, 2012

What was change here ?

@Ocramius
Ocramius Apr 13, 2012

Loading with ?w=0 shows no diff, so whitespace I guess

@ruian
ruian Apr 13, 2012

I don't know why it shows diff here...

@webmozart
webmozart Apr 22, 2012

Can revert this hunk? I think you can do this like:

git checkout -p -- src/Symfony/Component/Form/Tests/FormFactoryTest.php

and then only select this hunk.

@vicb vicb commented on an outdated diff Apr 13, 2012
src/Symfony/Component/Form/Tests/FormFactoryTest.php
+ Guess::MEDIUM_CONFIDENCE
+ )));
+
+ $this->guesser2->expects($this->once())
+ ->method('guessPattern')
+ ->with('Application\Author', 'firstName')
+ ->will($this->returnValue(new PatternGuess(
+ '/[a-z-A-Z]/',
+ Guess::HIGH_CONFIDENCE
+ )));
+
+ $factory = $this->createMockFactory(array('createNamedBuilder'));
+
+ $factory->expects($this->once())
+ ->method('createNamedBuilder')
+ ->with('text', 'firstName', null, array('pattern' => '/[a-z-A-Z]/'))
@vicb
vicb Apr 13, 2012

should be \- in the middle ?

@ruian

What do you mean by "please revert all permission changes" because i did not change any permissions by myself :/

@stof
Symfony member

@ruian which OS are you using ? If you are on windows, you should configure git to ignore file permissions (as Windows cannot handle them). Msysgit should do it by default but some other tools don't configure git this way: http://stackoverflow.com/questions/6476513/git-file-permissions-on-windows-7

@ruian

@stof i'm on linux but i did not change any chmod or chown since a clone the repo

@stof
Symfony member

@ruian In your pull request, you are changing file permissions from 644 to 755. This is what should be reverted

@jalliot jalliot and 1 other commented on an outdated diff Apr 13, 2012
...ent/Form/Extension/Validator/ValidatorTypeGuesser.php
@@ -233,6 +246,21 @@ public function guessMinLengthForConstraint(Constraint $constraint)
}
/**
+ * Guesses a field's pattern based on the given constraint
+ *
+ * @param Constraint $constraint The constraint to guess for
+ *
+ * @return Guess The guess for the pattern
+ */
+ public function guessPatternForConstraint(Constraint $constraint)
+ {
+ switch (get_class($constraint)) {
+ case 'Symfony\Component\Validator\Constraints\MaxLength':
@jalliot
jalliot Apr 13, 2012

Don't you mean Regex?

@stof stof and 1 other commented on an outdated diff Apr 13, 2012
src/Symfony/Component/Form/FormFactory.php
@@ -329,13 +329,14 @@ public function createBuilderForProperty($class, $property, $data = null, array
$typeGuess = $this->guesser->guessType($class, $property);
$maxLengthGuess = $this->guesser->guessMaxLength($class, $property);
$minLengthGuess = $this->guesser->guessMinLength($class, $property);
- $requiredGuess = $this->guesser->guessRequired($class, $property);
+ $requiredGuess = $this->guesser->guessRequired($class, $property);
@stof
stof Apr 13, 2012

why changing this line ? We don't align = in Symfony. It makes updates painful (changing many lines when you add a new longer key), and you don't align the whole block anyway

@ruian
ruian Apr 13, 2012

ah ok, i will revert it.

@stof
Symfony member

Can you squash your commits ?

@ruian

you mean rebase ? If it the case i have never test

@jalliot

Look at the doc at the end of the page ;)

@ruian

Yep i will look at it

@ruian

@stof I squash my commits. It's ok ?

@stof
Symfony member

It looks good to me. @fabpot @bschussek ready for the final review

@ruian could you send PRs to https://github.com/doctrine/DoctrineMongoDBBundle https://github.com/doctrine/DoctrinePHPCRBundle and https://github.com/doctrine/DoctrineCouchDBBundle to add the new method ?

@ruian

yep sure

@vicb vicb and 2 others commented on an outdated diff Apr 13, 2012
src/Symfony/Component/Form/FormFactory.php
$type = $typeGuess ? $typeGuess->getType() : 'text';
$maxLength = $maxLengthGuess ? $maxLengthGuess->getValue() : null;
- $minLength = $minLengthGuess ? $minLengthGuess->getValue() : null;
@vicb
vicb Apr 13, 2012

this is wrong... as getValue() can return null. Could you revert and add a comment here (which I should have done in first place).

@ruian
ruian Apr 13, 2012

Yep sure i revert it as you wish

@stof
stof Apr 13, 2012

it is needed as you code is broken in case of a null value

@ruian
ruian Apr 13, 2012

It's fixed now, i will squash and make some PR into others Repo who depends of this

@ruian

@stof I made all Doctrine PRs to respect this new one

@ruian

@bschussek any comments on this PR ?

@webmozart webmozart commented on an outdated diff Apr 17, 2012
...ent/Form/Extension/Validator/ValidatorTypeGuesser.php
@@ -233,6 +246,21 @@ public function guessMinLengthForConstraint(Constraint $constraint)
}
/**
+ * Guesses a field's pattern based on the given constraint
+ *
+ * @param Constraint $constraint The constraint to guess for
+ *
+ * @return Guess The guess for the pattern
+ */
+ public function guessPatternForConstraint(Constraint $constraint)
+ {
+ switch (get_class($constraint)) {
+ case 'Symfony\Component\Validator\Constraints\Regex':
+ return new PatternGuess($constraint->pattern, Guess::LOW_CONFIDENCE);
@webmozart
webmozart Apr 17, 2012

You should use ValueGuess instead.

@webmozart
webmozart Apr 17, 2012

You should put MEDIUM_CONFIDENCE here. A second case statement should be added for the MinLength constraint. If you check FormFactory, a "pattern" constraint is created there for min length - these two functionalities must now be merged.

@webmozart webmozart commented on an outdated diff Apr 17, 2012
src/Symfony/Component/Form/FormFactory.php
@@ -330,12 +330,14 @@ public function createBuilderForProperty($class, $property, $data = null, array
$maxLengthGuess = $this->guesser->guessMaxLength($class, $property);
$minLengthGuess = $this->guesser->guessMinLength($class, $property);
@webmozart
webmozart Apr 17, 2012

guessMinLength() should be removed entirely and merged into guessPattern()

@webmozart
Symfony member

Hi @ruian, thank you for the PR! I added some comments and hope you can check them soon. If you need help, just drop a message.

@ruian

@bschussek thx for your comments, i will do this as soon as possible

@jmikola jmikola referenced this pull request in doctrine/DoctrineMongoDBBundle Apr 17, 2012
Merged

add guess pattern #93

@ruian

@bschussek should i remove all call to guessMinLength ?

@webmozart webmozart commented on an outdated diff Apr 20, 2012
...ent/Form/Extension/Validator/ValidatorTypeGuesser.php
@@ -233,6 +245,21 @@ public function guessMinLengthForConstraint(Constraint $constraint)
}
/**
+ * Guesses a field's pattern based on the given constraint
+ *
+ * @param Constraint $constraint The constraint to guess for
+ *
+ * @return Guess The guess for the pattern
+ */
+ public function guessPatternForConstraint(Constraint $constraint)
+ {
+ switch (get_class($constraint)) {
+ case 'Symfony\Component\Validator\Constraints\Regex':
+ return new ValueGuess($constraint->pattern, Guess::MEDIUM_CONFIDENCE );
@webmozart
webmozart Apr 20, 2012

The pattern for the MinLength constraint should be added here as well (with LOW_CONFIDENCE).

@webmozart
Symfony member

@ruian Yes, guessMinLength() can be removed entirely (and all references to it).

@ruian

ping @bschussek is it good like this ?

@webmozart webmozart commented on an outdated diff Apr 20, 2012
...ent/Form/Extension/Validator/ValidatorTypeGuesser.php
{
$guesser = $this;
return $this->guess($class, $property, function (Constraint $constraint) use ($guesser) {
- return $guesser->guessMinLengthForConstraint($constraint);
+ return $guesser->guessPatternConstraint($constraint);
@webmozart
webmozart Apr 20, 2012

Please keep the "For" in the method name.

@webmozart webmozart commented on an outdated diff Apr 20, 2012
...ent/Form/Extension/Validator/ValidatorTypeGuesser.php
case 'Symfony\Component\Validator\Constraints\SizeLength':
- return new ValueGuess($constraint->min, Guess::HIGH_CONFIDENCE);
+ return new ValueGuess(sprintf('.{%s,}', $constraint->min), Guess::LOW_CONFIDENCE);
+
+ case 'Symfony\Component\Validator\Constraints\Min':
@webmozart
webmozart Apr 20, 2012

This is not correct. Min enforces a value to have a minimal numeric value, for example Min(100), but has no relation to lengths.

@webmozart
webmozart Apr 20, 2012

Ah, my bad. You should keep it as before, i.e. put

strlen((string) $constraint->min)

instead of

$constraint->min
@webmozart webmozart commented on an outdated diff Apr 20, 2012
...ent/Form/Extension/Validator/ValidatorTypeGuesser.php
+ case 'Symfony\Component\Validator\Constraints\Size':
@webmozart
webmozart Apr 20, 2012

This should be changed like Min.

@webmozart webmozart commented on an outdated diff Apr 20, 2012
...ent/Form/Extension/Validator/ValidatorTypeGuesser.php
+ case 'Symfony\Component\Validator\Constraints\Size':
+ return new ValueGuess(sprintf('.{%s,}', $constraint->min), Guess::LOW_CONFIDENCE);
+
+ case 'Symfony\Component\Validator\Constraints\Regex':
+ return new ValueGuess($constraint->pattern, Guess::MEDIUM_CONFIDENCE );
+
case 'Symfony\Component\Validator\Constraints\Type':
@webmozart
webmozart Apr 20, 2012

This should be removed. Guessing "null" does not help.

@webmozart webmozart and 1 other commented on an outdated diff Apr 20, 2012
...mfony/Bridge/Doctrine/Form/DoctrineOrmTypeGuesser.php
{
$ret = $this->getMetadata($class);
if ($ret && $ret[0]->hasField($property) && !$ret[0]->hasAssociation($property)) {
$mapping = $ret[0]->getFieldMapping($property);
if (in_array($ret[0]->getTypeOfField($property), array('decimal', 'float'))) {
- return new ValueGuess(null, Guess::MEDIUM_CONFIDENCE);
+ return new ValueGuess(null, Guess::LOW_CONFIDENCE);
@webmozart
webmozart Apr 20, 2012

What's the point of this line?

@ruian
ruian Apr 20, 2012

It's because of merging min into pattern. But if it's useless i remove it

@webmozart
webmozart Apr 20, 2012

Returning a guess with the value "null" is (almost always) useless, because that's the same as having no guess at all. So you can remove this one.

@webmozart webmozart and 2 others commented on an outdated diff Apr 20, 2012
...ent/Form/Extension/Validator/ValidatorTypeGuesser.php
case 'Symfony\Component\Validator\Constraints\Type':
if (in_array($constraint->type, array('double', 'float', 'numeric', 'real'))) {
- return new ValueGuess(null, Guess::MEDIUM_CONFIDENCE);
+ return new ValueGuess(null, Guess::LOW_CONFIDENCE);
@webmozart
webmozart Apr 20, 2012

What's the point in returning null here?

@ruian
ruian Apr 20, 2012

Same as doctrineOrmTypeGuesser comment

@vicb
vicb Apr 20, 2012

Because you can not guess the max length of a float... may be it should not return anything after all

@ruian
ruian Apr 20, 2012

Ok, i understand, but why it was in guessMin if a null was return ?

@vicb
vicb Apr 20, 2012

Ok the rationale:

  • When you have a min value, you guess a min length of this min (LOW_CONFIDENCE) , lines below
  • If this value is a float type, this is wrong so you guess null with MEDIUM_CONFIDENCE to override the previous guess.

Example:
You want a float greater than 5, 4.512313 is not valid but length(4.512314) > length(5)

@webmozart
webmozart Apr 20, 2012

I don't know. If a guesser can't guess, it shouldn't guess. So you can remove this one too.

@vicb
vicb Apr 20, 2012

@bschussek did you get my comment before posting ?

@webmozart
webmozart Apr 20, 2012

I did not :) This makes sense. Please add a comment then that has at least the length of your explanatory comment above ;)

@webmozart
webmozart Apr 20, 2012

Please do also add the link to the ticket as comment.

@vicb
vicb Apr 20, 2012

I'll open an issue for adding a comment. This has to be done for:

  • the validator,
  • the ORM,
  • the ODM,
  • Propel
@ruian

@bschussek @vicb i think it's good now, and i think i understood the diff between Size and SizeLength Constraint ValueGuess (One is for numeric and the other for string). It was not clear in my mind when i made the change. But now it's okay

@webmozart webmozart commented on an outdated diff Apr 22, 2012
...mfony/Bridge/Doctrine/Form/DoctrineOrmTypeGuesser.php
@@ -113,9 +113,20 @@ public function guessMaxLength($class, $property)
}
/**
- * {@inheritDoc}
+ * Guesses a field's pattern based on the given constraint
+ *
+ * - When you have a min value, you guess a min length of this min (LOW_CONFIDENCE) , lines below
+ * - If this value is a float type, this is wrong so you guess null with MEDIUM_CONFIDENCE to override the previous guess.
+ * Example:
+ * You want a float greater than 5, 4.512313 is not valid but length(4.512314) > length(5)
+ * PR https://github.com/symfony/symfony/pull/3927
+ *
+ * @param string $class The fully qualified class name
+ * @param string $property The name of the property to guess for
+ *
+ * @return Guess The guess for the pattern
@webmozart
webmozart Apr 22, 2012

Please add the comment to FormTypeGuesserInterface and use {@inheritdoc} in all implementing classes.

@webmozart
Symfony member

Please squash the commits once you're done. You should also update the CHANGELOG.

@ghost

This needs to be rebased with also as it currently wont merge. (git fetch; git rebase origin/master).

@ruian ruian merged commit b0a6956 into symfony:master Apr 23, 2012
@lsmith77

something is strange with this PR .. why is it marked as merged by @ruian? and why are there no changes in the diff? and why was it labeled as no BC break?

@ruian

@lsmith77 I don't know, what append, i merge the master into my branch...
So a recreate a PR #4077 but it seems alright now.

@webmozart
Symfony member

@lsmith77 You are right, this should be marked as BC break. I originally thought that guessMinLength() was only introduced after 2.0, but it was not. I will update the documentation.

@ruian

@bschussek do you want i create a new PR for the changelog ?

@webmozart
Symfony member

@ruian I already did this. Thanks anyway!

@fabpot fabpot added a commit that referenced this pull request Apr 23, 2012
@fabpot fabpot merged branch bschussek/issue4077 (PR #4085)
Commits
-------

d9e142b [Form] Restored and deprecated method `guessMinLength` in FormTypeGuesser

Discussion
----------

[Form] Restored and deprecated method `guessMinLength` in FormTypeGuesser

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -

![Travis Build Status](https://secure.travis-ci.org/bschussek/symfony.png?branch=issue4077)

This is a follow-up PR to #4077 (see comments in #3927 for reference).
c2a426e
@mmucklo mmucklo pushed a commit that referenced this pull request May 23, 2013
@fabpot fabpot merged branch bschussek/issue4077 (PR #4085)
Commits
-------

d9e142b [Form] Restored and deprecated method `guessMinLength` in FormTypeGuesser

Discussion
----------

[Form] Restored and deprecated method `guessMinLength` in FormTypeGuesser

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -

![Travis Build Status](https://secure.travis-ci.org/bschussek/symfony.png?branch=issue4077)

This is a follow-up PR to #4077 (see comments in #3927 for reference).
2511b2f
@craigmarvelley craigmarvelley pushed a commit to craigmarvelley/symfony that referenced this pull request Nov 26, 2013
@fabpot fabpot merged branch ruian/guess_pattern (PR #4077)
Commits
-------

f7200e4 [Form] added method `guessPattern` to FormTypeGuesserInterface

Discussion
----------

[Form] add guess pattern

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: symfony#3766
Todo: -

Due to some trouble when rebase my previous PR i open a new one with Master merged
Refs PR: symfony#3927

---------------------------------------------------------------------------

by fabpot at 2012-04-23T10:25:57Z

@vicb @bschussek ok for you?

---------------------------------------------------------------------------

by bschussek at 2012-04-23T10:26:51Z

please do also rephrase the commit message to something clearer, like

    [Form] added method `guessPattern` to FormTypeGuesserInterface

---------------------------------------------------------------------------

by bschussek at 2012-04-23T10:27:35Z

Otherwise this looks good :)

---------------------------------------------------------------------------

by ruian at 2012-04-23T10:29:18Z

every changes done
27b4774
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment