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

getBasePath() returns incorrect value for bare stream wrapper path #65

Open
wants to merge 2 commits into
base: 4.8.x
Choose a base branch
from

Conversation

Crell
Copy link
Contributor

@Crell Crell commented Aug 13, 2024

I've run into a bug where trying to glob a stream path on just the root fails. Specifically, in my app, Glob::getBasePath('foo://'); returns foo:/, rather than foo://. That means when trying to Glob::glob('foo://*'), the base path is wrong, so it cannot find any files. Moreover, since PHP doesn't recognize foo:/ as a file stream, whatever stream wrapper is registered for foo gets ignored. (Specifically, the elseif (is_dir($basePath)) clause comes back false, because there's no stream wrapper for foo:/, only for foo://.)

I believe the attached PR fixes the issue, though I defer to the maintainers if they'd prefer to take a slightly different approach. I had to split the test cases up because presumably a path of '' is not supposed to be supported as it's not absolute.

I... do not understand why the tests are failing with a permission denied error on PHP 8.0 only. That feels spurious, but perhaps the maintainers have a better understanding of what gives.

@Crell
Copy link
Contributor Author

Crell commented Aug 14, 2024

OK, slight correction. This works for a root path, but it breaks a pattern like foo://bar.*. It does, however, work for foo:///bar.*. I'm not sure how to resolve that.

@Nek-
Copy link
Collaborator

Nek- commented Sep 6, 2024

Hello @Crell , can you add a test case for foo://bar.* ?

If it currently works for foo:///bar.*, I guess we need to ensure that it will continue to work as well...

Btw thanks for the report and the PR.

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