Skip to content

Conversation

@c0nst4ntin
Copy link
Contributor

With this Pull Request some of the capture groups that are not needed will be turned into non-capture groups.

From my research, I can't see any major performance difference in the two methods, but I guess it can't hurt to add this.

FYI: I tested this with the existing test in this repository but not in any real word projects or scenarios

@milwad-dev
Copy link
Owner

I can't understand what is the benefits of this changes?

@milwad-dev milwad-dev changed the title Remove unused capture groups [1.x] Remove unused capture groups Jun 11, 2023
@c0nst4ntin
Copy link
Contributor Author

In Regex, anything in brackets "()" is considered a capture group. This means that you can later reference the matched text using a backrefence like "<1>". By using?: you can specify that the group should only be used as a group and not capture the match.

Source: https://www.regular-expressions.info/refcapture.html

According to some threads, there is a slight performance difference in the usage (which would make sense to me as well):
https://stackoverflow.com/questions/33243292/capturing-group-vs-non-capturing-group

In theory, this should simply improve Regex performance (although it is probably only noticeable in very extreme cases).

Copy link
Owner

@milwad-dev milwad-dev left a comment

Choose a reason for hiding this comment

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

Just add this for another rules

@c0nst4ntin
Copy link
Contributor Author

Is there any further way I can assist?

In some classes this was already added: https://github.com/milwad-dev/laravel-validate/blob/1.x/src/Rules/ValidCamelCase.php#L18

In some it can't be added, as it needs to be a capture group: https://github.com/milwad-dev/laravel-validate/blob/1.x/src/Rules/ValidHtmlTag.php#L18

And in some cases, the regex was quite complex so I didn't want to make it even more complex: https://github.com/milwad-dev/laravel-validate/blob/1.x/src/Rules/ValidIpAddressIPV6.php#L18

@milwad-dev milwad-dev merged commit 633b6d8 into milwad-dev:1.x Jun 11, 2023
@milwad-dev
Copy link
Owner

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