Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(matches): prevent regex state from breaking following validations #1975

Merged
merged 1 commit into from
Jun 30, 2022

Conversation

fedeci
Copy link
Contributor

@fedeci fedeci commented May 25, 2022

Refs
express-validator/express-validator#1127
express-validator/express-validator#1150

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)

return pattern.test(str);
return !!str.match(pattern);
Copy link
Contributor Author

@fedeci fedeci May 25, 2022

Choose a reason for hiding this comment

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

.test keeps the state when used along with a regex that has the global or the sticky flag. We can prevent that behaviour getting stateless validation using .match

/cc @tux-tn @ezkemboi

Copy link

@tonysamperi tonysamperi May 30, 2022

Choose a reason for hiding this comment

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

What about using Array.isArray(str.match(pattern)) or str.match(pattern) !== null since the possible return values of .matches are Array or null?
The result is obviously the same, but semantically seems more appropriate to me :)
And maybe they'll be more inclined to accept the PR :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I honestly find that excessively verbose and probably won't be a blocker for the pr. Let's see what the maintainers think.

@codecov
Copy link

codecov bot commented May 25, 2022

Codecov Report

Merging #1975 (4fdebaf) into master (cfcf911) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #1975   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          103       103           
  Lines         2097      2097           
  Branches       473       473           
=========================================
  Hits          2097      2097           
Impacted Files Coverage Δ
src/lib/matches.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfcf911...4fdebaf. Read the comment docs.

Copy link
Member

@tux-tn tux-tn left a comment

Choose a reason for hiding this comment

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

Thank you @fedeci ! LGTM 🎉

@tux-tn tux-tn added the ready-to-land For PRs that are reviewed and ready to be landed label Jun 26, 2022
@fedeci
Copy link
Contributor Author

fedeci commented Jun 26, 2022

We have published an hot fix in express-validator so we are fine to have this merged later too!

@tux-tn
Copy link
Member

tux-tn commented Jun 26, 2022

Good for you, really sorry for the delay @fedeci !

Copy link
Member

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

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

Good catch, thanks @fedeci !

@profnandaa profnandaa merged commit 9a0bb35 into validatorjs:master Jun 30, 2022
@fedeci fedeci deleted the fix/regex-state branch June 30, 2022 09:01
@tonysamperi
Copy link

Good catch, thanks @fedeci !

@profnandaa Actually, I catched it! 😂😂😂

@profnandaa
Copy link
Member

Ah, thanks @tonysamperi :-D

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
ready-to-land For PRs that are reviewed and ready to be landed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants