Problem/Motivation
Currently whenever a file is written using the StreamWrapper it is copied to a file in the temporary directory and than uploaded. We could save disk space and IO for Drupal file operations by intercepting at the file_system service layer to intercept copy(), move() and moveUploadedFile() (we currently already do intercept to catch errors) and calling an s3 putObject() directly.
Steps to reproduce
Any file write such as copy() will duplicate.
Proposed resolution
Add file upload logic into S3fsFileService service decorator. Leave StreamWrapper logic for code that does not used the managed API.
Remaining tasks
User interface changes
None Expected.
API changes
Possible new functions to centralize duplicate code operations
Data model changes
None Expected.
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | decrease_resource_ut-3204635-11.patch | 1.95 KB | cmlara |
| #4 | interdiff-3204635-3-4.txt | 4.56 KB | cmlara |
| #4 | decrease_resource_ut-3204635-4.patch | 13.9 KB | cmlara |
| #3 | decrease_resource_ut-3204635-3.patch | 11.62 KB | cmlara |
Comments
Comment #2
cmlaraComment #3
cmlaraFirst round at this.
the new copyObject() function is being used only on the same StreamWrapper. This could be used cross schemes since we only support a single bucket at the moment, however I choose to write it assuming a StreamWrapper barrier between public/private/s3. Party for simplicity in the code, and partly because I don't know what a 4.x branch of this will look like and how viable cross bucket detection would be and I don't want to add a feature to 3.x that I may have to take away in 4.x.
Advantages over the previous code:
putObject reads the original file and directly uploads it without using a buffer from the AWS StreamWrapper (no need to create a duplicate copy on disk of a file) This will reduce peak disk space usage and reduce IOPS.
cobyObject: Allows the file to be copied or renamed (AWS does renames as a copy+unlink) on AWS without reading the source file, this will save download and upload bandwidth along with disk IOPS. php copy() is normally done as s stream read+write.
Comment #4
cmlaraSwitched to using $mimeGuesser to match mainline code, also added checks to use guessMimeType for D9.1 and above.
Adjusted for renamed hook_s3fs_copy_params_alter
Removed the streamWriteData function as its code is now handled by the new copy and move functions.
Comment #7
cmlaraComment #8
sara101 commentedWe are seeing issues with this latest update. Looks like there's a mismatch with the services and dependencies injected.
Error "PHP Fatal error: Uncaught TypeError: Argument 7 passed to Drupal\s3fs\S3fsFileService::__construct() must be an instance of Symfony\Component\Mime\MimeTypeGuesserInterface, instance of Drupal\Core\ProxyClass\File\MimeType\MimeTypeGuesser given, called in /opt/drupal/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 259 and defined in /opt/drupal/web/modules/contrib/s3fs/src/S3fsFileService.php:101"
s3fs.services.yml is including '@file.mime_type.guesser' in the arguments but S3fsFileService.php is injecting an instance of MimeTypeGuesserInterface $mimeGuesser
Comment #9
cmlara@Sara101
Drupal 8? If so I think I see what I did wrong in mixing up the old class vs the new class for the typehinting.
Giving it a deeper look still to see if I have a better class to pull off of for typehiting since I had missed the proxy class previously when looking.
Comment #10
sara101 commentedYes, using Drupal 8. It seems to work replacing the class Symfony\Component\Mime\MimeTypeGuesserInterface with Drupal\Core\ProxyClass\File\MimeType\MimeTypeGuesser. It supports the new methods in Drupal 9 https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21ProxyClas....
I did not test on Drupal 9 though.
Comment #11
cmlaraIt appears I did mix up Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface which was used in 8.9/9.0 (Symfony 4) and \Symfony\Component\Mime\MimeTypeGuesserInterface (Symfony 5) for the typehint base as the root fault for this and failed to run the tests against the D8.9 lab to verify.
The Symfony 4 version throws deprecation warnings in D9/Symfony 5 which isn't ideal and could break later if we keep it.
Double checked with core and the MimeType Guesser isn't actually passed in as a service anywhere in the code its always obtained from the container service so we do not have a well established base to run off of.
I don't believe \Drupal\Core\ProxyClass\File\MimeType\MimeTypeGuesser can be relied upon as a class for the typehinting as it isn't an interface its a specific implementation.
At this point I think the cleanest way for me to fix this is to just not typehint the variable at this time and wait until the module ends support for D8.9/9.1
Attached patch should fix this.
Comment #14
cmlara