Unify file upload validation from rest and json api modules

The title seems very confusing, shouldn't that say that we should move it to the file *module*. The title sounds like moving it to \Drupal\Core\FileSystem or so, which isn't possible as the file entity is in file module.

rest.module is just as much "core" as file.module, for now anyway ;)

i am working on it

fix test cases of #10

sorry @naveen433, i didn't see your comment

please find below attach patch file and interdiff file

As mentioned in the comment (https://www.drupal.org/project/drupal/issues/3221794#comment-14157504), #13 patch is approved, so to have pass patch as the latest patch in the ticket I am re-uploading the patch.

Thanks
Pavan

1. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
   @@ -56,10 +52,6 @@
    class FileUploadResource extends ResourceBase {
    
   -  use EntityResourceValidationTrait {
   -    validate as resourceValidate;
   -  }
   -
   

   Wondering if there are any BC aspects to this, subclasses that use this method or so, same for the custom methods that are currently removed. to be extra careful, we might need to keep them around and slap a trigger_error() on them.

2. +++ b/core/modules/file/src/Validation/FileUploadValidator.php
   @@ -0,0 +1,132 @@
   +    $errors = file_validate($file, $validators);
   +    if (!empty($errors)) {
   +      $translator = new DrupalTranslator();
   +      foreach ($errors as $error) {
   +        $violation = new ConstraintViolation($translator->trans($error),
   

   Not sure about the translation stuff. Looking at \Drupal\file\Plugin\Validation\Constraint\FileValidationConstraintValidator, it doesn't do that, and the messages are already translatable strings. \Drupal\Core\Validation\DrupalTranslator::trans() returns TranslatableMarkup object unchanged.

3. +++ b/core/modules/file/src/Validation/FileUploadValidator.php
   @@ -0,0 +1,132 @@
   +          EntityAdapter::createFromEntity($file),
   

   maybe do this only once outside of the loop?

4. +++ b/core/modules/file/src/Validation/FileUploadValidator.php
   @@ -0,0 +1,132 @@
   +    $validators = [
   +      // Add in our check of the file name length.
   +      'file_validate_name_length' => [],
   +    ];
   

   Wondering what exactly "our" is in this scope. This is copied from the existing method, which in turn is copied 1:1 from _file_save_upload_single().

5. +++ b/core/modules/file/src/Validation/FileUploadValidator.php
   @@ -0,0 +1,132 @@
   +    // Cap the upload size according to the PHP limit.
   +    $max_filesize = Bytes::toNumber(Environment::getUploadMaxSize());
   +    if (!empty($settings['max_filesize'])) {
   +      $max_filesize = min($max_filesize, Bytes::toNumber($settings['max_filesize']));
   +    }
   +
   +    // There is always a file size limit due to the PHP server limit.
   

   this doesn't hurt as we're specifically only checking the upload, but I'm still confused about it. In the widget/form elements, it is both validation and documentation/info for the user, so considering it makes sense, but that doesn't really seem to be the case here.

6. +++ b/core/modules/file/src/Validation/FileUploadValidator.php
   @@ -0,0 +1,132 @@
   +  public function prepareFilename($filename, array &$validators): string {
   +    // The actual extension validation occurs in
   +    // \Drupal\file\Plugin\rest\resource\FileUploadResource::validate().
   +    $extensions = $validators['file_validate_extensions'][0] ?? '';
   +    $event = new FileUploadSanitizeNameEvent($filename, $extensions);
   +    $this->eventDispatcher->dispatch($event);
   

   the comment needs to updated.

7. +++ b/core/modules/rest/tests/src/Functional/FileUploadResourceTestBase.php
   @@ -349,7 +349,7 @@ public function testPostFileUploadDuplicateFileRaceCondition() {
        // Make the same request again. The upload should fail validation.
        $response = $this->fileRequest($uri, $this->testFileData);
   -    $this->assertResourceErrorResponse(422, PlainTextOutput::renderFromHtml("Unprocessable Entity: validation failed.\nuri: The file public://foobar/example.txt already exists. Enter a unique file URI.\n"), $response);
   +    $this->assertResourceErrorResponse(422, PlainTextOutput::renderFromHtml("Unprocessable Entity: file validation failed.\nuri: The file public://foobar/example.txt already exists. Enter a unique file URI.\n"), $response);
      }
    
      /**
   @@ -462,7 +462,7 @@ public function testFileUploadInvalidFileType() {
   
   @@ -462,7 +462,7 @@ public function testFileUploadInvalidFileType() {
    
        // Test with a JSON file.
        $response = $this->fileRequest($uri, '{"test":123}', ['Content-Disposition' => 'filename="example.json"']);
   -    $this->assertResourceErrorResponse(422, PlainTextOutput::renderFromHtml("Unprocessable Entity: file validation failed.\nOnly files with the following extensions are allowed: <em class=\"placeholder\">txt</em>."), $response);
   +    $this->assertResourceErrorResponse(422, PlainTextOutput::renderFromHtml("Unprocessable Entity: file validation failed.\n: Only files with the following extensions are allowed: <em class=\"placeholder\">txt</em>.\n"), $response);
    
        // Make sure that no file was saved.
        $this->assertEmpty(File::load(1));
   @@ -488,7 +488,7 @@ public function testFileUploadLargerFileSize() {
   
   @@ -488,7 +488,7 @@ public function testFileUploadLargerFileSize() {
    
        // Generate a string larger than the 50 byte limit set.
        $response = $this->fileRequest($uri, $this->randomString(100));
   -    $this->assertResourceErrorResponse(422, PlainTextOutput::renderFromHtml("Unprocessable Entity: file validation failed.\nThe file is <em class=\"placeholder\">100 bytes</em> exceeding the maximum file size of <em class=\"placeholder\">50 bytes</em>."), $response);
   +    $this->assertResourceErrorResponse(422, PlainTextOutput::renderFromHtml("Unprocessable Entity: file validation failed.\n: The file is <em class=\"placeholder\">100 bytes</em> exceeding the maximum file size of <em class=\"placeholder\">50 bytes</em>.\n"), $response);
    
        // Make sure that no file was saved.
        $this->assertEmpty(File::load(1));
   @@ -598,7 +598,7 @@ public function testFileUploadMaliciousExtension() {
   
   @@ -598,7 +598,7 @@ public function testFileUploadMaliciousExtension() {
        $this->refreshTestStateAfterRestConfigChange();
    
        $response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example_7.php"']);
   -    $this->assertResourceErrorResponse(422, "Unprocessable Entity: file validation failed.\nFor security reasons, your upload has been rejected.", $response);
   +    $this->assertResourceErrorResponse(422, "Unprocessable Entity: file validation failed.\n: For security reasons, your upload has been rejected.\n", $response);
    
   

   most of these messages actually don't have a property path, so it looks pretty broken with a leading ":". Is it really worth adding that to the message? Make it dynamic at least, so we only add it if not empty?

1. +++ b/core/modules/file/src/Validation/FileUploadValidator.php
   @@ -0,0 +1,133 @@
   +  public function getUploadValidators(FieldDefinitionInterface $field_definition) {
   

   Can we add : array as a type hint for the return value.

2. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
   @@ -164,10 +163,12 @@ class FileUploadResource extends ResourceBase {
   -   * @param \Symfony\Contracts\EventDispatcher\EventDispatcherInterface $event_dispatcher
   +   * @param \Symfony\Contracts\EventDispatcher\EventDispatcherInterface|null $event_dispatcher
       *   The event dispatcher service.
   +   * @param \Drupal\file\Validation\FileUploadValidator $file_upload_validator
   +   *   The file upload validator.
       */
   -  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, $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, FileUploadValidator $file_upload_validator) {
   

   Should we not change the 2 parameters, so that we in D10 can easily remove the one that is no longer needed. The same with modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php.

> 2. I assume it was trying to follow the pattern in https://git.drupalcode.org/project/drupal/-/blob/9.2.x/core/lib/Drupal/C... ?

Yes, but that's a different thing. That's executing actual validation constraints and it will receive strings. Here we can expect that we receive already translated strings. It's a almost a no-op, but it's also extra code to follow/understand. I'd vote to simplify it when refactoring it into one place.

> 5. Not sure what the alternative is? Remove it? We'd need to add it back in all of the calling code

I don't know. Just thinking out aloud, honestly wondering about the reason/background for adding this explicitly. My understanding is that it can never get that far if file would really be above that limit as PHP would already block the upload or am I missing something?

> Looks like code in \Drupal\file\Plugin\Field\FieldType\FileItem::getUploadValidators() is also duplicate :-\

That is where it was copied from initially. The problem with that was is that's a chicken/egg situation, I've had that before as well. You need an entity with a file item object to get the upload validators out of that. Forms have that, other places like this upload logic do not, as you upload the file first and then pass the ID along when creating the entity on a separate API call. So we had to duplicate it. Now that we do put it on a dedicated service, we might be able to re-use that service in FileItem.

Same story with getUploadLocation(), that was even extracted into a static method to use it from generateSampleValue(), but it was made protected to prevent you from reusing that code ;) Both FileUploadResource and the Json API version duplicate that method too. But I suppose it's not a good match for this new service.

On 2: Well, arguably FileValidationConstraintValidator is a constraint too and will actually run it through \Drupal\Core\TypedData\Validation\ExecutionContext::addViolation() so that will pass through trans() too. Still feel like it would make sense to remove the call here, as we have the choice to do that, while FileValidationConstraintValidator does not. And we must assume that file_validate() returns TranslatableMarkup() objects, because those validation messages have placeholders and re-translating them would be wrong. It's only not strictly wrong because of the lazy-translation behavior of t() in D8.

The testbot is returning with the error: The service "jsonapi.file.uploader.field" has a dependency on a non-existent service "file.upload_validator".

Hm, yeah, the actual dependency was more hidden before, an argument could be made that there are use cases where you want to use jsonapi without files but it already didn't properly handle that before and I guess just relied on not using/calling that service/plugin before. The plugin definition didn't have a module dependency either.

Kernel test fails are very expected, you only enable the specific modules you need to get the test to pass and that now includes file due to the service definition.

Both the plugin and the service could be dynamically added if the file module is enabled, similar to \Drupal\entity_reference_revisions\EntityReferenceRevisionsServiceProvider or so. Not sure if it's worth it :)

1. +++ b/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php
   @@ -136,6 +139,11 @@ public function __construct(LoggerInterface $logger, FileSystemInterface $file_s
        $this->eventDispatcher = $event_dispatcher;
   
   @@ -352,63 +360,6 @@ protected function streamUploadData() {
   -    $this->eventDispatcher->dispatch($event);
   

   The only place where $this->eventDispatcher was used will be removed in this patch. Should we therefor add a deprecation message to the class variable in __construct()?

2. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
   @@ -182,6 +183,11 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
        $this->eventDispatcher = $event_dispatcher;
   
   @@ -458,41 +465,18 @@ protected function validateAndLoadFieldDefinition($entity_type_id, $bundle, $fie
   -    $this->eventDispatcher->dispatch($event);
   

   The same as with the class TemporaryJsonapiFileFieldUploader.

3. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
   @@ -2,36 +2,32 @@
   -use Drupal\rest\Plugin\rest\resource\EntityResourceValidationTrait;
   
   @@ -56,10 +52,6 @@
   -  use EntityResourceValidationTrait {
   -    validate as resourceValidate;
   -  }
   

   We are removing the use of the trait Drupal\rest\Plugin\rest\resource\EntityResourceValidationTrait. That trait is also used in: Drupal\rest\Plugin\rest\resource\EntityResource and Drupal\user\Plugin\rest\resource\UserRegistrationResource. Should we removed that trait also from those classes? I am not sure, just asking.

If #39.3 will get fixed in a followup/postponed issue, then it does not to be resolved in this issue.

Here is the rerolled patch. Retaining status 'Needs work' for changes suggested in #39, thanks!