10000 Issue 4911 fix by gajdaw · Pull Request #5128 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Issue 4911 fix #5128

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Issue 4911 fix #5128

wants to merge 1 commit into from

Conversation

gajdaw
Copy link
Contributor
@gajdaw gajdaw commented Jul 31, 2012

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: Build Status
Fixes the following tickets: #4911
Todo: [list of todos pending]
License of the code: MIT
Documentation PR:

Requires #4739

I followed @fabpot suggestion: #4932 (comment)

@stof
Copy link
Member
stof commented Jul 31, 2012

This PR needs to be rebased

@gajdaw
Copy link
Contributor Author
gajdaw commented Jul 31, 2012

@stof Done.

@travisbot
Copy link

This pull request fails (merged d1ea869c into b1618d2).

vicb
vicb reviewed Aug 6, 2012
View reviewed changes
src/Symfony/Component/Finder/Iterator/PathFilterIterator.php
*/
protected function toRegex($str)
{
if ($this->isRegex($str)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

return $this->isRegex($str) ? $str : '/'.preg_quote($str, '/').'/';

@vicb
Copy link
Contributor
vicb commented Aug 6, 2012

I think that $finder->path() could easily be confused with $finder->in().

Would $finder->pathContains()' &$finder->pathNotContains()' be better names ? (it would also be consistent with the current methods $finder->contains()' &$finder->notContains()'.

@gajdaw
Copy link
Contributor Author
gajdaw commented Aug 6, 2012

@vicb Thanks for your suggestion. I've changed names of path() and notPath() according to your post, but I left property names unchanged: Finder::paths, Finder::notPaths.

Do you think I should change them as well?

What do you think about filter name? Currently it is PathFilterIterator?

@travisbot
Copy link

This pull request passes (merged e5ce6206 into 842b599).

@stof
Copy link
Member
stof commented Oct 13, 2012

@gajdaw Can you update this PR with the glob support (as requested in #4932 which seems to be a subset of this PR) ?

@fabpot
Copy link
Member
fabpot commented Oct 30, 2012

Now that #4739 is merged, can you rebase this PR?

@fabpot fabpot mentioned this pull request Oct 30, 2012
Closed
@fabpot
Copy link
Member
fabpot commented Oct 30, 2012

@gajdaw Can you also work on another PR to implement glob support for the in() method? Thanks.

@gajdaw
Copy link
Contributor Author
gajdaw commented Oct 30, 2012

@fabpot PR is rebased and merged. The fix works fine for PhpAdapter only.

@jfsimon Can you take a look at this PR? I have added one file, i.e.:

$tmpDir.'/.foo/bar';

in RealIteratorTestCase setup method.

This is a test case for a bug suggested by stof in #4911.

I'm not sure but it seems that GnuFindAdapter should be adjusted.

This PR passes tests on Windows.

@stof stof mentioned this pull request Oct 30, 2012
Closed
@jfsimon jfsimon mentioned this pull request Feb 26, 2013
Closed
fabpot added a commit that referenced this pull request Feb 26, 2013
@fabpot
merged branch jfsimon/issue-4911 (PR #7188)
9c46043 
This PR was submitted for the master branch but it was merged into the 2.2 branch instead (closes #7188).

Commits
-------

3b2cb79 [Finder] fixed paths/notPaths regex for shell adapters
45b84c5 [Finder] fix issue 4911

Discussion
----------

[Finder] Fixes paths/notPaths issue with shell adapters

This PR is built upon #5128 from @gajdaw.
It fixes wrong handling of regex in `paths` and `notPaths` criteria by shell adapters.

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #4911, #5128
@fabpot
Copy link
Member
fabpot commented Feb 26, 2013

Closing in favor of #7188

@fabpot fabpot closed this Feb 26, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gajdaw @stof @travisbot @vicb @fabpot
0