-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add native copyObject API support for R2 disks
#3834
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
Open
kha333n
wants to merge
8
commits into
spatie:main
Choose a base branch
from
kha333n:feature/r2-native-copy-support
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
c2668e3
Add support for R2 disk optimized copy behavior
kha333n b237f4b
Refactor R2 disk configuration and improve code formatting
kha333n 6061928
Update R2 disk configuration to use environment variable for optimize…
kha333n f24d7ee
Add in docs R2 disk configuration option for optimized copy behavior
kha333n e389063
Moved forcing server side copy behaviour from Media Library config to…
kha333n 62bdbe6
Removed unnecessary formating
kha333n dc3e34d
Add unit tests for server-side copy behavior in R2 disk configuration
kha333n 0c83dac
Update installation-setup.md
freekmurze File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,204 @@ | ||
| <?php | ||
|
|
||
| use Mockery as m; | ||
| use Illuminate\Contracts\Filesystem\Factory; | ||
| use Spatie\MediaLibrary\MediaCollections\Filesystem; | ||
| use Spatie\MediaLibrary\MediaCollections\Models\Media; | ||
| use Spatie\MediaLibrary\Support\RemoteFile; | ||
|
|
||
| function callProtected(object $obj, string $method, array $args = []) | ||
| { | ||
| $ref = new ReflectionClass($obj); | ||
| $m = $ref->getMethod($method); | ||
| $m->setAccessible(true); | ||
| return $m->invokeArgs($obj, $args); | ||
| } | ||
|
|
||
| beforeEach(function () { | ||
| $base = config('filesystems.disks.s3_disk') ?? []; | ||
| config()->set('filesystems.disks.s3_disk', array_merge($base, [ | ||
| 'driver' => 's3', | ||
| 'force_server_copy' => false, | ||
| ])); | ||
| }); | ||
|
|
||
| afterEach(function () { | ||
| m::close(); | ||
| }); | ||
|
|
||
| it('returns true when force_server_copy is enabled (same disk, non-local, even with headers)', function () { | ||
| config()->set('filesystems.disks.s3_disk.force_server_copy', true); | ||
|
|
||
| $fs = m::mock(Factory::class); | ||
| $filesystem = new Filesystem($fs); | ||
|
|
||
| $file = m::mock(RemoteFile::class); | ||
| $file->shouldReceive('getDisk')->andReturn('s3_disk'); | ||
| $file->shouldReceive('getKey')->andReturn('source/key.jpg'); | ||
| $file->shouldReceive('getFilename')->andReturn('file.jpg'); | ||
|
|
||
| $media = m::mock(Media::class)->makePartial(); | ||
| $media->disk = 's3_disk'; | ||
| $media->shouldReceive('getDiskDriverName')->andReturn('s3'); | ||
| $media->shouldReceive('getConversionsDiskDriverName')->andReturn('s3'); | ||
| $media->shouldReceive('getCustomHeaders')->andReturn(['ACL' => 'public-read']); | ||
|
|
||
| $result = callProtected($filesystem, 'shouldCopyFileOnDisk', [$file, $media, 's3']); | ||
| expect($result)->toBeTrue(); | ||
| }); | ||
|
|
||
| it('returns false when force_server_copy is disabled and headers exist', function () { | ||
| $fs = m::mock(Factory::class); | ||
| $filesystem = new Filesystem($fs); | ||
|
|
||
| $file = m::mock(RemoteFile::class); | ||
| $file->shouldReceive('getDisk')->andReturn('s3_disk'); | ||
| $file->shouldReceive('getKey')->andReturn('source/key.jpg'); | ||
| $file->shouldReceive('getFilename')->andReturn('file.jpg'); | ||
|
|
||
| $media = m::mock(Media::class)->makePartial(); | ||
| $media->disk = 's3_disk'; | ||
| $media->shouldReceive('getDiskDriverName')->andReturn('s3'); | ||
| $media->shouldReceive('getConversionsDiskDriverName')->andReturn('s3'); | ||
| $media->shouldReceive('getCustomHeaders')->andReturn(['ACL' => 'public-read']); | ||
|
|
||
| $result = callProtected($filesystem, 'shouldCopyFileOnDisk', [$file, $media, 's3']); | ||
| expect($result)->toBeFalse(); | ||
| }); | ||
|
|
||
| it('returns false when source and destination disks differ, even if force_server_copy is enabled', function () { | ||
| config()->set('filesystems.disks.s3_disk.force_server_copy', true); | ||
|
|
||
| $fs = m::mock(Factory::class); | ||
| $filesystem = new Filesystem($fs); | ||
|
|
||
| $file = m::mock(RemoteFile::class); | ||
| $file->shouldReceive('getDisk')->andReturn('another_s3_disk'); | ||
| $file->shouldReceive('getKey')->andReturn('source/key.jpg'); | ||
| $file->shouldReceive('getFilename')->andReturn('file.jpg'); | ||
|
|
||
| $media = m::mock(Media::class)->makePartial(); | ||
| $media->disk = 's3_disk'; | ||
| $media->shouldReceive('getDiskDriverName')->andReturn('s3'); | ||
|
|
||
| $result = callProtected($filesystem, 'shouldCopyFileOnDisk', [$file, $media, 's3']); | ||
| expect($result)->toBeFalse(); | ||
| }); | ||
|
|
||
| it('returns true for local driver regardless', function () { | ||
| $fs = m::mock(Factory::class); | ||
| $filesystem = new Filesystem($fs); | ||
|
|
||
| $file = m::mock(RemoteFile::class); | ||
| $file->shouldReceive('getDisk')->andReturn('local_disk'); | ||
| $file->shouldReceive('getKey')->andReturn('source/key.jpg'); | ||
| $file->shouldReceive('getFilename')->andReturn('file.jpg'); | ||
|
|
||
| $media = m::mock(Media::class)->makePartial(); | ||
| $media->disk = 'local_disk'; | ||
| $media->shouldReceive('getDiskDriverName')->andReturn('local'); | ||
|
|
||
| $result = callProtected($filesystem, 'shouldCopyFileOnDisk', [$file, $media, 'local']); | ||
| expect($result)->toBeTrue(); | ||
| }); | ||
|
|
||
| it('performs server-side copy when force_server_copy is enabled on same disk', function () { | ||
| config()->set('filesystems.disks.s3_disk.force_server_copy', true); | ||
|
|
||
| $fs = m::mock(Factory::class); | ||
| $diskMock = m::mock(); | ||
|
|
||
| $fs->shouldReceive('disk')->with('s3_disk')->andReturn($diskMock); | ||
| $diskMock->shouldReceive('copy') | ||
| ->once() | ||
| ->with('source/key.jpg', m::on(fn ($dest) => str_ends_with($dest, '/file.jpg'))); | ||
| $diskMock->shouldNotReceive('getDriver'); | ||
|
|
||
| $filesystem = new class($fs) extends Filesystem { | ||
| public function getMediaDirectory(Media $media, ?string $type = null): string | ||
| { | ||
| return 'media/dir/'; | ||
| } | ||
| }; | ||
|
|
||
| $file = m::mock(RemoteFile::class); | ||
| $file->shouldReceive('getDisk')->andReturn('s3_disk'); | ||
| $file->shouldReceive('getKey')->andReturn('source/key.jpg'); | ||
| $file->shouldReceive('getFilename')->andReturn('file.jpg'); | ||
|
|
||
| $media = m::mock(Media::class)->makePartial(); | ||
| $media->disk = 's3_disk'; | ||
| $media->conversions_disk = 's3_disk'; | ||
| $media->shouldReceive('getDiskDriverName')->andReturn('s3'); | ||
| $media->shouldReceive('getConversionsDiskDriverName')->andReturn('s3'); | ||
| $media->shouldReceive('getCustomHeaders')->andReturn([]); | ||
|
|
||
| $filesystem->copyToMediaLibraryFromRemote($file, $media); | ||
| }); | ||
|
|
||
| it('streams when force_server_copy is disabled and headers exist', function () { | ||
| config()->set('filesystems.disks.s3_disk.force_server_copy', false); | ||
|
|
||
| $fs = m::mock(Factory::class); | ||
| $targetDisk = m::mock(); | ||
| $targetDriver = m::mock(); | ||
|
|
||
| $readStream = fopen('php://temp', 'w+'); | ||
| fwrite($readStream, 'payload'); | ||
| rewind($readStream); | ||
|
|
||
| // Subclass to avoid hitting Storage::disk(...)->getDriver()->readStream(...) | ||
| $filesystem = new class($fs, $readStream) extends Filesystem { | ||
| private $testStream; | ||
| public function __construct($fs, $stream) { parent::__construct($fs); $this->testStream = $stream; } | ||
| public function getMediaDirectory(\Spatie\MediaLibrary\MediaCollections\Models\Media $media, ?string $type = null): string | ||
| { | ||
| return 'media/dir/'; | ||
| } | ||
| public function copyToMediaLibraryFromRemote(\Spatie\MediaLibrary\Support\RemoteFile $file, \Spatie\MediaLibrary\MediaCollections\Models\Media $media, ?string $type = null, ?string $targetFileName = null): void | ||
| { | ||
| $destinationFileName = $targetFileName ?: $file->getFilename(); | ||
| $destination = $this->getMediaDirectory($media, $type) . $destinationFileName; | ||
|
|
||
| $diskDriverName = (in_array($type, ['conversions', 'responsiveImages'])) | ||
| ? $media->getConversionsDiskDriverName() | ||
| : $media->getDiskDriverName(); | ||
|
|
||
| // Assert branch: should be FALSE (streaming) | ||
| $shouldCopy = (function ($file, $media, $diskDriverName) { | ||
| $ref = new \ReflectionClass(\Spatie\MediaLibrary\MediaCollections\Filesystem::class); | ||
| $m = $ref->getMethod('shouldCopyFileOnDisk'); | ||
| $m->setAccessible(true); | ||
| return $m->invoke($this, $file, $media, $diskDriverName); | ||
| })($file, $media, $diskDriverName); | ||
|
|
||
| expect($shouldCopy)->toBeFalse(); | ||
|
|
||
| $headers = ['ContentType' => 'image/jpeg']; // minimal headers for writeStream | ||
| $this->streamFileToDisk($this->testStream, $destination, $media->disk, $headers); | ||
| } | ||
| }; | ||
|
|
||
| $fs->shouldReceive('disk')->with('s3_disk')->andReturn($targetDisk); | ||
| $targetDisk->shouldReceive('getDriver')->andReturn($targetDriver); | ||
| $targetDriver->shouldReceive('writeStream')->once(); | ||
|
|
||
| $file = m::mock(\Spatie\MediaLibrary\Support\RemoteFile::class); | ||
| $file->shouldReceive('getDisk')->andReturn('s3_disk'); // same disk to engage decision logic | ||
| $file->shouldReceive('getKey')->andReturn('source/key.jpg'); | ||
| $file->shouldReceive('getFilename')->andReturn('file.jpg'); | ||
|
|
||
| $media = m::mock(\Spatie\MediaLibrary\MediaCollections\Models\Media::class)->makePartial(); | ||
| $media->disk = 's3_disk'; | ||
| $media->conversions_disk = 's3_disk'; | ||
| $media->shouldReceive('getDiskDriverName')->andReturn('s3'); | ||
| $media->shouldReceive('getConversionsDiskDriverName')->andReturn('s3'); | ||
| $media->shouldReceive('getCustomHeaders')->andReturn(['ACL' => 'public-read']); | ||
|
|
||
| $filesystem->copyToMediaLibraryFromRemote($file, $media); | ||
|
|
||
| if (is_resource($readStream)) { | ||
| fclose($readStream); | ||
| } | ||
| }); | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Could require-dev our own spatie/invade package to do this.