[Form] add guess pattern #3927

ruian · 2012-04-13T15:13:53Z

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

stloyd · 2012-04-13T15:17:20Z

You need to revert change of permissions.

stof · 2012-04-13T15:17:38Z

src/Symfony/Bridge/Doctrine/Form/DoctrineOrmTypeGuesser.php

please revert all permission changes

vicb · 2012-04-13T15:18:40Z

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

stloyd · 2012-04-13T15:18:53Z

src/Symfony/Component/Form/FormTypeGuesserChain.php

@@ -68,6 +68,13 @@ public function guessMinLength($class, $property)
        });
    }

+    public function guessPattern($class, $property)


You should add {@inheritDoc}.

ruian · 2012-04-13T15:30:39Z

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

stof · 2012-04-13T15:34:03Z

@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 · 2012-04-13T15:35:14Z

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

stof · 2012-04-13T15:42:38Z

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

jalliot · 2012-04-13T15:54:01Z

src/Symfony/Component/Form/Extension/Validator/ValidatorTypeGuesser.php

+    public function guessPatternForConstraint(Constraint $constraint)
+    {
+        switch (get_class($constraint)) {
+            case 'Symfony\Component\Validator\Constraints\MaxLength':


Don't you mean Regex?

stof · 2012-04-13T16:17:50Z

Can you squash your commits ?

ruian · 2012-04-13T16:19:42Z

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

jalliot · 2012-04-13T16:23:37Z

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

ruian · 2012-04-13T16:29:43Z

Yep i will look at it

ruian · 2012-04-13T16:59:11Z

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

stof · 2012-04-13T17:02:02Z

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 · 2012-04-13T17:03:10Z

yep sure

vicb · 2012-04-13T17:10:53Z

src/Symfony/Component/Form/FormFactory.php


        $type = $typeGuess ? $typeGuess->getType() : 'text';

        $maxLength = $maxLengthGuess ? $maxLengthGuess->getValue() : null;
-        $minLength = $minLengthGuess ? $minLengthGuess->getValue() : null;


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

Yep sure i revert it as you wish

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

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

ruian · 2012-04-13T20:31:58Z

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

ruian · 2012-04-16T19:26:10Z

@bschussek any comments on this PR ?

webmozart · 2012-04-17T15:00:53Z

src/Symfony/Component/Form/Extension/Validator/ValidatorTypeGuesser.php

+    {
+        switch (get_class($constraint)) {
+            case 'Symfony\Component\Validator\Constraints\Regex':
+                return new PatternGuess($constraint->pattern, Guess::LOW_CONFIDENCE);


You should use ValueGuess instead.

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 · 2012-04-17T15:05:27Z

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 · 2012-04-17T15:13:31Z

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

ruian · 2012-04-18T16:58:14Z

@bschussek should i remove all call to guessMinLength ?

webmozart · 2012-04-20T09:25:51Z

src/Symfony/Component/Form/Extension/Validator/ValidatorTypeGuesser.php

+    {
+        switch (get_class($constraint)) {
+            case 'Symfony\Component\Validator\Constraints\Regex':
+                return new ValueGuess($constraint->pattern, Guess::MEDIUM_CONFIDENCE );


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

webmozart · 2012-04-20T09:27:27Z

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

ruian · 2012-04-20T14:02:33Z

ping @bschussek is it good like this ?

webmozart · 2012-04-20T15:18:42Z

src/Symfony/Component/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);


Please keep the "For" in the method name.

ruian · 2012-04-21T10:18:45Z

@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 · 2012-04-22T14:33:12Z

src/Symfony/Bridge/Doctrine/Form/DoctrineOrmTypeGuesser.php

+     * @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


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

webmozart · 2012-04-22T14:38:00Z

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

ghost · 2012-04-22T18:23:26Z

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

lsmith77 · 2012-04-23T11:58:32Z

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 · 2012-04-23T12:01:03Z

@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 · 2012-04-23T13:40:39Z

@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 · 2012-04-23T13:42:25Z

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

webmozart · 2012-04-23T14:04:01Z

@ruian I already did this. Thanks anyway!

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

@vicb

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

stof reviewed Apr 13, 2012
View reviewed changes

src/Symfony/Bridge/Doctrine/Form/DoctrineOrmTypeGuesser.php

@@ -16,6 +16,7 @@

Copy link

Member

stof Apr 13, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please revert all permission changes

stloyd reviewed Apr 13, 2012
View reviewed changes

jalliot reviewed Apr 13, 2012
View reviewed changes

vicb reviewed Apr 13, 2012
View reviewed changes

webmozart reviewed Apr 17, 2012
View reviewed changes

jmikola mentioned this pull request Apr 17, 2012

add guess pattern doctrine/DoctrineMongoDBBundle#93

Merged

webmozart reviewed Apr 20, 2012
View reviewed changes

webmozart reviewed Apr 22, 2012
View reviewed changes

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

webmozart mentioned this pull request Apr 23, 2012

[Form] Restored and deprecated method guessMinLength in FormTypeGuesser #4085

Merged

Uh oh!

[Form] add guess pattern #3927

[Form] add guess pattern #3927

Uh oh!

Conversation

ruian commented Apr 13, 2012

Uh oh!

stloyd commented Apr 13, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vicb commented Apr 13, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ruian commented Apr 13, 2012

Uh oh!

stof commented Apr 13, 2012

Uh oh!

ruian commented Apr 13, 2012

Uh oh!

stof commented Apr 13, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof commented Apr 13, 2012

Uh oh!

ruian commented Apr 13, 2012

Uh oh!

jalliot commented Apr 13, 2012

Uh oh!

ruian commented Apr 13, 2012

Uh oh!

ruian commented Apr 13, 2012

Uh oh!

stof commented Apr 13, 2012

Uh oh!

ruian commented Apr 13, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ruian commented Apr 13, 2012

Uh oh!

ruian commented Apr 16, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webmozart commented Apr 17, 2012

Uh oh!

ruian commented Apr 17, 2012

Uh oh!

ruian commented Apr 18, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webmozart commented Apr 20, 2012

Uh oh!

ruian commented Apr 20, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ruian commented Apr 21, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webmozart commented Apr 22, 2012

Uh oh!

ghost commented Apr 22, 2012

Uh oh!

lsmith77 commented Apr 23, 2012