Skip to content

[Form][Validator] Fixed generation of HTML5 pattern attribute based on Assert\Regex to remove delimiters. #4513

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

Closed
wants to merge 2 commits into from

Conversation

lavoiesl
Copy link
Contributor

@lavoiesl lavoiesl commented Jun 8, 2012

In Issue #3766, it was asked that Assert\Regex generates HTML5 pattern attribute.
It was done in PR #4077, but the generated Regex is in delimited format which is not supported by HTML5.

Hence, /[a-z]+/ would be converted to [a-z]+.
However, if options are specified like in /[a-z]+/i, it cannot be converted and this will throw an Exception. Developer will have to convert his Regex to /[a-zA-Z]+/

Note
I haven’t been able to run PHPUnit, anyone is invited to test the code and the tests themselves.
Sorry about that.
Tests were ran using Travis

@craue
Copy link
Contributor

craue commented Jun 8, 2012

Is this related to #4174?

@lavoiesl
Copy link
Contributor Author

lavoiesl commented Jun 8, 2012

Yes, I didn’t see it at first, thanks.

@lavoiesl
Copy link
Contributor Author

lavoiesl commented Jun 8, 2012

However, instead of throwing an exception when delimiters are used, it may be a good idea not to put slashes at all and some on the fly when used in preg_match. I believe this is how is works for Route requirements.

Even better, we could support both to be backward compatible (but still throw the Exception when options are used)

@lavoiesl
Copy link
Contributor Author

lavoiesl commented Jun 8, 2012

As @excelwebzone said here, it would be best to support a html_pattern option. I will add it and redo the pull request.

@lavoiesl lavoiesl closed this Jun 8, 2012
fabpot added a commit that referenced this pull request Jul 4, 2012
Commits
-------

6f9eda9 [Form][Validator] Fixed generation of HTML5 pattern attribute based on Assert\Regex to remove delimiters.

Discussion
----------

[Form][Validator] Fixed generation of HTML5 pattern attribute based on Assert\Regex by removing delimiters or using a new option: htmlPattern.

Hopefully, this time is the good one…

* Fixes: [#3766, #4077, #4513, #4520, #4521]
* Bug fix: yes
* Feature addition: yes
* BC break: no
* Symfony2 tests pass: yes

In Issue #3766, it was asked that Assert\Regex generates HTML5 pattern attribute.
It was done in PR #4077, but the generated Regex is in delimited format which is not supported by HTML5.

Hence, `/[a-z]+/` would be converted to `[a-z]+`.
If flags are specified like in `/[a-z]+/i`, it cannot be converted and pattern validation will be disabled client-side. If is however now possible, using a new option, `htmlPattern`, to specify the pattern you want to be used.

Example:

```php
<?php
/**
 * @Assert\Regex(pattern="/^[0-9]+[a-z]*$/i", htmlPattern="^[0-9]+[a-zA-Z]*$")
 */
private $civic_number;
```

**Note**: [Documentation](http://symfony.com/doc/current/reference/constraints/Regex.html) should be updated accordingly.

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

by lavoiesl at 2012-06-08T15:45:17Z

God, I just found out you can "add more commits to this pull request by pushing to the master branch on lavoiesl/symfony"…

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

by travisbot at 2012-06-08T15:50:31Z

This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1568634) (merged 2d767b41 into b84b46b).

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

by petajaros at 2012-07-04T14:23:16Z

Anything new about this issue?

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

by lavoiesl at 2012-07-04T16:25:43Z

Alright, tests are passing using `phpunit -c phpunit.xml.dist --filter 'RegexValidatorTest'`. @travisbot reports errors because he can’t even start the tests due to dependencies, which is not related

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

by vicb at 2012-07-04T16:31:13Z

It should be ready to merge when you have taken the last comments into account. thanks.

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

by lavoiesl at 2012-07-04T16:39:05Z

So it seems this PR will finally pass, thanks a lot.

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

by vicb at 2012-07-04T17:03:35Z

Thank you for this PR and the changes.

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

by fabpot at 2012-07-04T17:10:20Z

@lavoiesl Can you squash your commits before I merge? Thanks.

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

by lavoiesl at 2012-07-04T17:25:18Z

There. I also left trace of some commits I did.

Thanks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants