Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Finder] Glob wildcard while using double-star without ending slash#22239
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| $car =$strictLeadingDot ?'/(?:(?=[^\.])[^/]++/)*' :'/(?:[^/]++/)*'; | ||
| $i +=3; | ||
| }elseif (isset($glob[$i +2]) &&'**' ===$glob[$i +1].$glob[$i +2]) { | ||
| $car ='/.*'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This should match only at the end of the line - not the case right now.
This should account for hidden "dot-dirs", as done by the regexp L65.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Agree with the hidden "dot-dirs". But should it actually match only at the end of the line? I think it'd be better if it can actually match inside the line, don't you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it already matches inside the line: that's the first case of the "if"
seehttps://github.com/symfony/symfony/pull/22239/files#r109962164
d5b703b to94fe26aComparesroze commentedApr 5, 2017
@nicolas-grekas updated the PR and addedthis comment. |
| $car =$strictLeadingDot ?'/(?:(?=[^\.])[^/]++/)*' :'/(?:[^/]++/)*'; | ||
| $i +=3; | ||
| }elseif (isset($glob[$i +2]) &&'**' ===$glob[$i +1].$glob[$i +2]) { | ||
| $car =$strictLeadingDot ?'/(?=[^\.]).*' :'/.*'; |
nicolas-grekasApr 5, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
to me, the rx here should be the same as before, meaning the patch could have just one line:if ($firstByte && $strictWildcardSlash && isset($glob[$i + 2]) && '**' === $glob[$i + 1].$glob[$i + 2] && (!isset($glob[$i + 3]) || '/' === $glob[$i + 3])) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It won't work because of the ending slash in the existing regex.
| $i +=3; | ||
| }elseif (isset($glob[$i +2]) &&'**' ===$glob[$i +1].$glob[$i +2]) { | ||
| $car =$strictLeadingDot ?'/(?=[^\.]).*' :'/.*'; | ||
| $i +=2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
well, two:$i += 2 + isset($glob[$i + 3]);
sroze commentedApr 6, 2017
@nicolas-grekas unfortunately your two-line change wasn't an option because of the |
| $car =$strictLeadingDot ?'/(?:(?=[^\.])[^/]++/)*' :'/(?:[^/]++/)*'; | ||
| $i +=3; | ||
| if ($firstByte &&$strictWildcardSlash &&isset($glob[$i +2]) &&'**' ===$glob[$i +1].$glob[$i +2] && (!isset($glob[$i +3]) ||'/' ===$glob[$i +3])) { | ||
| $car ='[^/]++/'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
would that work?$car = $strictLeadingDot ? '(?:/(?=[^\.])[^/]*+)+' : '(?:/[^/]*+)+'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nup, it's not. I tried to make it working with a one-liner like that but all my attempts were either not matching "deep children" (level + 1) or matching the folder without its slash (likefoo forfoo/**)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this$car creation is also more readable.
sroze commentedApr 6, 2017
The built errored on Travis, could someone with super-powers could restart it? :) |
sstok commentedApr 7, 2017
Amend your commit and then force push (with the branch name) and should restart Travis 👍 |
9c63d42 to2c3ef42Comparesroze commentedApr 9, 2017
All green 👍 |
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
Status: reviewed
fabpot commentedApr 10, 2017
Can you add a note in the CHANGELOG? |
sroze commentedApr 10, 2017
@fabpot don't you think that@nicolas-grekas' one already for 3.3.0 is enough? |
fabpot commentedApr 10, 2017
Thank you@sroze. |
This PR allows to use the
Glob::toRegexmethod to dump a glob that will match "everything after" while using the double-star without leading slash./foo/**will therefore match/foo/aor/foo/a/b/cor even/foo/a/b/c/d.e.The use-case for this change is an application allowing its users to match a list of files/folders from such glob pattern.
Happy to add a line in the CHANGELOG and documentation if you feel that would make sense.