-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Filesystem] Safeguard (sym)link calls #46407
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
Conversation
aa47864
to
67df83c
Compare
This is trading an exception for another one, isn't it? Would this really solve the linked issue, if it's solvable? |
Yes, instead of a |
We could then throw a more specific exception, telling that symlink is disabled. |
Alternatively, we could replace the passed $symlink = static function($originDir, $targetDir) {
return symlink($originDir, $targetDir);
};
if (!self::box($symlink, $originDir, $targetDir)) {
$this->linkException($originDir, $targetDir, 'symbolic');
} This way, the user would get a fatal error |
Symlinking could fail for other reasons, so attempting the symlink and catching Symfony's IOException should be the proper workflow. Let's throw this exception also when the function is disabled. |
67df83c
to
9647d10
Compare
I've added a more specific error message. |
If we go with #46408, we should ensure that the behavior will be the same in both branches. |
Using We need to chose one from TypeError, Error or IOException :) |
I think we could remove the |
9647d10
to
17e612a
Compare
What I like about the current approach is that we'd do an early exit and skip the whole operation if the function is unavailable. |
Let's do both then: add the same check in box after changing its type hint, just in case any other methods are disabled? |
17e612a
to
c15028f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I've updated the patch to include my suggestion)
Agreed. 👍🏻 |
This PR let's link and symlink operations fail if the corresponding PHP functions are unavailable, e.g. because they have been disabled. Without the patch, the user receives a cryptic
TypeError
in such cases.I'm not really sure, if I can write tests for this scenario because we cannot disable those functions at runtime, can we?