[META] Unify file upload logic of REST and JSON:API

This was only postponed on #1927648: Allow creation of file entities from binary data via REST requests, which has landed.

From a JSON API perspective, what's most important to not duplicate is moving the file around, validating the file entity itself and handling streaming. What's less important is identifying the target entity type, entity and field. We have all that logic already.

Looking at the current REST resource plugin, it's only these lines that I wish were not in the service:

$filename = $this->validateAndParseContentDispositionHeader($request);
$field_definition = $this->validateAndLoadFieldDefinition($entity_type_id, $bundle, $field_name);
...
$file_data = fopen('php://input', 'rb');

The first two lines JSON API can handle easily. The third we could technically let the service handle, but that would reduce flexibility in the future. Passing a stream wrapper is easy enough. I imagine GraphQL would prefer to pass a stream along too.

Perhaps the "validate" portion of the first to methods could be kept as public static methods. The header parsing and field definition loading could be dropped and made implementation specific though.

@Wim Leers asked me to port some changes from #2958554-118: Allow creation of file entities from binary data via JSON API requests into this patch.

I don't think these issues/services actually overlap that much yet in reality, that one was thought as a replacement for *existing* functions while this is more about additional reusability on top of the existing API to avoid duplication between different API services.

Might actually make sense to have both, but we do need to have naming that makes sense.. that's currently more or less the only thing that these services seem to have in common ;)

Maybe this one could have a name that more reflects its use-case/purpose, something like StreamUploader maybe? And then this will likely use a bunch of services from the lower-level upload service the other issue is working on.

Idea: remove the interface, mark the class internal, commit it. That way, we can still refactor it in #3021652: Deprecate upload-related functions file.inc and move to a file upload service.

+1

+++ b/core/modules/file/src/FileUploaderInterface.php
@@ -0,0 +1,44 @@
+  /**
+   * Creates and validates a file entity for a file field from a file stream.
...
+   * @param string $filename
+   *   The name of the file.
...
+  public function handleFileUploadForField(FieldDefinitionInterface $field_definition, $filename, $stream, AccountInterface $owner);

After receiving a feature request in JSON:API to allow client-generated UUIDs for the file entity to be created, I think that only having $filename is too rigid.

I think $filename should be replaced by an $entity_values argument or that argument should be optional at the end.

If we mark this class internal, that suggestion can be incorporated in a followup.

1. +++ b/core/modules/file/file.services.yml
   @@ -4,3 +4,7 @@ services:
   +  file.uploader:
   +    class: Drupal\file\FileUploader
   +    arguments: ['@logger.channel.file', '@file_system', '@file.mime_type.guesser', '@token', '@lock', '@config.factory']
   

   Can this be private?

2. +++ b/core/modules/file/file.services.yml
   @@ -4,3 +4,7 @@ services:
   +  file.uploader:
   +    class: Drupal\file\FileUploader
   +    arguments: ['@logger.channel.file', '@file_system', '@file.mime_type.guesser', '@token', '@lock', '@config.factory']
   

   Should this be private too?

1. +++ b/core/modules/file/file.services.yml
   @@ -4,3 +4,7 @@ services:
   +
   +  file.uploader:
   +    class: Drupal\file\FileUploader
   +    arguments: ['@logger.channel.file', '@file_system', '@file.mime_type.guesser', '@token', '@lock', '@config.factory']
   

   I don't care that much about internal or not. What I do care about is a) where it lives and b) how it is called.

   Right now it is in file.module. I guess that is necessary because of the field dependency. I think the goal is that the service of the other module is in Drupal\Core\FileSystem and not have any file entity/field dependencies. At best some things that it does could be a separate service, but maybe we could consider to move parts to the new service when it exists.

   The other is naming, right now, this is called file.uploader. But it's not about any file, it's about files for file fields. Plus, @WimLeers confirmed in Slack that it is also only meant as a service for API implementations like REST, JSON-API, GraphQL and so on. And not a replacement for file_save_upload(), which is about form uploads.

   So I think this should have a name that reflects that.. FileFieldUploader maybe, possibly also include Api or Services or something like that?

2. +++ b/core/modules/file/src/FileUploader.php
   @@ -0,0 +1,477 @@
   +
   +    // Begin building file entity.
   +    $file = File::create([]);
   +    $file->setOwnerId($owner->id());
   

   This is a hidden \Drupal::service() dependency. If you want to make this thing unit-testable, you should inject the entity type manager and then do $this->entityTypeManager->getStorage('file')->create()

   Then we could possibly convert some of the exception tests we have for REST to unit tests for this class, so we don't have to have the same test coverage again for REST, JSON-API, .. Possibly at least some of it should probably be kept to assert how the specific format handles that. Which brings me nicely to my next point..

3. +++ b/core/modules/file/src/FileUploader.php
   @@ -0,0 +1,477 @@
   +    if (!file_unmanaged_move($temp_file_path, $file_uri, FILE_EXISTS_ERROR)) {
   +      throw new HttpException(500, 'Temporary file could not be moved to file location.');
   +    }
   

   This service is now directly tied to specific HTTP Responses/exceptions. That means it would be weird for JSON-API or GraphQL to catch HTTP exceptions and reformat these errors as a standard json-api error response or so.

   Maybe this service should throw generic exceptions instead. But then each implementation has to re-throw them, so not sure. I think in this case it might be OK, because the request itself is also format neutral and it's just a POST of the file + headers.

4. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
   @@ -137,33 +102,21 @@ class FileUploadResource extends ResourceBase {
   -  public function __construct(array $configuration, $plugin_id, $plugin_definition, $serializer_formats, LoggerInterface $logger, FileSystemInterface $file_system, EntityTypeManagerInterface $entity_type_manager, EntityFieldManagerInterface $entity_field_manager, AccountInterface $current_user, MimeTypeGuesserInterface $mime_type_guesser, Token $token, LockBackendInterface $lock, Config $system_file_config) {
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, $serializer_formats, LoggerInterface $logger, FileUploader $file_uploader, EntityTypeManagerInterface $entity_type_manager, EntityFieldManagerInterface $entity_field_manager, AccountInterface $current_user) {
        parent::__construct($configuration, $plugin_id, $plugin_definition, $serializer_formats, $logger);
   -    $this->fileSystem = $file_system;
   +    $this->fileUploader = $file_uploader;
   

   This would break subclasses. We're technically allowed to make such a change, but for example in my entity.manager issues, I've been very careful, with optional new arguments and BC to avoid breaking subclasses.

   Just pointing that out here, do with that information as you please ;)

2. I'm not saying it is a priority, but writing the code so that you *could* do it still makes sense. Otherwise why bother to inject anything? Sorry if I was unclear, what I meant is that File::create() is exactly the same as having a \Drupal::service() in your code. It's a nice helper for tests/non-oop code, but in services, we want to inject everything we can IMHO.

4. I know the current BC rules allow it, but we've been getting more strict about that recently because we have broken a lot of contrib/custom code over the last few minor releases due to constructor changes and it is annoying for contrib maintainers. I do plan to open an issue to improve the BC policy in that regard and also document safe(r) ways to override service/plugin constructors. Again, just pointing out that, depending on who is going to do the final review/commit, it might be put back to needs work. So it's a bit more code now, but one risk less that it won't be committed when it is RTBC ;)

I was pointed here by @gabesullice

Warning: #2492171: Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) will now conflict with this, since it, too, changes the FileUploadResource constructor.

Technically neither blocks the other, so I'm not postponing or making either a child of the other, but maybe it makes sense to get that in first, then make sure this plays nicely with it.

Cheers,
-Derek

Yeah, sorry. I totally feel your pain. I don't know what to do about it. I hereby promise to help make sure the new event dispatch gets properly moved around here in and the FileUploadResource constructor changes are properly merged. Whether that's re-rolling this once #2492171 lands, or helping to review your re-roll is TBD. ;)

Side note: I don't think anyone really reviews patches that are assigned to someone. That implies you're planning to do more work, so you might want to unassign yourself if that's not really true. However, it looks like this isn't "ready", and there's pending feedback that hasn't been incorporated. Maybe unassigned + NW is a more honest status for this? Or NW + assigned to you to indicate you're planning to incorporate the requested changes/fixes.

I do agree with #22.2: If you're going to DI at all, you might as well go all the way. File::create() isn't ideal in here. I also agree that integration tests for this are important, but it's good not to have hidden dependencies if they can be avoided.

Anyway, apologies for potentially further complicating your life. I was just trying to be responsible and handle the API-side at #2492171 ...

Cheers,
-Derek

p.s. Alternately, if by some miracle this gets RTBC and lands before #2492171: Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) does, I promise not to be mad at you that I'll have to re-roll #2492171 accordingly. ;)

+++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
@@ -164,6 +169,13 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
     $this->systemFileConfig = $system_file_config;
+    if ($file_field_uploader instanceof FileFieldUploader) {
+      $this->fileFieldUploader = $file_field_uploader;
+    }
+    else {
+      @trigger_error("The 'file.uploader.field' service must be passed to FileUploadResource::__construct(), it is required before Drupal 9.0.0. See https://www.drupal.org/project/drupal/issues/2940383.", E_USER_DEPRECATED);
+      $this->fileFieldUploader = \Drupal::service('file.uploader.field');
+    }
   }

Yes, the removed dependencies makes this really hard.

I think it is fine to remove them, as I wrote in my issue, even if "my" stricter rules are adopted, it's still a best-effort thing and we can make exceptions when we think the impact will be acceptable.

The only references to this class that I could with http://grep.xnddx.ru/search?text=FileUploadResource is jsonapi. That actually was useful because it highlights that the constructor of the new class has the wrong classname ;)

So lets clean this up..

I'd say the interdiff shows why this is a good decision in this case :)

Looks good to me, RTBC.

As far as related/overlapping issues go...

* #2244513: Move the unmanaged file APIs to the file_system service (file.inc) will conflict with this, because several of functions used here will be deprecated there and the issue does update them in the old class and their uses in the new one would trigger deprecation messages. I think it would make sense to get that in first unless it is set back to needs work as it has been @ RTBC for a week now and is a massive patch that will unblock at least 10 other issues for further cleanup around our file system API.
* #2492171: Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) has been set back a bit and I think it makes sense to get this one in first anyway. Then it won't have to be touched/refactored again here and it's already available then to jsonapi and other API modules.
* #3021652: Deprecate upload-related functions file.inc and move to a file upload service is waiting on the first issue to land and I'm not 100% sure yet how that will continue. I do believe there is a need for a separate file upload related services, or it could be merged into this. Still pretty much in flux, so definitely fine to wait until after this is in.

1. +++ b/core/modules/file/src/FileFieldUploader.php
   @@ -0,0 +1,488 @@
   +    // This will take care of altering $file_uri if a file already exists.
   +    file_unmanaged_prepare($temp_file_path, $file_uri);
   

   This changes file_uri, but leaves $prepared_filename as-is. Is that a good idea? Don't we want the resulting file entity to end up with the final filename (even with _2 if needed)?

2. +++ b/core/modules/file/src/FileFieldUploader.php
   @@ -0,0 +1,488 @@
   +    $file->setFilename($prepared_filename);
   

   Right here. Why not use the actual filename?

3. +++ b/core/modules/file/src/FileFieldUploader.php
   @@ -0,0 +1,488 @@
   +    return basename($filename);
   

   This gets all fubar with multibyte chars in your filename, as we painfully learned at #2492171: Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc). I believe you need to use the file_system service for this if you want it to handle mb char filenames. See #2492171-257: Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) for more.

4. +++ b/core/modules/file/src/FileFieldUploader.php
   @@ -0,0 +1,488 @@
   +    assert(is_null($entity) || $field_definition->getTargetEntityTypeId() === $entity->getEntityTypeId() && $field_definition->getTargetBundle() === $entity->bundle());
   

   we can call assert() in regular code like this? ;) It just throws an exception if it fails? Cool. ;)

5. +++ b/core/modules/file/src/FileFieldUploader.php
   @@ -0,0 +1,488 @@
   +    $entity_type_manager = \Drupal::entityTypeManager();
   

   \Drupal::sad(). I guess we can't use the DI'ed service here since this is a public static. :/

NW for point 3. Everything else is optional.

Also, this probably needs a change record to tell people about the new service, changes to the constructors that were modified, etc.

Otherwise, agreed this looks RTBC.

Thanks!
-Derek

Also, the summary is mostly empty or full of lies (API changes section). ;) That should get a refresh now that this is all decided and nearly done.

Thanks,
-Derek

Bold, RTBC'ing your own patch! :)

1-3: Hrm, okay, makes sense. Sure, we can fix 3 in the re-roll of #2492171: Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc). It's already fixed over there (with tests).

4. Yeah, I dig runtime assert(). My previous life was a C/C++ programmer. ;) I just hadn't noticed this in core (yet), and was happily surprised this was used. My "cool ;)" was in earnest, not sarcasm.

5. Hah, great. ;) Even better.

Interdiff looks peachy. +1 to RTBC!

Re: CR: sorry, I missed the @internal discussion for the new service. Makes sense. However, don't we at least want to warn people about this API change (which still isn't in the summary):

-  public function __construct(array $configuration, $plugin_id, $plugin_definition, $serializer_formats, LoggerInterface $logger, FileSystemInterface $file_system, EntityTypeManagerInterface $entity_type_manager, EntityFieldManagerInterface $entity_field_manager, AccountInterface $current_user, MimeTypeGuesserInterface $mime_type_guesser, Token $token, LockBackendInterface $lock, Config $system_file_config) {
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, $serializer_formats, LoggerInterface $logger, EntityTypeManagerInterface $entity_type_manager, EntityFieldManagerInterface $entity_field_manager, AccountInterface $current_user, FileFieldUploader $file_field_uploader) {

I know #3030640: [policy, documentation] Clarify Constructor BC policy and document best practices for subclassing other classes is still TBD, but isn't it kind to warn people about these sorts of changes in the meanwhile? Perhaps that's just noise, since presumably no one has extended this class. *shrug*

Thanks,
-Derek

Yes, I'm +1 to the constructor change. I just see people requiring CRs for basically everything, so I thought I'd make sure we were covered. But yeah, fine, I doubt anyone cares that this is changing. Maybe we can footnote it in the eventual CR when the service is no longer @internal (I guess over at #3021652: Deprecate upload-related functions file.inc and move to a file upload service).

Thanks,
-Derek

1. It's definitely the right way to go to try to share this functionality
2. I think it order to add this to the File module we need to add kernel / unit test coverage of this service to the File module - we shouldn't depend on the rest tests.
3. I'm making this needs review because @Berdir as File entity / system maintainer rtbc'd this but I've got quite a few concerns that this won't make it any easier to share functionality between the UI and API services and that concerns me - but maybe @Berdir can convince me I shouldn't be concerned.

4. +++ b/core/modules/file/src/FileFieldUploader.php
   @@ -0,0 +1,461 @@
   + * @internal
   + */
   +class FileFieldUploader {
   

   Let's document what @internal means here.

5. +++ b/core/modules/file/src/FileFieldUploader.php
   @@ -0,0 +1,461 @@
   +    $temp_file_path = $this->readStreamDataToFile($stream);
   

   I think it might be easier to share functionality between this service and the UI code - ie _file_save_upload_single() is the creation of the tmp file happened outside of the creation of the File entity - and then the method that creates the File Entity could take an SPL file info object and some way to change whether we call file_unmanaged_prepare or \Drupal\Core\File\FileSystem::moveUploadedFile()

6. +++ b/core/modules/file/src/FileFieldUploader.php
   @@ -0,0 +1,461 @@
   +    // Lock based on the prepared file URI.
   +    $lock_id = $this->generateLockIdFromFileUri($file_uri);
   

   Having a lock like this only used for API first based file uploads feels odd - we can race with UI as well right? Also I would have thought we'd take a lock before reading the stream to a temp file.

7. +++ b/core/modules/file/src/FileFieldUploader.php
   @@ -0,0 +1,461 @@
   +  /**
   +   * Validates and extracts the filename from the Content-Disposition header.
   +   *
   +   * @param \Symfony\Component\HttpFoundation\Request $request
   +   *   The request object.
   +   *
   +   * @return string
   +   *   The filename extracted from the header.
   +   *
   +   * @throws \Symfony\Component\HttpKernel\Exception\BadRequestHttpException
   +   *   Thrown when the 'Content-Disposition' request header is invalid.
   +   */
   +  public static function validateAndParseContentDispositionHeader(Request $request) {
   

   I'm pondering whether or not this really belongs on this service or does the code really belong somewhere else.

8. +++ b/core/modules/file/src/FileFieldUploader.php
   @@ -0,0 +1,461 @@
   +    // Make sure only the filename component is returned. Path information is
   +    // stripped as per https://tools.ietf.org/html/rfc6266#section-4.3.
   +    return basename($filename);
   

   This needs to use \Drupal\Core\File\FileSystem::basename() - atm this has a bug due to a PHP bug - see https://bugs.php.net/bug.php?id=77239 so the static-ness here is problematic.

9. +++ b/core/modules/file/src/FileFieldUploader.php
   @@ -0,0 +1,461 @@
   +  /**
   +   * Gets a standard file upload stream.
   +   *
   +   * @return resource
   +   *   A stream ready to be read. Do not forget to close the stream after use
   +   *   with fclose().
   +   */
   +  public static function getUploadStream() {
   +    return fopen('php://input', 'rb');
   +  }
   

   This feels a bit odd can't the implementors do this or why is it passable in in the first place.

Re: #41:
1. Agreed.
2. Agreed.
3. Seems like what #3021652: Deprecate upload-related functions file.inc and move to a file upload service is trying to solve, and this is the share-all-the-API-stuff side of life.
4. I think it means "we don't fully know WTF we're doing until #3021652 is done, stay tuned." ;)
5. Maybe.
6. Maybe.
7. Yeah, probably. I think that's a reflection of this service being API-centric, per #22.1.
8. I tried at #35.3 and @Wim pushed back. I guess your pushes are heavier than mine. ;)
9. Agreed. I didn't notice that before, good catch. That does seem weird.

Re #35.3 / #41.8 - I can accept not solving it here but putting that method in a static makes solving it properly harder - which is why I brought it up.

Re: #43 - excellent point! I missed that detail both in your comment and in the function signature. Definitely agree. I'll leave it to @Wim to reply on why this method has to be static and if it's feasible to move it into something that can rely on the injected file_system service, instead. It lives in the questionable validateAndParseContentDispositionHeader() method, which perhaps doesn't belong here at all (per #41.7).

Thanks,
-Derek

Re: #45: I'm satisfied. ;) Back to RTBC.

This will collide with #3032620: \Drupal\file\Plugin\rest\resource\FileUploadResource uses basename() when it needs to use the Drupal version which is also RTBC, but we can easily sort that out depending on the order of commits. ;)

This will also collide with #3032390: Add an event to sanitize filenames during upload, but that's still at NW, so I'm not afraid. We can trivially re-roll that once this lands. The collision is pretty small in scope.

Thanks,
-Derek

Confused bot. Requeued #45.

Needs a reroll because the file_system issue landed finally. Means having to call fewer functions :)

About the CR, I still think that it would be useful, mostly for the changes on the existing plugin, not just the constructor but also the other methods that were moved away/refactored. If someone *did* subclass it, then they'd need to update their code most likely.

I had to create a CR for https://www.drupal.org/node/3030634, which I still think was overkill and just generates noise, I think it's fair to expect one here too, it's not that much work and lowers the chance that it gets pushed back.

About the @internal, I'm still a bit on the fence about the approach. At least the argumentation on the class is pretty bogus, the referenced issue has no intention of introducing any conceptual changes to how file uploads works, all it aims to do is move a bunch of functions to a service so we can remove file.inc. If anything is still in flux/internal, then it is your own API :)

Yay! The tests in #3032620: \Drupal\file\Plugin\rest\resource\FileUploadResource uses basename() when it needs to use the Drupal version caught your rebase mistake. ;) You're still calling basename() directly.

A bit more careful review this time...

1. +++ b/core/modules/file/src/FileFieldUploader.php
   @@ -0,0 +1,465 @@
   +class FileFieldUploader {
   

   Given that everything about this seems to assume services / API, I still wonder if this class should be named accordingly. If it's not really viable to use for the non-API case, we should say so. If it *is* viable, it'd be lovely to get an @see to #3021652: Deprecate upload-related functions file.inc and move to a file upload service or something.

2. +++ b/core/modules/file/src/FileFieldUploader.php
   @@ -0,0 +1,465 @@
   +      throw new HttpException(503, sprintf('File "%s" is already locked for writing.'), NULL, ['Retry-After' => 1]);
   

   Don't we need to do something to cleanup the $temp_file_path before throwing an Exception?

3. +++ b/core/modules/file/src/FileFieldUploader.php
   @@ -0,0 +1,465 @@
   +      throw new HttpException(500, 'Temporary file could not be moved to file location');
   

   And here, shouldn't we try to purge it, even if we couldn't move it? The most likely source of failure here would be at the new location, but we don't want to leave this file sitting around awaiting eventual temp purging.

   And, don't we also want to release the file lock right here before we throw the Exception?

4. +++ b/core/modules/file/src/FileFieldUploader.php
   @@ -0,0 +1,465 @@
   +          // Close the file streams.
   +          fclose($temp_file);
   

   The comment says "Close the file streams", but we only close 1 thing here. Seems we either want to also fclose($stream) here, or change the comment.

5. +++ b/core/modules/file/src/FileFieldUploader.php
   @@ -0,0 +1,465 @@
   +          // Close the file streams.
   +          fclose($temp_file);
   

   Ditto.

6. +++ b/core/modules/file/src/FileFieldUploader.php
   @@ -0,0 +1,465 @@
   +      // Close the file streams.
   +      fclose($temp_file);
   

   if fopen() returned NULL and we're inside the else from testing if($temp_file), seems like calling fclose($temp_file) is a bad idea.

   And ditto the concern about a plural comment with a single fclose().

7. +++ b/core/modules/file/src/FileFieldUploader.php
   @@ -0,0 +1,465 @@
   +    return 'file:rest:' . Crypt::hashBase64($file_uri);
   

   Does this lock ID really want ":rest:" in the name if the intention is to share this across multiple APIs?

8. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
   @@ -220,166 +154,28 @@ public function permissions() {
   +    $stream = FileFieldUploader::getUploadStream();
   +    $file = $this->fileFieldUploader->handleFileUploadForField($field_definition, $filename, $stream, $this->currentUser);
   +    fclose($stream);
   

   handleFileUploadForField() can throw exceptions without closing the stream. We should try/catch here so we definitely fclose($stream) in case of trouble, otherwise seem we're just leaking FDs.

9. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
   @@ -220,166 +154,28 @@ public function permissions() {
   -          // Close the file streams.
   -          fclose($temp_file);
   -          fclose($file_data);
   

   Yeah, the code we're replacing does two fclose()'s at this spot...

This issue was discussed at length by @Wim Leers, @Berdir, @dww and myself and I promised I would post my thoughts on this issue to recap my points.

• Adding an @internal service to File module without having standalone testing doesn't feel like the way forward because once this code is in the File module it has to stand on its own two feet.
• This service needs to be shareable by all things uploading files. This is a security critical area and Drupal should make it easy to do at the moment there are too many entry and exit points.
• Ideally we'd be able to create this service before JSON API is in core but the desire to get that in 8.7.0 and do this issue properly so the code can be shared by Rest, JSON API, and the UI is not compatible.
• Therefore we are better putting JSON API in as is and then doing further refactors to bring all the different parts into line.

Once JSON API has landed we should re-scope this issue and/or #3021652: Deprecate upload-related functions file.inc and move to a file upload service to be about providing a secure easy to use file upload service that can be leveraged core, contrib and custom for the various use-cases. As @Berdir pointed out with JSON API in core and its own set of methods calling low level file commands we've hit the rule of three.

For the record, I'm -1 to the strategy from #60.

a) That means the JSON:API version of this code is still buggy and needs work: #2843147-145: Add JSON:API to core as a stable module

b) That means yet more code paths we have to touch for #3032390: Add an event to sanitize filenames during upload and related upload hardening / cleanup.

I'd say:

c) Yet more deprecation hoop jumping as we try to merge/replace them all, but at least that one is mitigated by "hopefully less deprecation hoop jumping" if there are significant changes to the service once we carefully consider the UI/form case. ;)

Still although this perhaps makes things easier for the clean solution in 8.8.0, it makes things worse for 8.7.0. Given our general deprecation and API change frenzy during minor releases, I don't know that the benefits of waiting for "perfect" outweigh the costs. Hence, -1.

But (thankfully!) this isn't my decision to make. :) I defer to @alexpott.

But meanwhile, #2843147: Add JSON:API to core as a stable module is (should be) still blocked on fixing Drupal\jsonapi\ForwardCompatibility\FileFieldUploader.

FYI: #62.a is now at #3035866: Update \Drupal\jsonapi\ForwardCompatibility\FileFieldUploader to be in sync with \Drupal\file\Plugin\rest\resource\FileUploadResource

Re: #64 thanks for posting that for posterity, @Wim Leers! Definitely helpful to save conversations like that in the issue, both so they're not lost/forgotten, and to be more accessible to other contributors and folks not constantly on Slack. @Wim Leers++ ;)

What is this issue currently postponed on? I've had a read through and can't quite work it out, sorry.

While I'm not overly familiar with the internal workings of Drupal's file management, I have been a regular consumer of related APIs. A unified file management service would greatly simplify a lot of things we are trying to achieve.

@imclean: re: #68:

Ugh, whoops. We all totally forgot about this. :( It shouldn't be postponed anymore. It just needs work. However, I suspect we've missed all kinds of deadlines to do this properly for 8.9.x, and it's likely to be 9.1.x material at this point. But maybe @alexpott knows better so I'll leave the version alone for now.

Thanks/sorry,
-Derek

Thanks for the update Derek. Is there an overview somewhere of the various levels of file management and operations required? I see a lot of duplicated code in various parts of core an contrib (e.g. if file exists create new entity or reuse existing?).

Just quickly, here are the levels and choices I've come across recently, from low to high level.

"Physical" File

Existing filename

• Rename
• Replace
• Error

File Entity

Existing filename

• Replace
• Rename
• Error

Media Entity

Existing File Entity Reference

• Reuse
• Create new

Existing Filename

• Replace
• Rename
• Error

Node/other Entity

Existing Media Entity Reference

• Reuse
• Create new

Existing Filename

• Replace
• Rename
• Error

+1 for this effort! I'm working on trying to get the Feeds module capable of importing a remote file found in some external feed into a file field. I found all the complex code that both REST and JSON API are performing to handle this similar task and agree it would be wonderful to have some service that handles it all.

While I ported the file name security fix to graphql module I found a bug in FileUploadResource: #3184974: FileUploadResource does not release lock on validation errors.

So yep, would be really good to have a central core service.

@alexpott opened #3199264: Create a file upload service and replace the guts of \Drupal\jsonapi\Controller\TemporaryJsonapiFileFieldUploader, \Drupal\file\Plugin\rest\resource\FileUploadResource and _file_save_upload_single() as a critical task, which I just marked duplicate with this. But I'm bumping the prio here accordingly.

Thanks,
-Derek

This needed a serious reroll, the last commit I found that the patch in #58 applied to was 0b6e897

There were patch breaking changes in

• 04343d7 #3036197: REST FileUploadResource::streamUploadData() can call fclose(FALSE)
• 55c96a6 #3000057: Deprecate drupal_set_time_limit() and file_upload_max_size() and move to Environment component
• ae58cf0 #3069046: Properly manage newly required parameter of FileUsageBase::__construct()
• 9aa7d13 #3023015: Replace usages of deprecated constants in file.inc
• cc564e2 #3121362: Fix duplicate word typos (the the, to to, etc.) for code comments
• 7567d54 #3142934: Replace \Drupal\Component\Utility\Bytes::toInt() with \Drupal\Component\Utility\Bytes::toNumber() due to return type
• c8cc155 #3055193: [Symfony 5] The "Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesser" class is deprecated since Symfony 4.3, use "Symfony\Component\Mime\MimeTypes" instead.
• 1994ca5 SA-CORE-2020-012
• fd1d675 #2863652: Do not allow files with "php|pl|py|cgi|asp|js" extensions to be renamed to *.txt and be uploaded if *.txt is not allowed by the field widget
• 69a0a83 #3032390: Add an event to sanitize filenames during upload

I tried to keep as close as possible to the original patch while adding in the changes to FileUploadResource over the past 2 years.

All 44 functional tests that use FileUploadResourceTestBase passed for me locally, we'll see what testbot finds! 🤞

Bravo! 👏👏👏

@zrpnr: Fantastic work, thanks for bringing this forward!

Since you based your work on #58, most of my points from review #59 are not fixed. ;) But I can't blame you for that. You're "just" rerolling this beast after a ton of changes to all the related code. So here's a fresh review on #79:

1. +++ b/core/modules/file/file.services.yml
   @@ -4,3 +4,7 @@ services:
   +  file.uploader.field:
   +    class: Drupal\file\FileFieldUploader
   

   Probably doesn't matter, but it seems slightly weird that the class name is FileFieldUploader while the service ID is file.uploader.field. Shouldn't they agree? Can the service ID be file.field.uploader ?

2. +++ b/core/modules/file/file.services.yml
   @@ -4,3 +4,7 @@ services:
   +    arguments: ['@entity_type.manager', '@logger.channel.file', '@file_system', '@file.mime_type.guesser', '@token', '@lock', '@event_dispatcher']
   

   As a brand new service, this is our chance to put these dependencies in a "nice" order. Do we care? Is there any rhyme / reason for the current order?

3. +++ b/core/modules/file/src/FileFieldUploader.php
   @@ -0,0 +1,449 @@
   +use Drupal\rest\Plugin\rest\resource\EntityResourceValidationTrait;
   ...
   +  use EntityResourceValidationTrait {
   +    validate as resourceValidate;
   +  }
   

   Is any of this REST stuff actually used in here? I'm not seeing it. If so, seems like a poor separation of responsibilities. I'd think something generic here in \Drupal\File wouldn't/shouldn't know or care about anything REST-specific.

   Update: now that I got through the whole patch, I see the call in protected function validate()

       $this->resourceValidate($file);
   

   Yuck, I don't think we want that. Not entirely sure the best way to refactor this. Maybe Drupal\file\Plugin\rest\resource\FileUploadResource::post() can still worry about this? I'd have to think more about a truly clean solution, but after 7pm on a Friday night is not a good time for that. ;)

4. +++ b/core/modules/file/src/FileFieldUploader.php
   @@ -0,0 +1,449 @@
   +    if (!$event_dispatcher) {
   +      @trigger_error('The event dispatcher service should be passed to FileUploadResource::__construct() since 9.2.0. This will be required in Drupal 10.0.0. See https://www.drupal.org/node/3032541', E_USER_DEPRECATED);
   +      $event_dispatcher = \Drupal::service('event_dispatcher');
   +    }
   

   This is all new code. How could anyone be calling it without the event dispatcher? I think this can all be removed. It's got the wrong class name in the deprecation message, so it looks like this was copied in the (heroic!) reroll from somewhere else that does need it, but put here where it doesn't belong.

5. +++ b/core/modules/file/src/FileFieldUploader.php
   @@ -0,0 +1,449 @@
   +   * @return \Drupal\file\FileInterface|\Drupal\Core\Entity\EntityConstraintViolationListInterface
   +   *   The newly uploaded file entity, or a list of validation constraint
   +   *   violations
   ...
   +    // Validate the file entity against entity-level validation and field-level
   +    // validators.
   +    $this->validate($file, $validators);
   

   The @return says we'll return a list of validation errors (if any). But the code no longer does that. I think we lost saving the return value from validate() during the reroll. I think we should keep the documented behavior, and get the code back to agree.

6. +++ b/core/modules/file/src/FileFieldUploader.php
   @@ -0,0 +1,449 @@
   +      throw new HttpException(503, sprintf('File "%s" is already locked for writing.'), NULL, ['Retry-After' => 1]);
   

   We've got a %s in the sprintf(), but no value to replace it with. Either this doesn't need a sprintf() at all, or we should pass the filename.

7. +++ b/core/modules/file/src/FileFieldUploader.php
   @@ -0,0 +1,449 @@
   +    // Begin building file entity.
   

   // Begin building the file entity.

8. +++ b/core/modules/file/src/FileFieldUploader.php
   @@ -0,0 +1,449 @@
   +    else {
   +      $file->setMimeType($this->mimeTypeGuesser->guess($prepared_filename));
   +      @trigger_error('\Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Implement \Symfony\Component\Mime\MimeTypeGuesserInterface instead. See https://www.drupal.org/node/3133341', E_USER_DEPRECATED);
   +    }
   

   Again, as new code, can this ever happen here?

9. +++ b/core/modules/file/src/FileFieldUploader.php
   @@ -0,0 +1,449 @@
   +      throw new HttpException(500, 'Temporary file could not be moved to file location');
   

   Would:

   "Temporary file could not be moved to permanent location"

   be more clear here? I know it's not really "permanent", either. ;) But as written, this is a little confusing.

10. +++ b/core/modules/file/src/FileFieldUploader.php
    @@ -0,0 +1,449 @@
    +    // First, check the header exists.
    

    // First, check that the header exists.

11. +++ b/core/modules/file/src/FileFieldUploader.php
    @@ -0,0 +1,449 @@
    +      throw new BadRequestHttpException('"Content-Disposition" header is required. A file name in the format "filename=FILENAME" must be provided.');
    ...
    +      throw new BadRequestHttpException('No filename found in "Content-Disposition" header. A file name in the format "filename=FILENAME" must be provided.');
    ...
    +      throw new BadRequestHttpException('The extended "filename*" format is currently not supported in the "Content-Disposition" header.');
    

    I think we should consistently use 'the "Content-Disposition" header' in all these exceptions. The last one does it that way. The first two do not.

12. +++ b/core/modules/file/src/FileFieldUploader.php
    @@ -0,0 +1,449 @@
    +    // @see \Drupal\file\Plugin\rest\resource\FileUploadResource::validate()
    

    Not sure why we're @see'ing to a rest thing here. Don't we want to @see self::validate() instead?

13. +++ b/core/modules/file/src/FileFieldUploader.php
    @@ -0,0 +1,449 @@
    +  /**
    +   * Gets a standard file upload stream.
    +   *
    +   * @return resource
    +   *   A stream ready to be read. Do not forget to close the stream after use
    +   *   with fclose().
    +   */
    +  public static function getUploadStream() {
    +    return fopen('php://input', 'rb');
    +  }
    
    +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
    @@ -238,172 +149,30 @@ public function permissions() {
    -  protected function streamUploadData() {
    -    // 'rb' is needed so reading works correctly on Windows environments too.
    -    $file_data = fopen('php://input', 'rb');
    

    I don't understand why this static method is here. Not everything trying to upload a file necessarily wants to use php://input as the upload stream, right? If that's what callers want, seems like it should be their problem, not the responsibility of this class.

    If we are keeping it here, we're losing the comment about 'rb' being required on Windoze machines...

14. +++ b/core/modules/file/src/FileFieldUploader.php
    @@ -0,0 +1,449 @@
    +   * Reads file upload data to temporary file and moves to file destination.
    

    a) "Reads file upload data to a temporary file."

    b) This method doesn't "moves to file destination", so let's end the comment after "temporary file."

15. +++ b/core/modules/file/src/FileFieldUploader.php
    @@ -0,0 +1,449 @@
    +  protected function readStreamDataToFile($stream) {
    

    Can we type hint $stream as resource?

16. +++ b/core/modules/file/src/FileFieldUploader.php
    @@ -0,0 +1,449 @@
    +          // Close the file streams.
    ...
    +          // Close the file streams.
    

    We're only closing a single stream, the temp file. Using "streams" is confusing here, since it sort of implies we're closing the $stream we were passed in, which we definitely don't do. I think both of these should say:

    // Close the temp file stream.

    See #59.4 and 59.5. ;)

17. +++ b/core/modules/file/src/FileFieldUploader.php
    @@ -0,0 +1,449 @@
    +      // Close the input file stream since we can't proceed with the upload.
    

    Not true. This comment should be removed. The input file stream is now the caller's responsibility as documented. ;)

18. +++ b/core/modules/file/src/FileFieldUploader.php
    @@ -0,0 +1,449 @@
    +   * @param array $validators
    ...
    +  protected function validate(FileInterface $file, array $validators) {
    

    Is there a fancier / more precise type we can use for these, not just 'array'?

19. +++ b/core/modules/file/src/FileFieldUploader.php
    @@ -0,0 +1,449 @@
    +   * @return \Drupal\Core\Entity\EntityConstraintViolationListInterface
    +   *   The list of constraint violations, if any.
    ...
    +    if (!empty($errors)) {
    +      $message = "Unprocessable Entity: file validation failed.\n";
    +      $message .= implode("\n", array_map(function ($error) {
    +        return PlainTextOutput::renderFromHtml($error);
    +      }, $errors));
    +
    +      throw new UnprocessableEntityHttpException($message);
    +    }
    

    The docs don't match the code. Docs say we return a list of constraint violations. Code is to throw an exception that unpacks all the violations into a single string. Seems like the documentation would be a better API than the current implementation. ;)

20. +++ b/core/modules/file/src/FileFieldUploader.php
    @@ -0,0 +1,449 @@
    +   * Prepares the filename to strip out any malicious extensions.
    

    The new FileUploadSanitizeNameEvent can do a lot more than this. Maybe:

    "Prepares the filename for safe use on the site."

    ?

    And, we should probably add:

    @see \Drupal\Core\File\Event\FileUploadSanitizeNameEvent
    

21. +++ b/core/modules/file/src/FileFieldUploader.php
    @@ -0,0 +1,449 @@
    +  protected function prepareFilename($filename, array &$validators) {
    

    We do not want to pass $validators by reference. We're not modifying the array (and shouldn't be).

22. +++ b/core/modules/file/src/FileFieldUploader.php
    @@ -0,0 +1,449 @@
    +    // The actual extension validation occurs in
    +    // \Drupal\file\FileFieldUploader::validate().
    

    // The actual extension validation occurs in self::validate().

23. +++ b/core/modules/file/src/FileFieldUploader.php
    @@ -0,0 +1,449 @@
    +   *   An array suitable for passing to file_save_upload() or the file field
    +   *   element's '#upload_validators' property.
    

    Aren't we trying to kill file_save_upload()? ;)

24. +++ b/core/modules/file/src/FileFieldUploader.php
    @@ -0,0 +1,449 @@
    +    // Cap the upload size according to the PHP limit.
    ...
    +
    +    // There is always a file size limit due to the PHP server limit.
    

    These comments (and the newline between them) make it seem like we're dealing with separate things here. We're not. It's all 1 thing.

    Also, it's not just "according to the PHP limit". It's the min() of PHP's limit and the field setting.

    So maybe ditch the 2nd comment and newline, and use this for the 1st comment:

    // Cap the upload size according to the PHP limit and field settings (if any).

    Or something...

25. +++ b/core/modules/file/src/FileFieldUploader.php
    index d46a8b20b0..5860cbb76c 100644
    --- a/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
    
    --- a/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
    +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
    

    This is great progress. We've moved a bunch of stuff out of RESTlandia into something generic.

    But the scope of the issue is to unify this with the regular file UI and JSON:API. Seems like none of that is here. See the plan from the end of #60. Now that JSON:API is in core, we should see a similarly huge # of lines removed from core/modules/jsonapi somewhere... ;)

    Ditto, I believe the goal of this issue is also to decimate _file_save_upload_single() and move most/all of that into this service.

    So _file_save_upload_single() + JSON:API + REST = "the rule of 3" mentioned in #60.

26. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
    @@ -148,40 +83,21 @@ class FileUploadResource extends ResourceBase {
    -  public function __construct(array $configuration, $plugin_id, $plugin_definition, $serializer_formats, LoggerInterface $logger, FileSystemInterface $file_system, EntityTypeManagerInterface $entity_type_manager, EntityFieldManagerInterface $entity_field_manager, AccountInterface $current_user, $mime_type_guesser, Token $token, LockBackendInterface $lock, Config $system_file_config, EventDispatcherInterface $event_dispatcher = NULL) {
    +  public function __construct(array $configuration, $plugin_id, $plugin_definition, array $serializer_formats, LoggerInterface $logger, EntityTypeManagerInterface $entity_type_manager, EntityFieldManagerInterface $entity_field_manager, AccountInterface $current_user, FileFieldUploader $file_field_uploader) {
    

    That seems like a lot of API change. :( Isn't this going to potentially blow up a lot of things? Don't we need to be more careful here and do more deprecation dancing?

Thanks again @zrpnr for getting us back to a working patch! Agreed with #80: Bravo!!

#81.25 is very important. I also think we need to think about how the API looks.

Here I have tried to address the following points of comment #81:
1, 2, 4, 5, 6, 7, 9, 10, 12, 14, 15, 16, 17, 20, 21, 22, and 24.

This still needs work for the remaining points.

Another reason not to have a shared service.

Edit: a very misleading typo

@ravi.shankar re: #83: Thanks! Mostly your fixes look good.

However, for point 2 you only changed exactly the spot I flagged. That's changing the order of arguments that get passed to the constructor for this service class. So we also need to fix FileFieldUploader:: __construct() to match:

+++ b/core/modules/file/src/FileFieldUploader.php
@@ -136,112 +110,56 @@ class FileUploadResource extends ResourceBase {
+   * Constructs a FileFieldUploader instance.
+   *
+   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
+   *   The entity type manager.
    * @param \Psr\Log\LoggerInterface $logger
    *   A logger instance.
    * @param \Drupal\Core\File\FileSystemInterface $file_system
    *   The file system service.
-   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
-   *   The entity type manager.
-   * @param \Drupal\Core\Entity\EntityFieldManagerInterface $entity_field_manager
-   *   The entity field manager.
-   * @param \Drupal\Core\Session\AccountInterface $current_user
-   *   The currently authenticated user.
    * @param \Symfony\Component\Mime\MimeTypeGuesserInterface $mime_type_guesser
    *   The MIME type guesser.
    * @param \Drupal\Core\Utility\Token $token
    *   The token replacement instance.
    * @param \Drupal\Core\Lock\LockBackendInterface $lock
    *   The lock service.
-   * @param \Drupal\Core\Config\Config $system_file_config
-   *   The system file configuration.
    * @param \Symfony\Component\EventDispatcher\EventDispatcherInterface $event_dispatcher
    *   The event dispatcher service.
    */
-  public function __construct(array $configuration, $plugin_id, $plugin_definition, $serializer_formats, LoggerInterface $logger, FileSystemInterface $file_system, EntityTypeManagerInterface $entity_type_manager, EntityFieldManagerInterface $entity_field_manager, AccountInterface $current_user, $mime_type_guesser, Token $token, LockBackendInterface $lock, Config $system_file_config, EventDispatcherInterface $event_dispatcher = NULL) {
-    parent::__construct($configuration, $plugin_id, $plugin_definition, $serializer_formats, $logger);
-    $this->fileSystem = $file_system;
+  public function __construct(EntityTypeManagerInterface $entity_type_manager, LoggerInterface $logger, FileSystemInterface $file_system, $mime_type_guesser, Token $token, LockBackendInterface $lock, EventDispatcherInterface $event_dispatcher = NULL) {
     $this->entityTypeManager = $entity_type_manager;
-    $this->entityFieldManager = $entity_field_manager;
-    $this->currentUser = $current_user;
+    $this->logger = $logger;
+    $this->fileSystem = $file_system;

So, still TODO from #81:
2. (see above)
3.
6. You removed the %s, but maybe we wanted to pass in the filename instead?
8.
11.
13.
18.
19.
23. I'm only half joking. ;) Seems weird to be adding references to code we're also trying to kill. Relates to point 25.
25. (Agreed with @alexpott - this is the main thing that needs (much) more work before this is ready for real review.)
26.

Everything else in the interdiff looks good, and I confirm you fixed my concern in the other cases.

Thanks again!
-Derek

Tagging this for work sprint

Taking a look

Attempt at a re-roll of #83 while fixing the order of arguments in FileFieldUploader::_construct()

Something changed with the validators.

@dww in #81.5 you say we should return a list of validators. However, the current code calls:

• \Drupal\rest\Plugin\rest\resource\EntityResourceValidationTrait::validate() which doesn't return validators and instead throws \Symfony\Component\HttpKernel\Exception\UnprocessableEntityHttpException
• \file_validate() which returns an array of error messages.

I think it makes it difficult to do that. What's the ideal interface we want?

Given the current dependencies on file and rest modules for validation, I think it might be worth looking at splitting this issue up into smaller achievable chunks. Thoughts?

I've created a meta issue #3221796: [META] Modernise file upload logic to track this and the separate tasks as proposed above. Please feel free to speak up if you think we should handle this differently.

This is only blocked on #3221794: Unify file upload validation from rest and json api modules now.

After spending some time on #3221794: Unify file upload validation from rest and json api modules I've come to the conclusion its not going to make things easier by splitting this issue down. The changes to the dependencies by splitting off the validation will only need to be undone when we need to update them for the uploader.

I moved some of the logic in that issue, including the undeclared dependencies on the file module in JSON API, and have put them into this combined issue.

I have been working through the common code and differences between REST and JSON API, as well as taking a look at what _file_upload_single() and GraphQL modules are doing.

I've tried to split out the Uploader into smaller, single-purpose classes. This can hopefully reduce the complexity. e.g.

• core/modules/file/src/Upload/FileFieldResolver.php
• core/modules/file/src/Upload/FileFieldUploader.php
• core/modules/file/src/Upload/FileUploadAccessChecker.php
• core/modules/file/src/Upload/FileUploadResult.php
• core/modules/file/src/Upload/FileUploadStreamReader.php
• core/modules/file/src/Upload/FileUploadValidator.php

There are still differences in validation. For example JSON API checks permissions for the entity the file field is attached to, but REST is much simpler. Not sure if we should be trying to unify that logic here.

I expect there will be a number of errors with this patch (e.g. error message formatting is slightly different in each module), but wanted to post it for early feedback.

The interdiff is also not going to be too useful as there are a lot of changes.

Fix newlines in error messages.

First quick review:

1. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
   @@ -56,45 +47,6 @@
   -  const REQUEST_HEADER_FILENAME_REGEX = '@\bfilename(?<star>\*?)=\"(?<filename>.+)\"@';
   ...
   -  const BYTES_TO_READ = 8192;
   

   I am sure if we can remove these constants without creating a BC break.

2. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
   @@ -148,40 +88,43 @@ class FileUploadResource extends ResourceBase {
   -  public function __construct(array $configuration, $plugin_id, $plugin_definition, $serializer_formats, LoggerInterface $logger, FileSystemInterface $file_system, EntityTypeManagerInterface $entity_type_manager, EntityFieldManagerInterface $entity_field_manager, AccountInterface $current_user, $mime_type_guesser, Token $token, LockBackendInterface $lock, Config $system_file_config, EventDispatcherInterface $event_dispatcher = NULL) {
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, array $serializer_formats, LoggerInterface $logger, ?EntityTypeManagerInterface $entity_type_manager, ?EntityFieldManagerInterface $entity_field_manager, AccountInterface $current_user, FileFieldUploader $file_field_uploader = NULL, FileFieldResolver $file_field_resolver = NULL, FileUploadAccessChecker $file_upload_access_checker = NULL) {
   

   I am quite sure this will be a BC break. Adding, and changing typehints. Changing the parameters. If contrib is overriding this method, they will have a problem.

3. +++ b/core/modules/file/src/Upload/FileFieldResolver.php
   @@ -0,0 +1,64 @@
   +  public function resolveFieldDefinition(string $entity_type_id, string $bundle, string $field_name): FieldDefinitionInterface {
   

   Could we add some testing for this new method.

4. +++ b/core/modules/file/src/Upload/FileFieldUploader.php
   @@ -0,0 +1,349 @@
   + *   marking it as an API instead.
   

   Could we add "public" before "API".

5. +++ b/core/modules/file/src/Upload/FileFieldUploader.php
   @@ -0,0 +1,349 @@
   +   * @param array $settings
   +   *   The array of field settings.
   ...
   +  protected function getUploadLocation(array $settings) {
   

   Which keys are those? Which ones are required?

6. +++ b/core/modules/file/src/Upload/FileFieldUploader.php
   @@ -0,0 +1,349 @@
   +   * @param string $file_uri
   +   *   The file URI.
   ...
   +   * @return string
   +   *   The generated lock ID.
   ...
   +  protected static function generateLockIdFromFileUri($file_uri) {
   

   Can we add the typehint "string" twice.

7. +++ b/core/modules/file/src/Upload/FileUploadAccessChecker.php
   @@ -0,0 +1,91 @@
   +   * @throws \Drupal\Component\Plugin\Exception\PluginNotFoundException
   +   *   Thrown if the entity type could not be found.
   ...
   +  public function checkUploadAccess(string $entity_type_id, string $bundle, FieldDefinitionInterface $field_definition): AccessResultInterface {
   

   How does the exception get thrown. Not saying that it is wrong, just asking.

8. +++ b/core/modules/file/src/Upload/FileUploadAccessChecker.php
   @@ -0,0 +1,91 @@
   +   * @return \Drupal\Core\Access\AccessResultInterface
   +   *   The file upload access result.
   ...
   +  public function checkUploadAccessForEntity(AccountInterface $account, FieldDefinitionInterface $field_definition, EntityInterface $entity = NULL) {
   

   Can we add AccessResultInterface as a typehint.

9. +++ b/core/modules/file/src/Upload/FileUploadStreamReader.php
   @@ -0,0 +1,85 @@
   +   * @param resource $stream
   ...
   +  public function readStreamDataToFile($stream): string {
   

   What is "resource" exactly in "@param resource $stream"?

10. +++ b/core/modules/jsonapi/src/Controller/FileUpload.php
    @@ -190,6 +206,58 @@ public function handleFileUploadForNewResource(Request $request, ResourceType $r
    +   * @throws \Drupal\Core\Entity\EntityStorageException
    +   *   Thrown if the upload's target resource could not be saved.
    ...
    +  protected function handleFileUpload(Request $request, ResourceType $resource_type, $file_field_name, FieldableEntityInterface $entity = NULL): FileInterface {
    ...
    +      throw new NotFoundHttpException($e->getMessage(), $e);
    ...
    +      throw new AccessDeniedHttpException($e->getMessage(), $e);
    ...
    +      throw new HttpException(500, $e->getMessage(), $e);
    ...
    +      throw new ServiceUnavailableHttpException(1, $e->getMessage(), $e);
    ...
    +      throw new HttpException(500, 'Temporary file could not be moved to file location', $e);
    ...
    +      throw new UnprocessableEntityHttpException($upload_result->getViolationMessage());
    

    This method throws a bunch of non-documented exceptions. And the one that is documented is never throw as far as I can see.

11. +++ b/core/modules/jsonapi/src/Controller/FileUpload.php
    @@ -190,6 +206,58 @@ public function handleFileUploadForNewResource(Request $request, ResourceType $r
    +   * @param \Drupal\Core\Entity\FieldableEntityInterface|null $entity
    +   *   The entity for which the file is to be uploaded.
    ...
    +  protected function handleFileUpload(Request $request, ResourceType $resource_type, $file_field_name, FieldableEntityInterface $entity = NULL): FileInterface {
    

    Can we a to the documentation that the parameter is "optional".

12. +++ b/core/modules/jsonapi/src/Controller/FileUpload.php
    @@ -190,6 +206,58 @@ public function handleFileUploadForNewResource(Request $request, ResourceType $r
    +   * @param string $file_field_name
    +   *   The file field for which the file is to be uploaded.
    ...
    +  protected function handleFileUpload(Request $request, ResourceType $resource_type, $file_field_name, FieldableEntityInterface $entity = NULL): FileInterface {
    

    Can we add the typehint "string" for the parameter $file_field_name.

> I am quite sure this will be a BC break. Adding, and changing typehints. Changing the parameters. If contrib is overriding this method, they will have a problem.

Plugin classes are explicitly not in the BC policy and we can change them.

Thanks for the review @daffie.

This is still a work in progress, and I'm keen to get feedback on how we split out the classes for re-use etc. Getting feedback from the JSON API (@gabesullice) and REST (@Wim Leers) module maintainers would be great! Also GraphQL if they are watching this issue.

Also, we still need to replace duplicate code in _file_save_upload_single().

Responses to #104:

1. Added back and deprecated.
2. From past issues we have made a best-effort to provide BC even if it's not covered by the policy. I think this is our best effort.
3. Added a unit test.
4. Fixed
5. Changed this to take the $file_directory and $scheme args instead of $settings
6. Fixed
7. \Drupal\Core\Entity\EntityTypeManagerInterface::getDefinition() throws \Drupal\Component\Plugin\Exception\PluginNotFoundException by implementing \Drupal\Component\Plugin\Discovery\DiscoveryInterface::getDefinition()
8. Fixed
9. a file pointer resource returned from fopen(). I updated the comment to "The file handler returned from fopen()." to clarify
10. \Drupal\file\Upload\FileFieldUploaderInterface::uploadFile() throws EntityStorageException. Added the missing @throws docs.
11. Fixed
12. Fixed

I've also added an interface \Drupal\file\Upload\FileFieldUploaderInterface as I think it makes the API much more readable.

🤔 I'm wondering if we exclude _file_save_upload_single() from this issue. It is different in a few ways:

1. It's called from file_save_upload() which supports multi-file uploads
2. It gets the files from \Drupal::request()->files->get('files', [])
3. REST and JSON API modules both use php://input for getting POST body, but that method does not support multi-part uploads
4. REST and JSON API modules both use fields to store file data, and get their validators and settings from a file definition

Random idea. Can we add smaller public methods to the new service that we could maybe re-use, for example validation handling? But that could also be done later.

@Berdir yeah I split out that into it's own service. Wasn't sure if that was going too far.

@kimpepper Path #106 has bugs and can't be used to upload a file via REST API

Please check this method \Drupal\file\Upload\FileFieldUploader::createFile
At that time the file is stored in a temporary directory but you are creating a file entity with a destination URI. The file wasn't moved to the destination URI yet.

As result, other modules can't use hook_file_validate() cause they won't be able to realize where is the uploaded file.
We can see the correct behavior in the file_move() function: the file URI should be set only after the file moving.

There is the updated patch

After coming back to this issue after a bit of a break, I think we need to break it down into bite-sized chunks.

Since _file_save_upload_single() is only creating file entities, and does not care about file fields, I split it off to a new issue: #3232248: Move _file_save_upload_single to a service and deprecate. We may be able to leverage that for the file uploads in this issue.

#3232248: Move _file_save_upload_single to a service and deprecate

blocker is in #3232248: Move _file_save_upload_single to a service and deprecate

Wonder if we should focus on getting #3223209: deprecate file_save_data, file_copy and file_move and replace with a service in first? Not sure how much overlap / conflict there still would be. I haven't had time to dig into the latest code here to understand what's happening and where.

@dww this issue delivers far more tangible benefits than the other issue. This one should be prioritised

Working on this

I took a fresh stab at this after #3232248: Move _file_save_upload_single to a service and deprecate landed so no interdiff. This is a work in progress.

The main points are:

• Refactored \Drupal\jsonapi\Controller\FileUpload first. Still need to look at REST
• Want to wrap \Drupal\file\Upload\FileUploadHandler which uses \Symfony\Component\HttpFoundation\File\UploadedFile.
• UploadedFile is primarily used for form uploads with multipart form data.
• Both JSON API and REST use content-disposition header and php://input to read file data directly.
• This means UploadedFile isn't directly available from the $request, and we need to manage it ourselves
• \Drupal\file\Upload\FileFieldUploadHandler wraps \Drupal\file\Upload\FileUploadHandler and uses settings, validators etc from the the file field settings
• I created \Drupal\file\Upload\FileStreamUploader to read php://input and return an UploadedFile so we can re-use FileUploadHandler
• I broke out
  • \Drupal\file\Upload\FileFieldUploadAccessChecker
  • \Drupal\file\Upload\FilenameExtractor
  • \Drupal\file\Upload\FileFieldUploadHandler
  • \Drupal\file\Upload\FileStreamUploader

  as their own services and wrote unit tests for them

Looking for early feedback on the approach before I tackle REST.

Fixes some of the tests due to implied dependency on the file module.

Fixes a few more tests.

I may have hit a stumbling block using UploadedFile as \Symfony\Component\HttpFoundation\File\UploadedFile::isValid() calls is_uploaded_file() which only works for files that are in the $_FILES superglobal which I think is only set on form posts.

One way might be to change our \Drupal\file\Upload\FileUploadHandler::handleFileUpload() to take a UploadedFileInterface then provide one based on symfony UploadedFile for forms, and another based on php://input for REST & JSON API.

This is an effort to implement what I proposed in #124.

It introduces:

• \Drupal\file\Upload\UploadedFileInterface with two subclasses:
• \Drupal\file\Upload\FormUploadedFile which wraps Symfony\Component\HttpFoundation\File\UploadedFile
• \Drupal\file\Upload\RawUploadedFile for use with our JSON API / REST controllers.
• I updated \Drupal\file\Upload\FileUploadHandler::handleFileUpload() to take a UploadedFileInterface
• \Drupal\file\Upload\FileFieldUploadHandler now subclasses FileUploadHandler so we can handle the special case of \Drupal\Core\File\FileSystemInterface::moveUploadedFile() which only works with form uploaded files.

Fixes a couple more test fails, and switches FileFieldUploadHandlerTest to a kernel test.

I have a proposal. We move jsonapi.file_upload out of jsonapi.services.yml and into \Drupal\jsonapi\JsonapiServiceProvider::register() and we can register the service if the file module is around. \Drupal\jsonapi\Routing\Routes::getFileUploadRoutesForResourceType() already depends on the File entity type existing.

As discussed in Slack, if this doesn't get in before 9.3.0 alpha, we need a smaller issue to ensure we are not painted into a corner with UploadedFile. I created #3245244: Make FileUploadHandler re-usuable for non-form file uploads.

We might want to look at \Drupal\file\Plugin\Field\FieldType\FileItem in this issue too. It has some duplicate code for:

• getUploadLocation()
• getUploadValidators()

+++ b/core/modules/jsonapi/jsonapi.info.yml
@@ -6,3 +6,4 @@ version: VERSION
 configure: jsonapi.settings
 dependencies:
   - drupal:serialization
+  - drupal:file

If we end up doing this, we're going to need an update/post update in jsonapi to install file module.

Dynamically registering the services depending on whether file module is installed or not seems like a good idea.... But what do we do with the route/controller if it's not though?

I think I commented earlier that this dependency already existed, it was just hidden because it was copied/code/function calls. It would probably already fail in a very ugly way on HEAD.

We do have a route requirement to check for another module, for example:

comment.node_redirect:
  path: '/comment/{node}/reply'
  defaults:
    _controller: '\Drupal\comment\Controller\CommentController::redirectNode'
  requirements:
    _entity_access: 'node.view'
    _module_dependencies: 'node'
    node: \d+

We can possibly do the same here?

If it's completely broken in HEAD, then I think it'd be fine to make the route dependent on file module too.

Looking closer, I think it's not actually a problem.

The routes are defined dynamically anyway, and if there are no file fields on a given resource, it just won't define any routes for it. So defining the service dynamically should be fine?

See \Drupal\jsonapi\Routing\Routes::getFileUploadRoutesForResourceType(), we could make it more explicit with a module exists check there but it should be fine.

OK that is encouraging. Given that, I think we should go for #130, so marking needs work for that. If the routes are only defined when there's a file field, then it's probably fine to leave as-is.

Re-roll of #128 after #3245244: Make FileUploadHandler re-usuable for non-form file uploads

This moves 'jsonapi.file_upload' service to be dynamically registered if the file module is enabled as per #130.

Re-roll of ##14 after #3223209: deprecate file_save_data, file_copy and file_move and replace with a service

Re-roll of #141

Worked with @larowlan to try and figure out the test fails. Found one bug where we weren't passing through the entity for access checks, but don't think we've found the main reason yet.

Pass EXISTS_RENAME to handleFileUpload().

#3154962: TemporaryJsonapiFileFieldUploader::checkFileUploadAccess() checks for bundle landed

Tried to fix build failures

Overall I appreciate the density of comments in the code -- it helps a lot with following what the code is doing!

I'm not sure about the pattern of adding lots of services which only have one method, which is only used once.

1. +++ b/core/modules/file/src/FileFieldDefinitionResolver.php
   @@ -0,0 +1,64 @@
   + * Provides a File field resolver.
   

   First line should try to say what the class does, not just repeat its name.

   Though I'm not really sure what the purpose of this class is -- it's just a wrapper around the entity field manager which also checks the target type. It's only called once. Is it really necessary for this to be in a service?

2. +++ b/core/modules/file/src/FileFieldDefinitionResolver.php
   @@ -0,0 +1,64 @@
   +   *   The entity type manager.
   

   Stray line.

3. +++ b/core/modules/file/src/FileFieldDefinitionResolver.php
   @@ -0,0 +1,64 @@
   +    if ($field_definition->getSetting('target_type') !== 'file') {
   

   Are we checking the setting rather than the field type to allow for entity reference fields which target files?

4. +++ b/core/modules/file/src/Upload/FileFieldUploadAccessChecker.php
   @@ -0,0 +1,70 @@
   +   *   If the entity does not exist and it is not given, create access to the
   +   *   file will be checked.
   

   What file? Should this say 'field' instead?

5. +++ b/core/modules/file/src/Upload/FileFieldUploadHandler.php
   @@ -0,0 +1,164 @@
   +  protected $fileUploadHandler;
   

   I can't find where this property is used.

6. +++ b/core/modules/file/src/Upload/FileFieldUploadHandler.php
   @@ -0,0 +1,164 @@
   +   * The token replacement instance.
   

   Doesn't agree with the @param doc on the constructor.

7. +++ b/core/modules/file/src/Upload/FileFieldUploadHandler.php
   @@ -0,0 +1,164 @@
   +   *   Thrown when a file upload error occurred.
   

   Documentation standards say this should be a complete sentence.

8. +++ b/core/modules/file/src/Upload/RawFileUploader.php
   @@ -0,0 +1,92 @@
   +  const BYTES_TO_READ = 8192;
   

   Would be nice if the docs explained why this number.

9. +++ b/core/modules/file/src/Upload/RawFileUploader.php
   @@ -0,0 +1,92 @@
   +   *   (Optional) The source of the upload. Defaults to 'php://input'.
   

   'optional' should be in lowercase.

10. +++ b/core/modules/file/src/Upload/RawUploadedFile.php
    @@ -0,0 +1,121 @@
    +   * @param string $path
    +   * @param string $originalName
    +   * @param string $mimeType
    +   * @param int $error
    

    Missing docs.

As the scope of this task to introduce new _generic_ APIs in Drupal, having a \Drupal\file\Upload\FileFieldUploadHandler::handleFileUpload() public method (inherited from the parent class) and also an overridden `\Drupal\file\Upload\FileFieldUploadHandler::moveUploadedFile() triggers my senses because we are might abusing inheritance here.

I was testing #150 using json api + private files upload + anonymous user and it works for 1 file (i can see deprecations warning) but for many files it looks like upload override files id in "anonymous_allowed_file_ids" so there is no option to save a entity because validation fails.

It's been a long time since I looked at this issue. I've taken the patch at #150 and fixed up a few of the PHPCS issues to see how the tests will run.

Fix return type declarations and remove unnecessary method overrides.

Digging into the test failures, it looks like in Drupal\Tests\jsonapi\Functional\FileUploadTest::testFileUploadMaliciousExtension we are assuming a file with filename example.php will get successfully uploaded as example.php_.txt

core/modules/jsonapi/tests/src/Functional/FileUploadTest.php (line 621):

    // Test using a masked exploit file.
    $response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example.php"']);
    // The filename is not munged because .txt is added and it is a known
    // extension to apache.
    $expected = $this->getExpectedDocument(1, 'example.php_.txt', TRUE);

I'm a bit confused why this should not get rejected given in core/modules/system/src/EventSubscriber/SecurityFileUploadEventSubscriber.php (line 78) we have the comment:

      // This upload will be rejected by file_validate_extensions() anyway so do
      // not make any alterations to the filename. This prevents a file named
      // 'example.php' being renamed to 'example.php_.txt' and uploaded if the
      // .txt extension is allowed but .php is not. It is the responsibility of
      // the function that dispatched the event to ensure file_validate() is
      // called with 'file_validate_extensions' in the list of validators if
      // $extensions is not empty.