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] adds a way to ignore AccessDeniedException #7518

Merged
merged 1 commit into from Apr 22, 2013

Conversation

jfsimon
Copy link
Contributor

@jfsimon jfsimon commented Mar 29, 2013

This PR adds a Finder::ignoreUnreadableDirs() method which tells Finder to ignore unreadable directories instead of throwing an AccessDeniedException.

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #6981

@jfsimon
Copy link
Contributor Author

jfsimon commented Mar 29, 2013

@vicb are you okay with the ignoreUnreadableDirs name?

*/
public function ignoreUnreadableDirs($ignore = true)
{
$this->ignoreUnreadableDirs = $ignore;
Copy link
Contributor

Choose a reason for hiding this comment

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

(Boolean) $ignore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

@vicb
Copy link
Contributor

vicb commented Mar 29, 2013

Does not sound 100% right to me but I can't find anything better. May be a native speaker could help here to know if this is good or come with a better name ?

@lazyhammer
Copy link
Contributor

adds a way to ignore AccessDeniedException

So maybe Inaccessible?

@staabm
Copy link
Contributor

staabm commented Mar 29, 2013

Maybe omitInaccesible? ignorePermissions?

@kingcrunch
Copy link
Contributor

Or the other way round, for example accessibleOnly, or readableOnly

@jfsimon
Copy link
Contributor Author

jfsimon commented Apr 11, 2013

@vicb do you have an idea regarding to above suggestions?

@vicb
Copy link
Contributor

vicb commented Apr 11, 2013

I let you pick the best. I think there must be a verb in the name, ie setAccessibleOnly.

I have one question that is somehow related to this PR: What happens with unreadable files ? Let's say that you are looking for a file which contains "<?php" but some of the file are not readable by the process which runs the Finder, what would happen, Would the AccessDeniedException been thrown ?

If this is not the case yet, the code should be updated (both native & php drivers).

There should also be a way to ignore those exceptions, ie ->ignoreUnreadable(Finder::FILE | Finder::DIRECTORY)

@jfsimon
Copy link
Contributor Author

jfsimon commented Apr 11, 2013

Go for setAccessibleOnly.

In the case of an unreadable file content lookup, the native adapter throws an AccessDeniedException (as this exception is thrown anytime stderr returns something), but in the case of the PHP adapter the file is just not matched if not readable. Changing this will be a BC break, but is required as the current behavior it's not consistent.

@vicb
Copy link
Contributor

vicb commented Apr 11, 2013

the PHP adapter the file is just not matched if not readable

why ?

Edit: ok, found why

fabpot added a commit that referenced this pull request Apr 22, 2013
This PR was merged into the master branch.

Discussion
----------

[Finder] adds a way to ignore AccessDeniedException

This PR adds a `Finder::ignoreUnreadableDirs()` method which tells `Finder` to ignore unreadable directories instead of throwing an `AccessDeniedException`.

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #6981

Commits
-------

a63b30b [Finder] added Finder::ignoreUnreadableDirs() method
@fabpot fabpot closed this Apr 22, 2013
@fabpot fabpot merged commit a63b30b into symfony:master Apr 22, 2013
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.

None yet

6 participants