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

Should exceptions include prefixed source/destination paths? #1840

Open
ziadoz opened this issue Dec 19, 2024 · 0 comments
Open

Should exceptions include prefixed source/destination paths? #1840

ziadoz opened this issue Dec 19, 2024 · 0 comments

Comments

@ziadoz
Copy link

ziadoz commented Dec 19, 2024

Question

We're using Flysystem via Laravel (11.36.1), and a silly bug slipped into our codebase where we accidentally passed absolute paths to the move() method instead of relative ones.

This caused the underlying LocalFilesystemAdapter to throw an UnableToMoveFile exception, however the paths in the exception message aren't prefixed, which made the bug very difficult to discover.

The code is fairly simple:

// config/filesystems.php
/*
'foobar' => [
      'driver'      => 'local',
      'root'        => storage_path('filesystems/local/foobar'),
      'throw'       => true,
  ],
*/

// Our broken code...
Storage::disk('foobar')->move(
    '/srv/app/storage/filesystems/foobar/folder/foo.txt', 
    '/srv/app/storage/filesystems/foobar/folder/bar.txt',
);

This caused an exception:

  "exception": {
    "class": "League\\Flysystem\\UnableToMoveFile",
    "code": 0,
    "file": "/srv/app/vendor/league/flysystem/src/UnableToMoveFile.php:56",
    "message": "Unable to move file from srv/app/storage/filesystems/foobar/folder/foo.txt to srv/app/storage/filesystems/foobar/folder/bar.txt, because unknown reason",
    "trace": [...],
}

I suppose the lack of a / at the beginning of the filename possibly should have tipped us off, but at a glance the filenames in the exception looked fine without further digging.

It's only when I dug into the move() method and dumped out the $sourcePath and $destinationPath variables that I realised we were actually trying to move /srv/app/storage/filesystems/foobar/srv/app/storage/filesystems/foobar/folder/foo.txt to /srv/app/storage/filesystems/foobar/srv/app/storage/filesystems/foobar/folder/bar.txt, and Flysystem was unable to create all the sub-directories due to the weird paths. https://github.com/thephpleague/flysystem/blob/3.x/src/Local/LocalFilesystemAdapter.php#L256

Should the exceptions thrown by Flysystem include the prefixed source/destination paths? It seems like it could possibly give a more accurate idea of what operation was occurring when the error happened, which would be useful when debugging.

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

No branches or pull requests

1 participant