Skip to content
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

[Finder] Glob wildcard while using double-star without ending slash #22239

Closed

Conversation

Projects
None yet
5 participants
@sroze
Copy link
Member

commented Apr 2, 2017

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets ø
License MIT
Doc PR ø

This PR allows to use the Glob::toRegex method to dump a glob that will match "everything after" while using the double-star without leading slash. /foo/** will therefore match /foo/a or /foo/a/b/c or 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.

$car = $strictLeadingDot ? '/(?:(?=[^\.])[^/]++/)*' : '/(?:[^/]++/)*';
$i += 3;
} elseif (isset($glob[$i + 2]) && '**' === $glob[$i + 1].$glob[$i + 2]) {
$car = '/.*';

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 3, 2017

Member

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.

This comment has been minimized.

Copy link
@sroze

sroze Apr 5, 2017

Author Member

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?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 5, 2017

Member

it already matches inside the line: that's the first case of the "if"
see https://github.com/symfony/symfony/pull/22239/files#r109962164

@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Apr 3, 2017

@sroze sroze force-pushed the sroze:glob-double-star-without-leading-slash branch from d5b703b to 94fe26a Apr 5, 2017

@sroze

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2017

@nicolas-grekas updated the PR and added this comment.

$car = $strictLeadingDot ? '/(?:(?=[^\.])[^/]++/)*' : '/(?:[^/]++/)*';
$i += 3;
} elseif (isset($glob[$i + 2]) && '**' === $glob[$i + 1].$glob[$i + 2]) {
$car = $strictLeadingDot ? '/(?=[^\.]).*' : '/.*';

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 5, 2017

Member

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])) {

This comment has been minimized.

Copy link
@sroze

sroze Apr 6, 2017

Author Member

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;

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 5, 2017

Member

well, two: $i += 2 + isset($glob[$i + 3]);

@sroze

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2017

@nicolas-grekas unfortunately your two-line change wasn't an option because of the / at the end. I've pushed a refactored version to allow this / not to be there when just using ** without / after.

$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 = '[^/]++/';

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 6, 2017

Member

would that work?
$car = $strictLeadingDot ? '(?:/(?=[^\.])[^/]*+)+' : '(?:/[^/]*+)+'

This comment has been minimized.

Copy link
@sroze

sroze Apr 6, 2017

Author Member

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 (like foo for foo/**)

This comment has been minimized.

Copy link
@sroze

sroze Apr 6, 2017

Author Member

I think this $car creation is also more readable.

@sroze

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2017

The built errored on Travis, could someone with super-powers could restart it? :)

@sstok

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2017

Amend your commit and then force push (with the branch name) and should restart Travis 👍

@sroze sroze force-pushed the sroze:glob-double-star-without-leading-slash branch from 9c63d42 to 2c3ef42 Apr 9, 2017

@sroze

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2017

All green 👍

@sroze sroze changed the title [Finder] Glob wildcard while using double-star without leading slash [Finder] Glob wildcard while using double-star without ending slash Apr 9, 2017

@nicolas-grekas
Copy link
Member

left a comment

👍
Status: reviewed

@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 10, 2017

Can you add a note in the CHANGELOG?

@sroze

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2017

@fabpot don't you think that @nicolas-grekas' one already for 3.3.0 is enough?

@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 10, 2017

Thank you @sroze.

@fabpot fabpot closed this in bbb0d5e Apr 10, 2017

@sroze sroze deleted the sroze:glob-double-star-without-leading-slash branch Apr 10, 2017

@fabpot fabpot referenced this pull request May 1, 2017

Merged

Release v3.3.0-BETA1 #22603

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.