-
Notifications
You must be signed in to change notification settings - Fork 11.8k
[12.x] Add Storage::at() that returns a object similar to Str::of() #58271
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
|
Adding new methods to the interface is a breaking change. |
|
You're absolutely right, and I appreciate you pointing that out! Adding a new method to the interface is indeed a breaking change for anyone implementing I naively assumed that would affect very few people in practice, but you're correct that it's still a BC break and we shouldn't take that lightly. Setting that concern aside, what do you think of the overall approach and the |
| namespace Illuminate\Contracts\Filesystem; | ||
|
|
||
| use Illuminate\Filesystem\StoragePath; | ||
|
|
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.
| /** | |
| * @method \Illuminate\Filesystem\StoragePath at(string $path) | |
| */ |
Use docblock instead adding breaking change method in a minor release.
|
Your example is slightly contrived as throw_unless(Storage::disk('s3')->exists('some/path.txt'));
Storage::disk('s3')->readStream('some/path.txt');
Storage::disk('s3')->size('some/path.txt');can be written as $disk = Storage::disk('s3');
throw_unless($disk->exists('some/path.txt'));
$disk->readStream('some/path.txt');
$disk->size('some/path.txt');which will result in a more fair comparison |
| protected string $path; | ||
|
|
||
| protected FilesystemContract|CloudFilesystemContract|FilesystemAdapter $filesystem; | ||
|
|
||
| public function __construct(string $path, FilesystemContract|CloudFilesystemContract|FilesystemAdapter $filesystem) | ||
| { | ||
| $this->path = $path; | ||
| $this->filesystem = $filesystem; | ||
| } |
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.
If you want, you can use constructor property promotion here:
| protected string $path; | |
| protected FilesystemContract|CloudFilesystemContract|FilesystemAdapter $filesystem; | |
| public function __construct(string $path, FilesystemContract|CloudFilesystemContract|FilesystemAdapter $filesystem) | |
| { | |
| $this->path = $path; | |
| $this->filesystem = $filesystem; | |
| } | |
| public function __construct( | |
| protected string $path, | |
| protected FilesystemContract|CloudFilesystemContract|FilesystemAdapter $filesystem | |
| ) { | |
| } |
| /** | ||
| * Get the contents of a file as decoded JSON. | ||
| */ | ||
| public function json(int $flags = 0): ?array |
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.
You can narrow this further down if you want
| public function json(int $flags = 0): ?array | |
| /** | |
| * @param int-mask<JSON_BIGINT_AS_STRING, JSON_INVALID_UTF8_IGNORE, JSON_INVALID_UTF8_SUBSTITUTE, JSON_OBJECT_AS_ARRAY, JSON_THROW_ON_ERROR> | |
| */ | |
| public function json(int $flags = 0): ?array |
| /** | ||
| * Get the file size. | ||
| */ | ||
| public function size(): int |
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 assume, you can safely do this here:
| public function size(): int | |
| /** | |
| * @return non-negative-int | |
| */ | |
| public function size(): int |
| /** | ||
| * Get the checksum for the file. | ||
| */ | ||
| public function checksum(array $options = []): string|false | ||
| { | ||
| if ($this->filesystem instanceof FilesystemAdapter) { | ||
| return $this->filesystem->checksum($this->path, $options); | ||
| } | ||
|
|
||
| return false; | ||
| } |
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.
Probably overkill but if the class was made generic, we could do the following here to help static analysis:
| /** | |
| * Get the checksum for the file. | |
| */ | |
| public function checksum(array $options = []): string|false | |
| { | |
| if ($this->filesystem instanceof FilesystemAdapter) { | |
| return $this->filesystem->checksum($this->path, $options); | |
| } | |
| return false; | |
| } | |
| /** | |
| * Get the checksum for the file. | |
| * | |
| * @return (TFilesystem is FilesystemAdapter ? string|false : false) | |
| */ | |
| public function checksum(array $options = []): string|false | |
| { | |
| if ($this->filesystem instanceof FilesystemAdapter) { | |
| return $this->filesystem->checksum($this->path, $options); | |
| } | |
| return false; | |
| } |
| * Write the contents of a file. | ||
| * | ||
| * @param \Psr\Http\Message\StreamInterface|\Illuminate\Http\File|\Illuminate\Http\UploadedFile|string|resource $contents | ||
| * @param mixed $options |
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.
We could try to narrow this a bit:
| * @param mixed $options | |
| * @param \Illuminate\Contracts\Filesystem\Filesystem::VISIBILITY_PUBLIC|\Illuminate\Contracts\Filesystem\Filesystem::VISIBILITY_PRIVATE|(array{visibility: \Illuminate\Contracts\Filesystem\Filesystem::VISIBILITY_PUBLIC|\Illuminate\Contracts\Filesystem\Filesystem::VISIBILITY_PRIVATE}&array<string, mixed>) $options |
or
| * @param mixed $options | |
| * @param \Illuminate\Contracts\Filesystem\Filesystem::VISIBILITY_*|(array{visibility: \Illuminate\Contracts\Filesystem\Filesystem::VISIBILITY_*}&array<string, mixed>) $options |
or
| * @param mixed $options | |
| * @template TVisibility \Illuminate\Contracts\Filesystem\Filesystem::VISIBILITY_PUBLIC|\Illuminate\Contracts\Filesystem\Filesystem::VISIBILITY_PRIVATE | |
| * @param TVisibility|(array{visibility: TVisibility}&array<string, mixed>) $options |
| /** | ||
| * Get the URL for the file. | ||
| */ | ||
| public function url(): string | ||
| { | ||
| if ($this->filesystem instanceof CloudFilesystemContract) { | ||
| return $this->filesystem->url($this->path); | ||
| } | ||
|
|
||
| throw new RuntimeException('This driver does not support retrieving URLs.'); | ||
| } |
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.
Same here with the generics
| /** | |
| * Get the URL for the file. | |
| */ | |
| public function url(): string | |
| { | |
| if ($this->filesystem instanceof CloudFilesystemContract) { | |
| return $this->filesystem->url($this->path); | |
| } | |
| throw new RuntimeException('This driver does not support retrieving URLs.'); | |
| } | |
| /** | |
| * Get the URL for the file. | |
| * | |
| * @return (TFilesystem is \Illuminate\Contracts\Filesystem\Cloud ? string : never) | |
| * @throws \RuntimeException | |
| */ | |
| public function url(): string | |
| { | |
| if ($this->filesystem instanceof CloudFilesystemContract) { | |
| return $this->filesystem->url($this->path); | |
| } | |
| throw new RuntimeException('This driver does not support retrieving URLs.'); | |
| } |
|
I think it's a kinda cool idea but not sure I want to introduce it / maintain it right now. Release it as a package! 🔥 |
This PR introduces a fluent wrapper for storage operations similar to
Str::of()- eliminating repetitive path parameters throughout your codebase.Motivation
Currently, every filesystem operation requires repeating the path:
With
Storage::at(), the path is provided once:Features
at()calls:$storage->at('dir1')->at('dir2')copyTo()andmoveTo()return new path instances for further operationsIlluminate\Contracts\Filesystem\Filesystemmethods supportedConditionable,Dumpable,Macroable,TappableExample
Implementation
StoragePathclass inIlluminate\Filesystemat()method toFilesystemAdapter,FilesystemManager, andStoragefacadeat()methodcopyTo()/moveTo()return new instances;copy()/move()return bool (for compatibility)Note
StoragePathwrapper is opt-in via theat()method.I appreciate feedback on the approach and am happy to adjust if needed (e.g., alternative naming, implementation details, or framework target version). I can provide comprehensive tests if this is considered for merge. :-)