Move file upload validation from file.module to constraint validators

Didn't look too closely yet, one related issue I'd like to point out is #2867336: File size validator should only respect the explicitly configured maximum file size.

That's a very awkward if you use s3 upload or any other mechanism that doesn't rely on manually uploading files on your server. If a file already exists then there is no reason to validate it against the upload max size IMHO.

I understand it's tricky to refactor things and change them at the same time, but I'd love to find a way to finally resolve that isuse.

It's very exciting to see this materializing! Don't mind my review too much since it's mostly made up of nitpicks on what I understand is an initial patch to illustrate the concept.

I understand it's tricky to refactor things and change them at the same time, but I'd love to find a way to finally resolve that isuse.

@Berdir Yeah... I hear you, but I'd be very reluctant to do that. From my limited experience with the file system, its issues appear to be plagued by scope creep and a desire to fix everything every other thing before fixing any one thing. Let's race these issues against each other instead.

1. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -331,6 +333,30 @@ public function getUploadValidators() {
   +    $constraints[] = new FileSize(['max_filesize' => $max_filesize]);
   
   +++ b/core/modules/file/src/Validation/Constraint/FileSizeLimit.php
   @@ -0,0 +1,29 @@
   +class FileSizeLimit extends Constraint {
   

   Are these supposed to be the same: FileSize vs FileSizeLimit?

2. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -331,6 +333,30 @@ public function getUploadValidators() {
   +    if (!empty($settings['file_extensions'])) {
   +      $constraints[] = new FileExtension(['extensions' => $settings['file_extensions']]);
   +    }
   

   Are there default file extension constraints we should have here?

3. +++ b/core/modules/file/src/Validation/Constraint/FileExtension.php
   @@ -0,0 +1,33 @@
   +  public $message = 'Only files with the following extensions are allowed: %files-allowed.';
   

   Nit: s/files-allowed/allowed-extensions/

4. +++ b/core/modules/file/src/Validation/Constraint/FileExtensionValidator.php
   @@ -0,0 +1,23 @@
   +class FileExtensionValidator extends ConstraintValidator {
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function validate($value, Constraint $constraint) {
   +    $regex = '/\.(' . preg_replace('/ +/', '|', preg_quote($constraint->extensions)) . ')$/i';
   +    if (!preg_match($regex, $value->getFilename())) {
   +      $this->context->addViolation($constraint->message, ['%files-allowed' => $constraint->extensions]);
   +    }
   +  }
   

   Are there unit tests in file.module for extension validation that we can port to this constraint?

5. +++ b/core/modules/file/src/Validation/Constraint/FileSizeLimit.php
   @@ -0,0 +1,29 @@
   +  const DEFAULT_FILE_LIMIT = 0;
   

   Nit: s/FILE/SIZE/

6. +++ b/core/modules/file/src/Validation/Constraint/FileSizeUserQuota.php
   @@ -0,0 +1,28 @@
   +  const DEFAULT_USER_LIMIT = 0;
   

   Nit: s/LIMIT/QUOTA/

1. +++ b/core/modules/file/file.module
   @@ -99,25 +104,49 @@ function file_field_widget_info_alter(array &$info) {
   + *   The validation constraints.
   

   Keyed by what? And in what order?

2. +++ b/core/modules/file/file.module
   @@ -99,25 +104,49 @@ function file_field_widget_info_alter(array &$info) {
   +function _file_convert_validators_to_constraints(FileInterface $file, array $validators): array {
   

   Could this go on a service somewhere instead of being a function? We can mark it as @internal.

3. +++ b/core/modules/file/file.module
   @@ -99,25 +104,49 @@ function file_field_widget_info_alter(array &$info) {
   +      // We can't convert the validator, so lets just call it.
   

   Typo: should be "let's".

4. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -341,6 +343,30 @@ public function getUploadValidators() {
   +  public function getUploadConstraints(): array {
   

   I think we need some docs on this class to explain why we have a mishmash of procedural validators and Symfony constraints.

   I assume it's because of BC? But shouldn't there also be an issue to deprecate the functional validators?

Capturing some conversation from DrupalSouth

\Drupal\Core\TypedData\TypedDataManager::getValidator has some prior art to create a validator

\Drupal\Core\TypedData\TypedData::validate has how to validate once you have the validator

Can the issue summary also be updated to include agreed upon solution please

Thanks!

To reflect current discussion

If CC error could be addressed.

Searched for file_validate_size() and all instances appear to be addressed.

Fyi expectDeprecation is deprecated itself but seems #3322785: [meta] Several expect*() methods have been deprecated in PHPUnit 9.6 there is still a plan being made.

This is looking amazing. I've done half the review and will finish it off tomorrow, here's observations so far

1. +++ b/core/modules/ckeditor5/src/Controller/CKEditor5ImageController.php
   @@ -238,20 +258,7 @@ protected function validate(FileInterface $file, array $validators) {
   -    $errors = file_validate($file, $validators);
   -    if (!empty($errors)) {
   -      $translator = new DrupalTranslator();
   -      foreach ($errors as $error) {
   -        $violation = new ConstraintViolation($translator->trans($error),
   -          (string) $error,
   -          [],
   -          EntityAdapter::createFromEntity($file),
   -          '',
   -          NULL
   -        );
   -        $violations->add($violation);
   -      }
   -    }
   +    $violations->addAll($this->fileValidator->validate($file, $validators));
   

   🔥

2. +++ b/core/modules/ckeditor5/src/Controller/CKEditor5ImageController.php
   @@ -261,15 +268,14 @@ protected function validate(FileInterface $file, array $validators) {
   -  protected function prepareFilename($filename, array &$validators) {
   -    $extensions = $validators['file_validate_extensions'][0] ?? '';
   -    $event = new FileUploadSanitizeNameEvent($filename, $extensions);
   +  protected function prepareFilename($filename, string $allowed_extensions) {
   +    $event = new FileUploadSanitizeNameEvent($filename, $allowed_extensions);
   

   Technically this is a BC break but we don't provide API for controllers and it looks like nothing is extending it in contrib https://git.drupalcode.org/search?group_id=2&scope=blobs&search=%22exten... so I think we're fine as long as this is mentioned in the CR

3. +++ b/core/modules/file/file.field.inc
   @@ -152,16 +152,16 @@ function template_preprocess_file_upload_help(&$variables) {
   -  if (isset($upload_validators['file_validate_size'])) {
   -    $descriptions[] = t('@size limit.', ['@size' => format_size($upload_validators['file_validate_size'][0])]);
   +  if (isset($upload_validators['FileSizeLimit'])) {
   +    $descriptions[] = t('@size limit.', ['@size' => format_size($upload_validators['FileSizeLimit']['fileLimit'])]);
      }
   -  if (isset($upload_validators['file_validate_extensions'])) {
   -    $descriptions[] = t('Allowed types: @extensions.', ['@extensions' => $upload_validators['file_validate_extensions'][0]]);
   +  if (isset($upload_validators['FileExtension'])) {
   +    $descriptions[] = t('Allowed types: @extensions.', ['@extensions' => $upload_validators['FileExtension']['extensions']]);
      }
    
   -  if (isset($upload_validators['file_validate_image_resolution'])) {
   -    $max = $upload_validators['file_validate_image_resolution'][0];
   -    $min = $upload_validators['file_validate_image_resolution'][1];
   +  if (isset($upload_validators['FileImageDimensions'])) {
   +    $max = $upload_validators['FileImageDimensions']['maxDimensions'];
   +    $min = $upload_validators['FileImageDimensions']['minDimensions'];
   

   We will need to handle both formats here, as there will be existing render arrays with the theme hook in contrib/custom code with this in the old format.

   We should trigger a deprecation error if they are

4. +++ b/core/modules/file/src/Element/ManagedFile.php
   @@ -360,8 +360,8 @@ public static function processManagedFile(&$element, FormStateInterface $form_st
   -    if (isset($element['#upload_validators']['file_validate_extensions'][0])) {
   -      $extension_list = implode(',', array_filter(explode(' ', $element['#upload_validators']['file_validate_extensions'][0])));
   +    if (isset($element['#upload_validators']['FileExtension']['extensions'])) {
   +      $extension_list = implode(',', array_filter(explode(' ', $element['#upload_validators']['FileExtension']['extensions'])));
          $element['upload']['#attached']['drupalSettings']['file']['elements']['#' . $id] = $extension_list;
   

   We will need to handle both formats here, as there will be existing forms in contrib/custom code with this in the old format.

   We should trigger a deprecation error if they are

5. +++ b/core/modules/file/src/Plugin/Validation/Constraint/BaseFileConstraintValidator.php
   @@ -0,0 +1,35 @@
   +    if (!$value instanceof FileInterface) {
   +      throw new UnexpectedTypeException($value, FileInterface::class);
   +    }
   

   nice

6. +++ b/core/modules/file/src/Plugin/Validation/Constraint/FileExtensionConstraint.php
   @@ -0,0 +1,41 @@
   +  public function getDefaultOption(): ?string {
   

   we should be safe to narrow this to :string

7. +++ b/core/modules/file/src/Plugin/Validation/Constraint/FileExtensionSecureConstraintValidator.php
   @@ -0,0 +1,51 @@
   +    if (!$allowInsecureUploads  && preg_match(FileSystemInterface::INSECURE_EXTENSION_REGEX, $file->getFilename())) {
   

   🧐 think there's an extra space here

8. +++ b/core/modules/file/src/Plugin/Validation/Constraint/FileImageDimensionsConstraint.php
   @@ -0,0 +1,55 @@
   + * File extension secure constraint.
   

   copy/paste?

9. +++ b/core/modules/file/src/Plugin/Validation/Constraint/FileImageDimensionsConstraintValidator.php
   @@ -0,0 +1,123 @@
   +    if ($image->isValid()) {
   

   I think if we flip this and return early it would simplify the code, e.g.

   if (!$image->isValid()) {
     return;
   }

10. +++ b/core/modules/file/src/Plugin/Validation/Constraint/FileImageDimensionsConstraintValidator.php
    @@ -0,0 +1,123 @@
    +          if ($image->scale($width, $height)) {
    

    we can probably flip this too, and add the violation and return early, avoiding the else and another layer of nesting

11. +++ b/core/modules/file/src/Plugin/Validation/Constraint/FileImageDimensionsConstraintValidator.php
    @@ -0,0 +1,123 @@
    +            if (!empty($width) && !empty($height)) {
    ...
    +            elseif (empty($width)) {
    ...
    +            elseif (empty($height)) {
    

    we can return from each of these and avoid elseif

12. +++ b/core/modules/file/src/Plugin/Validation/Constraint/FileImageDimensionsConstraintValidator.php
    @@ -0,0 +1,123 @@
    +            $this->context->addViolation($constraint->messageResizedImageTooSmall,
    

    we can return here and avoid an else too

13. +++ b/core/modules/file/src/Plugin/Validation/Constraint/FileIsImageConstraintValidator.php
    @@ -0,0 +1,51 @@
    + * Validator for the FileExtensionSecureConstraint.
    

    copy/paste

14. +++ b/core/modules/file/src/Plugin/Validation/Constraint/FileNameLengthConstraint.php
    @@ -0,0 +1,39 @@
    +  public string $messageTooLong = "The file's name exceeds the 240 characters limit. Please rename the file and try again.";
    

    Should the 240 in this string be placeholdered? I think the maxLength would be configurable via the constraint constructor right?

15. +++ b/core/modules/file/src/Plugin/Validation/Constraint/FileNameLengthConstraintValidator.php
    @@ -0,0 +1,30 @@
    +    if (strlen($file->getFilename()) > $constraint->maxLength) {
    

    should this be using mb_strlen to allow for utf-8 characters?

16. +++ b/core/modules/file/src/Plugin/Validation/Constraint/FileSizeLimitConstraintValidator.php
    @@ -0,0 +1,74 @@
    + * Validates the file name size constraint.
    

    copy paste?

17. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
    @@ -451,38 +470,6 @@ protected function validateAndLoadFieldDefinition($entity_type_id, $bundle, $fie
    -  protected function validate(FileInterface $file, array $validators) {
    -    $this->resourceValidate($file);
    -
    

    Note to other reviewers, this just uses the parent now

    however, we can probably drop the use of the trait now

18. +++ b/core/modules/file/src/Validation/FileValidationEvent.php
    @@ -0,0 +1,27 @@
    +class FileValidationEvent extends Event {
    

    Part way through review, up to here

Got to the end, couple of questions about tests

1. +++ b/core/modules/file/file.field.inc
   @@ -153,20 +153,39 @@ function template_preprocess_file_upload_help(&$variables) {
   +    @trigger_error('\'file_validate_size\' is deprecated in drupal:10.2.0 and is removed from drupal:11.0.0. Use the \'FileSizeLimit\' constraint instead. See https://www.drupal.org/node/3363700', E_USER_DEPRECATED);
   

   do we need a legacy test for this and the equivalent BC layer in the managed file element ?

2. +++ b/core/modules/file/src/Validation/FileValidationEvent.php
   @@ -0,0 +1,27 @@
   +    public readonly ConstraintViolationListInterface $violations,
   

   was going to ask 'should there be a getter for this' then realised its public, 👌

3. +++ b/core/modules/file/src/Validation/FileValidator.php
   @@ -0,0 +1,95 @@
   + * Implements the file validator.
   

   minor: can we improve this? 'Provides a class for file validation' perhaps?

4. +++ b/core/modules/file/src/Validation/FileValidator.php
   @@ -0,0 +1,95 @@
   +    $entityAdapter = EntityAdapter::createFromEntity($file);
   

   I _think_ you can just call $file->getTypedData()

5. +++ b/core/modules/file/tests/src/Functional/SaveUploadFormTest.php
   @@ -95,7 +95,7 @@ protected function setUp(): void {
   -    $this->assertFileHooksCalled(['validate', 'insert']);
   +    $this->assertFileHooksCalled(['insert']);
   

   Should we add a test event listener using the new event that writes the 'validate' entry and keep these as they were? Would reduce the size of the patch and retain the existing coverage.

6. +++ b/core/modules/file/tests/src/Kernel/Plugin/Validation/Constraint/FileExtensionConstraintValidatorTest.php
   @@ -0,0 +1,133 @@
   +    $violations = $this->validator->validate($file, $validators);
   +    $this->assertCount(1, $violations, 'Invalid extension blocked.');
   

   should we assert the message is as expected?

7. +++ b/core/modules/file/tests/src/Kernel/Plugin/Validation/Constraint/FileImageDimensionsConstraintValidatorTest.php
   @@ -0,0 +1,144 @@
   +    $violations = $this->validator->validate($this->image, $validators);
   +    $this->assertCount(1, $violations, 'Got an error for an image that was not wide enough.');
   ...
   +    $violations = $this->validator->validate($this->image, $validators);
   +    $this->assertCount(1, $violations, 'Got an error for an image that was not tall enough.');
   ...
   +    $violations = $this->validator->validate($this->image, $validators);
   +    $this->assertCount(1, $violations, 'Small images report an error.');
   ...
   +      $violations = $this->validator->validate($this->image, $validators);
   +      $this->assertCount(1, $violations, 'An error reported for an oversized image that can not be scaled down.');
   ...
   +      $violations = $this->validator->validate($this->image, $validators);
   +      $this->assertCount(1, $violations, 'Oversize images that cannot be scaled get an error.');
   

   here too, I think we should be asserting the expected message to ensure the violation is what we expect rather than just the count

8. +++ b/core/modules/file/tests/src/Kernel/Plugin/Validation/Constraint/FileIsImageConstraintValidatorTest.php
   @@ -0,0 +1,67 @@
   +    $this->assertCount(1, $violations, 'An error reported for our non-image file.');
   
   +++ b/core/modules/file/tests/src/Kernel/Plugin/Validation/Constraint/FileNameLengthConstraintValidatorTest.php
   @@ -0,0 +1,44 @@
   +    $this->assertCount(1, $violations, 'An error reported for 241 length filename.');
   ...
   +    $this->assertCount(1, $violations, 'An error reported for 0 length filename.');
   
   +++ b/core/modules/file/tests/src/Kernel/Plugin/Validation/Constraint/FileSizeLimitConstraintValidatorTest.php
   @@ -0,0 +1,33 @@
   +    $this->assertCount(1, $violations, 'Error for the file being over the limit.');
   ...
   +    $this->assertCount(1, $violations, 'Error for the user being over their limit.');
   ...
   +    $this->assertCount(2, $violations, 'Errors for both the file and their limit.');
   

   here too

9. +++ b/core/modules/file/tests/src/Kernel/Validation/FileValidatorTest.php
   @@ -0,0 +1,91 @@
   +  protected function setUp(): void {
   +    parent::setUp();
   +    $this->installConfig(['system']);
   +    $this->installEntitySchema('file');
   +    $this->installEntitySchema('user');
   +    $this->installSchema('file', ['file_usage']);
   +
   +    $uri = 'public://druplicon.txt';
   +    $this->file = File::create([
   +      'uid' => 1,
   

   if we extend the abstract base class from above I think we can remove all this setup

Removing the needs architectural review tag as I think this looks good.

The cleanup in the API resources makes it clear this is the right model, we were mixing constraints and errors in a weird mis-mash. Now its a single approach (or it will be once we can remove the BC layer).

From what I can tell all of @larowlan's points have been addressed.

🤩

+++ b/core/modules/file/src/Plugin/Validation/Constraint/FileExtensionConstraint.php
@@ -0,0 +1,41 @@
+class FileExtensionConstraint extends Constraint {

+++ b/core/modules/file/src/Plugin/Validation/Constraint/FileIsImageConstraint.php
@@ -0,0 +1,27 @@
+class FileIsImageConstraint extends Constraint {

+++ b/core/modules/file/src/Plugin/Validation/Constraint/FileSizeLimitConstraint.php
@@ -0,0 +1,53 @@
+class FileSizeLimitConstraint extends Constraint {

I think that many of these could use \Symfony\Component\Validator\Constraints\FileValidator instead? 😅 That would mean less code to maintain on the Drupal side!

Related: #3365498: Add Missing Symfony Validators to Drupal Constraint Validator.

@Wim Leers showed me this and explained a lot about how the Constraints can be leveraged. Think if we can leverage them we should!

Thanks for checking that. Will remark then.

Issue credits

Just one comment and one follow up request, feel free to self RTBC or push back if not appropriate.

1. +++ b/core/modules/file/tests/file_test/src/Form/FileTestForm.php
   @@ -94,18 +94,18 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   -      $validators['file_validate_is_image'] = [];
   
   +++ b/core/modules/file/tests/file_test/src/Form/FileTestSaveUploadFromForm.php
   @@ -141,18 +141,18 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   -      $validators['file_validate_is_image'] = [];
   

   there's a lot of duplicate code between these two, can we get a followup to move to a base-class or trait?

2. +++ b/core/modules/file/tests/src/Kernel/LegacyValidatorTest.php
   @@ -8,8 +8,9 @@
   +class LegacyValidatorTest extends FileManagedUnitTestBase {
   
   +++ b/core/modules/file/tests/src/Kernel/Plugin/Validation/Constraint/FileIsImageConstraintValidatorTest.php
   @@ -0,0 +1,68 @@
   +  protected function setUp(): void {
   +    parent::setUp();
   +
   +    $this->image = File::create();
   +    $this->image->setFileUri('core/misc/druplicon.png');
   +    /** @var \Drupal\Core\File\FileSystemInterface $file_system */
   +    $file_system = \Drupal::service('file_system');
   +    $this->image->setFilename($file_system->basename($this->image->getFileUri()));
   +
   +    $this->nonImage = File::create();
   +    $this->nonImage->setFileUri('core/assets/vendor/jquery/jquery.min.js');
   +    $this->nonImage->setFilename($file_system->basename($this->nonImage->getFileUri()));
   +  }
   

   these two classes have identical setup methods - could/should we use a base class? We have one already in this patch, could some of this duplication go there?

Committed 79c7666 and pushed to 11.x. Thanks!

Published the changed record

FYI this introduced a subtle bug whereby if a validator passed through #upload_validators did not exist as a function it was assumed to be a constraint plugin, whereas some contrib modules used it as a namespaced variable that would ultimately be passed through to hook_file_validate(). This has caused problems with several contrib modules, including Webform (#3409599: Drupal\Component\Plugin\Exception\PluginNotFoundException: The "webform_file_validate_extensions" plugin does not exist).