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

add a case-insensitive variant of Form::PATTERN #187

Merged
merged 1 commit into from Jul 27, 2018

Conversation

Projects
None yet
2 participants
@renekliment
Copy link
Contributor

renekliment commented Jul 25, 2018

  • bug fix / new feature? a little bit of both; fixes #185
  • BC break? no
  • doc PR: TBD

This adds an option to validate form controls like via Form::PATTERN, but with case-insensitive regexp.

  1. Not sure about the name.
  2. Hesitated between an internal common validation method and two child methods - case-sensitive and case-insensitive, but went for this compact solution.

I have a question about the u regexp modifier. It is used in the PHP code, but not in the JS code. However, when I add it to the JS code, the tests break. Do you have more info on this? I thought this would result in enabled case folding on both sides consistently, but ... I don't know, this is not my area of expertise.

Appreciate any pointers / ideas for improvements.
Thank you.

@renekliment renekliment force-pushed the renekliment:pattern_icase branch from dd41d13 to 46d72e9 Jul 25, 2018

@dg

This comment has been minimized.

Copy link
Member

dg commented Jul 25, 2018

I have a question about the u regexp modifier.

Some old browsers (such as PhantomJS used for testing) do not support u modifier.

I fixed it in e3b5529

@@ -448,13 +448,14 @@
} catch (e) {} // eslint-disable-line no-empty
},

pattern: function(elem, arg, val) {
pattern: function(elem, arg, val, value, modifiers) {

This comment has been minimized.

@dg

dg Jul 25, 2018

Member

I think that argument caseInsensitive is better than modifiers.

This comment has been minimized.

@renekliment

renekliment Jul 25, 2018

Contributor

Ok. The same for the PHP bit then?

pub fun xxx(..., bool $caseInsensitive = false) {
    $modifiers = 'u' . ($caseInsensitive ? 'i' : '');
}

This comment has been minimized.

@dg

dg Jul 25, 2018

Member

Yes

@renekliment

This comment has been minimized.

Copy link
Contributor

renekliment commented Jul 25, 2018

@dg

Some old browsers (such as PhantomJS used for testing) do not support u modifier.

I see. I had thought that PhantomJS uses V8. Silly mistake. Thanks for the explanation.

I fixed it in e3b5529

Dope. I'll rebase on top of that.

Thank you.

@renekliment renekliment force-pushed the renekliment:pattern_icase branch from 46d72e9 to 7d632f4 Jul 26, 2018

@renekliment

This comment has been minimized.

Copy link
Contributor

renekliment commented Jul 27, 2018

@dg Rebased and adjusted. Let me know if you need anything else. Thank you.

@dg

This comment has been minimized.

Copy link
Member

dg commented Jul 27, 2018

Great, thanks!

@dg dg merged commit 9561dd8 into nette:master Jul 27, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.009%) to 95.034%
Details

dg added a commit that referenced this pull request Jul 27, 2018

@renekliment

This comment has been minimized.

Copy link
Contributor

renekliment commented Jul 27, 2018

@dg Dope! Thank you. If you could back-port it to v2.4 that would be even more awesome :-)

dg added a commit that referenced this pull request Jul 27, 2018

@dg

This comment has been minimized.

Copy link
Member

dg commented Jul 27, 2018

Done

@renekliment renekliment deleted the renekliment:pattern_icase branch Jul 27, 2018

renekliment added a commit to renekliment/nette-docs that referenced this pull request Aug 22, 2018

dg added a commit to nette/docs that referenced this pull request Aug 22, 2018

dg added a commit to nette/docs that referenced this pull request Aug 22, 2018

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