Make CKEditor5ImageController reuse FileUploadHandler

Seems failure could be related to change.

Sweet seeing all green! Tagging for CR

Thanks @kim.pepper reads well.

I didn't notice this because it's not in the component that the code actually lives in 😅

Looks fantastic!

But, 2 questions, one of which is a bug in the logic (although it does not currently cause problems, hence the passing tests, but it'll make future maintenance more difficult/confusing).

Would this make #3246260: Simplify CKEditor5ImageController once #2940383 lands obsolete too, or is that a further evolution of Drupal core's file upload handling?

All threads have been resolved.

Save credit to @Wim Leers for his review.

RTBC++

Blocker is in

Deprecation seems correct
CR is present and makes sense about what is now needed.

Reviewed this in detail. It looks excellent :)

We have detailed test coverage for this:

1. \Drupal\Tests\ckeditor5\Functional\ImageUploadTest::testUploadFileExtension()
2. \Drupal\Tests\ckeditor5\Functional\ImageUploadTest::testFileUploadLargerFileSize()
3. \Drupal\Tests\ckeditor5\Functional\ImageUploadTest::testLockAfterFailedValidation()

(Note that each of these asserts a 422 response is received when trying to upload something that is not allowed 👍

That's why I can confidently confirm this RTBC 😊

Just one minor issue with the deprecation messages

While this is RTBC it is only a refactoring; #3280279: Allow sites to programmatically opt in to accept more image type uploads in CKEditor 5: TIFF, SVG… is a major feature request that wants to make changes to the controller constructor as well, and I think it is worth postponing this issue on that one in order to try and get that into 10.2.x first; this can only go into 10.3.x/11.x as the parent issue did not land in 10.2.x.

Yes, this fixes #3372385: CKEditor file upload sets file URI prior to validation, causing validators to be unable to find the file.

But because it relies on code from the parent issue that is 10.2 we're kind of hamstrung.

We might need to unpostpone #3372385: CKEditor file upload sets file URI prior to validation, causing validators to be unable to find the file. and reinstate the old approach there

I think there's been a misunderstanding here. #3375447: Create an UploadedFile validator and deprecate error checking methods on UploadedFileInterface did not actually land only in 11.x … because 10.2.x was branched from 11.x AFTER #3375447 landed! That is the confusing part here 😅

EDIT: I realized this only yesterday, at #3280279-53: Allow sites to programmatically opt in to accept more image type uploads in CKEditor 5: TIFF, SVG….

So: moving back to RTBC: this can land first just fine (and I think @kim.pepper's reasons above are sufficiently convincing — tagging accordingly), and then we can land the #3280279: Allow sites to programmatically opt in to accept more image type uploads in CKEditor 5: TIFF, SVG… afterwards 👍

Not at computer to check but that is not what I remember nor what #3375447-26: Create an UploadedFile validator and deprecate error checking methods on UploadedFileInterface says... which is why I would prefer to get the other issue in first before diverging 10.2.x and 10.3.x/11.x any further.

See https://www.drupal.org/project/drupal/issues/3375447#comment-15284269 the other issue was 11.x only, I confirmed with git

I'll unpostpone #3372385: CKEditor file upload sets file URI prior to validation, causing validators to be unable to find the file.

I don't think we should postpone this on the feature addition though, this is a nice piece of cleanup for 11.x part of a larger string of criticals that consolidate file upload handling.

I read the issue summary and the comments. I did not find any unanswered questions or other work to do. I also think the change record is correct.

Went to commit #3280279: Allow sites to programmatically opt in to accept more image type uploads in CKEditor 5: TIFF, SVG… first but it has a phpcs fail.

So this one wins the 'who goes in first' prize.

Committed to 11.x and published change record.

Unpostponed #3389016: Add file upload lock handling to FileUploadHandler

Since FileUploadHandler doesn't have an interface and this plugin now hardcodes the used class, you can no longer decorate the file.upload_handler service and use this plugin without fatal errors ¯\_(ツ)_/¯

That seems worth an issue to add an interface