Move file upload validation from file.module to constraint validators

I'm shifting away from Symfony Validators to a (hopefully) simpler approach in 📌 Allow validators passed to file_validate to use callable syntax Closed: outdated

Re-roll to see what's breaking.

The last submitted patch, 22: 3221793-22.patch, failed testing. View results →

Fix test fails

I wonder if we should go further up the stack and replace '#upload_validators' in the Form API with an array of \Symfony\Component\Validator\Constraint instances? This probably means replacing how form validation works to be more inline with Symfony Validator component, and there seems to be some pushback on using it at all 📌 Discuss whether to decouple from Symfony Validator Active

Also not sure how the form API and existing validators tie into the whole Typed Data API and plugin validators like \Drupal\file\Plugin\Validation\Constraint\FileValidationConstraintValidator.

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?

Thanks @joachim. I'm still looking for the right approach for this.

Re: #25 Should we introduce Constraints to the Form API? Or should we just focus on upload validators and converting them over?

Re #26 If we do start to use Constraints, should we try and reuse the plugins that typed data api uses?

Chatted with @larowlan about this at DrupalSouth and decided the best approach is utilise constraint validation plugins.

We can create plugins with the same IDs as the function names for BC, and deprecate the functions.

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

Starting on the new approach described above in #29 and #30.

Posting here for architectural review.

Added an additional validation for FileExtension to test passing in arguments. We will need to use the existing '$value' option as we don't currently an associative array of options, just the function arguments.

I also updated the issue summary with the current approach and remaining tasks.

The last submitted patch, 34: 3221793-34.patch, failed testing. View results →

Still a work in progress. Did more work building out constraints and added logic for deprecating existing hook functions.

Ran into file_validate_image_resolution() which I think doesn't belong as a validator. I created 📌 Deprecate file_validate_image_resolution() and replace with a service Postponed which I think is a blocker for this issue.

If CC error could be addressed.

Re: #37 I dug a bit deeper and trying to remove file_validate_image_resolution() from the validators is going to be complex. I have added a \Drupal\file\Plugin\Validation\Constraint\FileImageDimensionsConstraint. Happy to hear opinions on how we can do this better.

Remaining tasks:

• Deprecate the legacy functions
• Replace usages in core

The last submitted patch, 39: 3221793-39.patch, failed testing. View results →

Fixes test fails and adds a test for FileNameLengthConstraintValidator.

Adds a test event subscriber.

Deprecates file_validate() and replaces usages.

Remaining tasks:

• Deprecate the legacy file validate hook functions
• Deprecate file_validate()

The last submitted patch, 43: 3221793-43.patch, failed testing. View results →

Replaced more usages of file_validate() and file_validate_* functions.

The last submitted patch, 45: 3221793-45.patch, failed testing. View results →

Fixing test fails.

The last submitted patch, 47: 3221793-47.patch, failed testing. View results →

JSON API has an undeclared dependency on file module so a lot of these fails are because we have a new file service that will error out when the container is built, whereas before we were just calling the file_validate() function and not checking if it was available.

Added 'file' as a module dependency to a load of the JSON-API tests.

The patch grows bigger, and the night grows darker...

The last submitted patch, 49: 3221793-49.patch, failed testing. View results →

Fixes tests.

Remaining tasks:

• Ensure we have the same test coverage for new ConstaintValidators as we did for legacy file_validate_* functions

The last submitted patch, 51: 3221793-51.patch, failed testing. View results →

Trying a different approach to fix test issue by checking $file instanceof ConstraintViolationListInterface instead of $file instanceof EntityConstraintViolationListInterface.

1. Renamed FileSizeMax to FileSizeLimit.
2. Copied over existing file_validate_* tests to ensure we don't lose any test coverage.

No more remaining tasks! Just reviews needed.

The last submitted patch, 54: 3221793-54.patch, failed testing. View results →

Don't use markup in test assertions.

Updated the CR to provide much more details and mapping old to new form API changes.

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

Fyi expectDeprecation is deprecated itself but seems 📌 Several expect*() methods have been deprecated in PHPUnit 9.6 Active 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

1. 👍️
2. Updated CR
3. Fixed
4. Fixed
5. 👍️
6. Fixed
7. Fixed
8. Fixed
9. Fixed
10. We can't return early as we might add another minDimensions violation further down.
11. Fixed
12. Fixed
13. Fixed
14. Fixed
15. Fixed. Although this is existing functionality.
16. Fixed
17. 👍️
18. 👍️

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).

1. Added a legacy test \Drupal\Tests\file\Kernel\LegacyFileThemeTest::testTemplatePreprocessFileUploadHelp()
2. 👍️
3. Fixed
4. I think it just calls ::createFromEntity() underneath, but lets find out.
5. I modified the file_validator_test module's event listener to set the test state that the FileUploadTest was previously checking, and re-added the check for 'validation' hook being fired (event though it's an event now). I had to rework the FileValidatorTest as we are no longer adding a violation in the event listener.
6. Removed this test as it's more fully covered by the tests below that use the dataProvider.
7. Fixed
8. Fixed
9. Fixed. Moved it from Drupal\Tests\file\Kernel\Plugin\Validation\Constraint\FileConstraintValidatorTestBase to \Drupal\Tests\file\Kernel\Validation\FileValidatorTestBase

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: 📌 Add Missing Symfony Validators to Drupal Constraint Validator Needs work .

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

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!

Had a look through the Symfony constraints. As mentioned in Slack, we have a \Drupal\file\FileInterface and \Drupal\Core\Image\ImageInterface we are validating, so the Symfony validators won't work.

Thanks for checking that. Will remark then.

Re-roll of #62

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?

1. Added follow-up issue 📌 Remove duplication from FileTestForm and FileTestSaveUploadFromForm Active
2. LegacyValidatorTest will be removed in the next major release, so we'd end up with only one use of the trait or base class.

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

Published the changed record

Automatically closed - issue fixed for 2 weeks with no activity.

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 lots of 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 ( 🐛 Drupal\Component\Plugin\Exception\PluginNotFoundException: The "webform_file_validate_extensions" plugin does not exist Active ).

A new bug reported for the BC layer: 🐛 file_save_upload broken by 10.2.x Needs work