CKEditor file upload sets file URI prior to validation, causing validators to be unable to find the file.

Failing test.

Max and I paired on this so I'm crediting myself

New failing test patch with @larowlan's suggested changes in it (much simpler).

Fix compiled in collaboration session as patch.

I have to run so I won't lay out all the issues that led to the changes within, if the comments are not obvious enough I can answer questions/edit etc.

Ugh, typical (of me). Here we go.

The last submitted patch, 5: 3372385-5-failing-test.patch, failed testing. View results →

This prevents clamav from working with CKE5

Leaning on the test only patch, which seems to show the problem described.
And the patch seems to address that.

Don't mind marking but since it was light review please no credit.

Same fix is needed for Drupal 9.5.9

9.5 is security only

We need a separate 11.x version where the test adds an event listener instead

Just adding a patch for Drupal 9.5.9 if anyone is interested.

Thanks for the backports @pramodganore :)

The version field on the issue to track which version requires committed code so since 9.5.x is security-only I'll put it back to 10.1.x.

Here are Drupal 11 test and fix patches.

Should we do this or should we instead just do 📌 [PP-4] Unify file upload logic of REST and JSON:API Postponed ? 🤔

My gut feeling is to fix this in its own right, because its blocking clamav module (And possibly other contrib modules)

The unified uploader would only go into 10.2 which is some time off.

If there hadn't been so much excellent progress on the file system lately I would say that postponing a bug on a task doesn't seem like a great idea.

As it stands I think my stance matches @larowlan's. Solving this could prevent a lot of pain on the part of people who are now upgrading to Drupal 10.

As I understand it, Drupal 9 goes out of service the moment 10.2 is released? That isn't a huge window for a transition that may be blocked by this. Yes people can use the patch(es) but I would argue we're in a position to prevent them having to find out the patches exist and are necessary :)

👍 Forgot for a moment that that would only help new versions of Drupal; we need to fix this in all versions 😅

Review of test

1. +++ b/core/modules/ckeditor5/tests/modules/ckeditor5_image_upload_test/ckeditor5_image_upload_test.info.yml
   @@ -0,0 +1,6 @@
   +  - ckeditor5:ckeditor5
   

   Nit: this should just be ckeditor5 or alternatively drupal:ckeditor5.

2. +++ b/core/modules/ckeditor5/tests/modules/ckeditor5_image_upload_test/src/EventSubscriber/CKEditorImageUploadTestSubscriber.php
   @@ -0,0 +1,42 @@
   +      $violation = new ConstraintViolation("I'm sorry, I can't let you do that Dave.", '', [], '', '', 'invalid', NULL, 'test-code-violation-file-exists');
   

   🤣👏

3. +++ b/core/modules/ckeditor5/tests/modules/ckeditor5_image_upload_test/src/EventSubscriber/CKEditorImageUploadTestSubscriber.php
   @@ -0,0 +1,42 @@
   +    $events[FileValidationEvent::class][] = ['onFileValidation', 20];
   

   Why priority 20? 🤔 Can we add a comment describing why, or (better) can we remove it?

Review of fix

1. +++ b/core/modules/ckeditor5/src/Controller/CKEditor5ImageController.php
   @@ -257,6 +259,18 @@ protected function validate(FileInterface $file, array $validators) {
   +    // Prevent validation of the uri field because the file validation handler
   

   Nit: s/uri/URI/

2. +++ b/core/modules/ckeditor5/src/Controller/CKEditor5ImageController.php
   @@ -257,6 +259,18 @@ protected function validate(FileInterface $file, array $validators) {
   +    $fieldNames = $violations->getFieldNames();
   +
   +    // The filterByFields() method doesn't work with an empty array.
   +    if ($fieldNames === ['uri']) {
   +      $violations = new EntityConstraintViolationList($file);
   +    }
   +    else {
   +      $violations->filterByFields(array_diff($fieldNames, ['uri']));
   +    }
   

   It took me multiple reads to get this. Not your fault, but by this weird API! 😬

   I think this would be simpler to read:

   if (in_array('uri', $violations->getFieldNames(), TRUE)) {
     $violations->filterByFields(array_diff($fieldNames, ['uri']));
   }
   

   Same effect, none of the puzzlingness.

   Hope you prefer this too? :)

It took me multiple reads to get this. Not your fault, but by this weird API! 😬

I think this would be simpler to read:

if (in_array('uri', $violations->getFieldNames(), TRUE)) {
$violations->filterByFields(array_diff($fieldNames, ['uri']));
}

Same effect, none of the puzzlingness.

Hope you prefer this too? :)

No, that won't work (and is likely why we have fails?)

Calling filterByFields doesn't work if the list of fields is an empty array.

Kind of miffed that this had working patches for both branches and was moved to MRs without any commentary on what was changed and why the move and now fails. Things like that can derail and issue that was moving rapidly.

As a result I'm closing both MRs, let's keep going with the patches.

larowlan → closed merge request !4343

larowlan → closed merge request !4342

#19 addressed with the exception of the part answered in #23 which I echo. We did try that first, unfortunately it didn't work.

Riiight — I thought my proposal would ensure that it'd never be empty, but that's not the case. Let's go with what we have here. Not this issue's responsibility to solve the confusing behavior of \Drupal\Core\Entity\EntityConstraintViolationList::filterByFields().

After reviewing the excellent diagram on 📌 [META] Modernise file upload logic Active I wonder if we can remove that special casing of the URL field by doing it in the same order as file managed (IE form based) uploads

As you can see in that diagram, $file->validate() isn't called until after the file is moved to it's final destination

That would fix the issue we're special casing for, but it feels odd to accept and move the file then validate the entity. We'd end up with the file in place but then return a violation

However the bulk of $file->validate() is about validation of the entity type data and that there's a unique file, it's not about the file on disk. So maybe that is appropriate - thoughts?

Based on #28, I think we need a @kim.pepper review here.

My invitation to File System maintainership was more from my enthusiasm rather than any deep-rooted expertise. I am trying to understand it myself.

Unfortunately, I can't offer any insight other than what @larowlan provided above. That's is: it's not the same, therefore that might be why it's not working.

I'm hoping to do a deeper dive into to the current REST/JSON API/GraphQL file uploaders and put together a sequence diagram of the entire flow, so that we can all better understand exactly what is going on currently, and come up with a desired state, and a plan for how to get there.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

larowlan → credited alextars → .

larowlan → credited keshav.k → .

larowlan → credited sakthi_dev → .

Closed 🐛 CKEditor5 image upload maximum dimensions don't work Closed: duplicate as a duplicate, copying credit

I reran the tests and it's all still green so I think #32 was a false positive.

As for #28 / #31 I'm a little lost, are we postponed on another issue now?

I think we should try to align the order with how file uploads do it.

i.e not call $file->validate() until after the move

That means we don't need to worry about the ::filterByFields stuff because at that point the URL is valid

Implements ##3

Changes are back in line with existing file upload logic, and we have test coverage.

kim.pepper → credited stefanos.petrakis → .

Adding issue credit from related issue that was closed as a duplicate of this. 📌 Possibly faulty validation sequence for single image upload Closed: duplicate

The same issue applies when inline images are uploaded with maximum dimensions set (eg 100x100). If dimensions are set, the validation function called is "file_validate_image_resolution".

Sorry I'm new to posting suggestions to Drupal, so I don't really know what I'm doing. But hopefully you can reproduce the same problem I'm seeing?

The logged error contains this: "Symfony\Component\HttpKernel\Exception\UnprocessableEntityHttpException: Object(Drupal\Core\Entity\Plugin\DataType\EntityAdapter).uri.0.value: This value should be of the correct primitive type. in Drupal\ckeditor5\Controller\CKEditor5ImageController->upload()"

I was able to fix this in combination with the patch provided by larowlan, but only with the following code added just after $temp_file_path = $upload->getRealPath();:

    // Some file validation requires access to the file iteself, which is
    // currently in the temporary directory. Initially set the File object
    // with the temporary file URI and update it if the file is moved.
    $temp_directory_path = $this->fileSystem->getTempDirectory();
    if (substr($temp_file_path, 0, strlen($temp_directory_path)) === $temp_directory_path) {
      $uri = preg_replace('/^' . preg_quote($temp_directory_path, '/') . '/', '', $temp_file_path);
      $temp_file_path = 'temporary://' . ltrim($uri, '/');
    }

The code makes sure the URI is in the correct form, and contains the scheme "temporary://" instead of the real file path, which isn't suitable for the URI.

Does this help?

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

Opened 📌 Make CKEditor5ImageController reuse FileUploadHandler Active

Let's postpone this on 📌 Make CKEditor5ImageController reuse FileUploadHandler Active , but AFAICT this would just be obsolete once that lands?

AFAICT this would just be obsolete once that lands?

Yes, correct.

Let's land that then! 😄🤝

The blocker can't go into 10.1/10.2 because it needs features of 📌 Create an UploadedFile validator and deprecate error checking methods on UploadedFileInterface RTBC which only went into 11.x (10.3)

So let's unpostpone this so we can fix the existing bug without that.

Here's a re-roll of the patch in #39 which was for 10.1, now it works with 10.2
Couldn't do an interdiff, but the only thing that needed fixing was adding the

use Drupal\Core\Entity\EntityConstraintViolationList;

line back in to the correct place in CKEditor5ImageController.php

Would recommend turning into an MR.

Removing bot tag as the bot should run.

As the latest development branch is 11.x. Created a MR against 11.x. If needed will create a MR for 10.2.x.

Patch from #50 worked for me on 10.2.0.

Mixing patches and MRs now. MRs are recommended but need to clean those up so it's clear which should be reviewed.

Added typehint for return and made patches hidden.

Seems to have failures.

As per https://www.drupal.org/node/3388990 → , I think we shouldn't introduce the current user and mimetype guesser etc,. in Drupal 11. Please correct me if I'm wrong and also suggest what can be done here.

Also I think file upload handler method handleFileUpload works in proper workflow. Validating the file and if it successful then creating a file.

should this be closed as obsolete as per #46 and #47 @larowlan @kim.pepper ?

Technically this isn't fixed in 10.2 so could still be fixed there only (as a bugfix).

Once 10.3 is out and 10.2 is security only, this is obsolete.

I just tested this on Drupal 10.3 (which was released yesterday) and it seems that there is still some work to do:

This is the error in Drupal 10.2:

Uploaded file public://inline-images/my-file.jpg could not be checked, and was deleted.

and then:

Symfony\Component\HttpKernel\Exception\UnprocessableEntityHttpException: Object(Drupal\file\Entity\File): The anti-virus scanner could not check the file, so the file cannot be uploaded. Contact the site administrator if this problem persists. in Drupal\ckeditor5\Controller\CKEditor5ImageController->upload() (line 201 of /var/www/docroot/core/modules/ckeditor5/src/Controller/CKEditor5ImageController.php).

This is the error in Drupal 10.3:

Uploaded file /tmp/phpCxbjJz could not be checked, and was deleted.

And then:

Symfony\Component\HttpKernel\Exception\UnprocessableEntityHttpException: Object(Drupal\file\Entity\File): The anti-virus scanner could not check the file, so the file cannot be uploaded. Contact the site administrator if this problem persists. in Drupal\ckeditor5\Controller\CKEditor5ImageController->upload() (line 176 of /var/www/docroot/core/modules/ckeditor5/src/Controller/CKEditor5ImageController.php).

Ignore #63 it was a problem in my setup.

My error was the following:

LibClamAV Error: cli_loaddbdir(): No supported database files found in /var/lib/clamav
ERROR: Can't open file or directory

I forgot to execute freshclam to update the virus database so it was failing when testing the file.

Once I executed the file everything started working as expected.

No need to do anything else than update Drupal to version 10.3

:-)

As per #62

Once 10.3 is out and 10.2 is security only, this is obsolete.

I assume we can now mark this as closed won't fix.

Should probably be Closed outdated

Patch in #50 works for me in Drupal 10.2 version.

Patches for 10.1 and 10.2 are available are issue forks within the main issue.

Although the 11.x commit applies cleanly against 10.2, it's not compatible with the API changes introduced in 📌 Create an UploadedFile validator and deprecate error checking methods on UploadedFileInterface RTBC as stated in #49, and the 10.2 patch (if applied) should be subsequently removed upon updating to 10.3.