Problem/Motivation

https://www.drupal.org/node/3363700 advises modules to move from hook_file_validate to use FileValidationEvent instead.

It appears that in some cases, this is not a suitable replacement.

Steps to reproduce

ClamAV contrib module switched from hook_file_validate to FileValidationEvent and it appears that in some cases, the file scanning logic now breaks.

If we log file size (uploading test PNG file on one of the content types - image field, but same problem with other file upload fields) as reported by the FileInterface in both FileValidationEvent and hook_file_validate, it seems they return different data.

$file_size = filesize($file->getFileUri());
\Drupal::logger('File validate')->error('Size of file is: ' . $file->getSize() . ', URI is: ' . $file->getFileUri() . ' file size is: ' . $file_size);

Using hook_file_validate, this logs

Size of file is: 387365, URI is: /tmp/phpzipzoE file size is: 387365

but using FileValidationEvent logs

Size of file is: 387365, URI is: /tmp/phpuX8xcs file size is: 506707

Should modules such as ClamAV use some other event than FileValidationEvent to make sure the physical file is the correct one and fully processed before doing custom validations?

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Comments

klemendev created an issue. See original summary.

quietone’s picture

quietone commented
Version: 10.5.x-dev » 11.x-dev
Issue summary: View changes

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Category: Bug report » Support request
Status: Active » Needs work

The event is for altering validation constraints. If you are creating a new one, you probably want to implement your own ConstraintValidator plug-in that extends \Drupal\file\Plugin\Validation\Constraint\BaseFileConstraintValidator.

klemendev’s picture

klemendev commented

Looking at this change record - https://www.drupal.org/node/3363700 - it recommends replacing hook_file_validate with FileValidationEvent. Is the change record wrong in this case? Because, as explained at https://www.drupal.org/project/clamav/issues/3503176#comment-15967597, the module adapted the recommended event replacement

klemendev’s picture

klemendev commented

Also, shouldn't this event still provide the correct file size when uploading, as if you check my post, the uploaded file data is not correct when using FileValidationEvent?

klemendev’s picture

klemendev commented
Category: Support request » Bug report
klemendev’s picture

klemendev commented

This may also be happening because the images are downsized if too big, and then two different sizes are in question - the true size and the resized one. Related: https://www.drupal.org/project/drupal/issues/3292350#comment-16093652

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented

As just an interested reader, arriving here via the CR (https://www.drupal.org/node/3363700) -> #3503176: File upload stopped working after updating to 2.1.0:

#4

Looking at this change record - https://www.drupal.org/node/3363700 - it recommends replacing hook_file_validate with FileValidationEvent. Is the change record wrong in this case?

Looks to me like the change record isn't wrong, and there may be a bit of terminology mix-up going on here. Maybe you are aware of this already, but then I'm just summarizing for other readers:

The event is specifically for altering the behavior of FileValidator. That is, usually: add extra constraints to the validator.

ClamAV does that. And I don't see any indication that ClamAV should be doing anything else.

#5

shouldn't this event still provide the correct file size

I guess yes, but the event can't help that it's wrong. The event shouod be provided the correct file size. It's the responsibility of whichever code calls the event, or (more likely) some code that updates the file before that, to make sure the file object's size property is correct.

This actually means (seems to me) that the title of this issue is wrong, and FileValidationEvent is fine. As long as it doesn't get a file object with wrong/outdated info.

And the related #3522463: FileImageDimensionsConstraintValidator does not update file size (regression) (file resizing) feels like a plausible candidate for code that's buggy and should fix the file size in the file object. (I can't judge that without diving into that code.) So this issue can be closed "(works as designed)" (perhaps after #3522463: FileImageDimensionsConstraintValidator does not update file size (regression) is fixed).

EDIT: ...the more precise reason this could be closed is that the bug mentioned in your issue description, which is also in the description of #3522463: FileImageDimensionsConstraintValidator does not update file size (regression), seems to be fixed there, as proven out by a test that I see in the patch.

(EDIT: ha! And I didn't spot that that patch is only a few hours old, while typing all this.)

o'briat’s picture

o'briat
Primary language French
Location Nantes
commented

I think the core problem is that FileImageDimensionsConstraintValidator changes the file size during a validation process, according to https://www.drupal.org/project/drupal/issues/3363745 this validation process should only does checks and returns info/errors.

This quick fix is to update the object's file size after resizing it (done again in https://www.drupal.org/project/drupal/issues/3522463), the long way is (IMHO) to redefine the API/events workflow of file Validator.

Should this issue closed and marked as duplicated of https://www.drupal.org/project/drupal/issues/3522463 ?

klemendev’s picture

klemendev commented
Status: Needs work » Closed (duplicate)

Closing as duplicate of https://www.drupal.org/project/drupal/issues/3522463