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

Fix directory detection through symlinks #127

Closed

Conversation

Voltra
Copy link

@Voltra Voltra commented Jan 31, 2025

Initially this PR (#125) was meant to correct an issue on Windows where symlink'd directories would be considered neither as a directory, nor a symlink.

It has since grown to ensure that symlink'd directories are properly exported on both Windows and UNIX platforms. This is important as php artisan storage:link creates such a symlink and users will very likely want to export it as if it was a regular directory.

On Windows, stepping into a debugger revealed that SplFileInfo::isDir(), SplFileInfo::isFile() and SplFileInfo::isLink() all returned false on that specific entry. On UNIX platforms, SplFileInfo::isDir() and SplFileInfo::isLink() both return true where SplFileInfo::isFile() returns false.

However, SplFileInfo::getRealPath() does yield the correct directory path (i.e. the one the symlink points to).

As such, this PR simply makes directory detection portable on all OS that could suffer from this specific issue without hindering the process for other types of files.

@Voltra
Copy link
Author

Voltra commented Jan 31, 2025

NB: I haven't taken the time to run the Windows-specific test on a Windows machine, but I've been using my PR as a local patch on a personal project without any issue this far

@freekmurze
Copy link
Member

I don't want to maintain this complexity in our package, so I'm going to pass on this for now and add a note in the readme that it will only work Unix systems.

@freekmurze freekmurze closed this Feb 3, 2025
@Voltra
Copy link
Author

Voltra commented Feb 3, 2025

I don't want to maintain this complexity in our package, so I'm going to pass on this for now and add a note in the readme that it will only work Unix systems.

With all due respect, as stated in the body of the PR, the package in its current state still fails to properly export UNIX symlinked directories

@Voltra
Copy link
Author

Voltra commented Feb 12, 2025

Something you can assert by running the test suite from the PR on an unpatched version of the package

@Voltra
Copy link
Author

Voltra commented Feb 12, 2025

I also feel a bit disrespected for being asked to write tests (on #125) to then have the fix dismissed here instead of dismissing it right away

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