[12.x] Add StorageUri value object for portable file references#58813
[12.x] Add StorageUri value object for portable file references#58813angus-mcritchie wants to merge 2 commits intolaravel:12.xfrom
StorageUri value object for portable file references#58813Conversation
|
Thanks for submitting a PR! Note that draft PRs are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface. Pull requests that are abandoned in draft may be closed due to inactivity. |
| return isset($value) ? StorageUri::parse($value) : null; | ||
| } | ||
|
|
||
| public function set($model, $key, $value, $attributes) |
There was a problem hiding this comment.
You could narrow the return type down the return type:
| public function set($model, $key, $value, $attributes) | |
| /** | |
| * @return ($value is \Illuminate\Support\StorageUri ? string : ($value is null ? null : string)) | |
| */ | |
| public function set($model, $key, $value, $attributes) |
or better
| public function set($model, $key, $value, $attributes) | |
| /** | |
| * @return string|null | |
| * @phpstan-return ($value is \Illuminate\Support\StorageUri ? string : ($value is null ? null : string)) | |
| */ | |
| public function set($model, $key, $value, $attributes) |
| * @param array|string|null $name | ||
| * @param array|string $options |
There was a problem hiding this comment.
Maybe, you can narrow the array down a bit:
| * @param array|string|null $name | |
| * @param array|string $options | |
| * @param array{disk: string, visibility: string}|string|null $name | |
| * @param array{disk: string, visibility: string}|string $options |
| * Create a streamed response for the file. | ||
| * | ||
| * @param string|null $name | ||
| * @param array $headers |
There was a problem hiding this comment.
This could be
| * @param array $headers | |
| * @param array<string, string $headers |
or at least
| * @param array $headers | |
| * @param array<string, mixed> $headers |
but maybe, this would be fine, too
| * @param array $headers | |
| * @param array<string, scalar> $headers |
| /** | ||
| * Get the instance as an array. | ||
| * | ||
| * @return array<string, string|null> |
There was a problem hiding this comment.
If you want, you can make it even more precise:
| * @return array<string, string|null> | |
| * @return array{disk: string|null, path: string} |
| /** | ||
| * Convert the object to its JSON representation. | ||
| * | ||
| * @param int $options |
There was a problem hiding this comment.
To narrow the range of expected integers, you can use the following:
| * @param int $options | |
| * @param int-mask<JSON_FORCE_OBJECT, JSON_HEX_QUOT, JSON_HEX_TAG, JSON_HEX_AMP, JSON_HEX_APOS, JSON_INVALID_UTF8_IGNORE, JSON_INVALID_UTF8_SUBSTITUTE, JSON_NUMERIC_CHECK, JSON_PARTIAL_OUTPUT_ON_ERROR, JSON_PRESERVE_ZERO_FRACTION, JSON_PRETTY_PRINT, JSON_UNESCAPED_LINE_TERMINATORS, JSON_UNESCAPED_SLASHES, JSON_UNESCAPED_UNICODE, JSON_THROW_ON_ERROR> $options |
| } | ||
|
|
||
| return new static($disk, $path); | ||
| } |
There was a problem hiding this comment.
Some would probably expect that they can pass in their S3 URL. While I agree, this shouldn't be possible in the parse() method, we could offer a separate method:
public static function fromUrl(string $url): static
{
foreach (config('filesystem.disks') as $disk => $config) {
if (array_key_exists('url', $config) && str_starts_with($url, $config['url'])) {
return new static($disk, substr($url, strlen($config['url'])));
}
}
throw new InvalidArgumentException('Passed URL does not match any configured disks');
}|
Great suggestions with narrowing the types @shaedrich , thank you! I'd like some indication from the maintainers that this PR is on the right track at a higher level before making further tweaks 👍 |
This PR introduces a
StorageUrivalue object that provides a portable, serializable way to reference files across different storage disks using a URI format:storage://disk/path/to/file.jpgas discussed here.There are some open questions at the bottom, but I am really looking to see if this is something @taylorotwell would be interested in merging before I go further with tests, API feedback and docs 😄
I used Opus 4.5 to generate most of this full disclaimer.
Motivation
Currently, storing file references in the database requires either:
This becomes problematic when:
The
StorageUrivalue object solves this by encapsulating both disk and path in a single, portable value.Usage Examples
Basic Construction
Eloquent Integration
File Uploads
Storage Facade Integration
Storage Operations (via StorageUri)
Email Attachments
Immutable Modifications
Serialization
API Summary
StorageUri
new StorageUri(?string $disk, string $path)StorageUri::make(string $path)StorageUri::onDisk(string $disk, string $path)StorageUri::parse(string $uri)StorageUri::of(string $uri)disk(): ?stringpath(): stringextension(): stringdirname(): stringbasename(): stringfilename(): stringwithDisk(?string $disk): selfwithPath(string $path): selftoUri(): stringtoArray(): arraytoJson(): stringexists(): boolget(): stringurl(): stringtemporaryUrl(DateTimeInterface $expiry): stringdownload(?string $name): StreamedResponsedelete(): boolNew Methods on Existing Classes
FilesystemAdapteruri(string $path): StorageUriUploadedFilestoreUri($path, $disk): StorageUriUploadedFilestoreUriAs($path, $name, $disk): StorageUriAttachmentfromStorageUri(StorageUri $uri)Zero Breaking Changes
This PR introduces only additive changes:
StorageUriclassAsStorageUricast (alias)My Assessment
Why I think this is worth doing
Open Questions
How should null disk URIs serialize?
storage:///path/to/file.jpg(triple slash)parse_url()returnsfalsefor this format, so it can't round-trip throughparse()storage://default/pathas a reserved keywordIs
StorageUrithe right name?StoragePath- emphasizes it's a path referenceFileReference- more genericStorageLocation- descriptive but longerWhen should null disk resolve to the default?
->url(),->exists(), etc.)