Make CKEditor5ImageController reuse FileUploadHandler

Changed CKEditor5ImageController to use FileUploadHandler.

I needed to add two things to FileUploadHandler to support this:

1. Added \Drupal\Core\File\FileSystemInterface::prepareDirectory() to create the destination sub-directory
2. Added \Drupal\Core\Lock\LockBackendInterface::acquire() / ::release() which gets duplicated by JSON API and REST too, so we can move more logic to shared code in 📌 Deprecate file_managed_file_save_upload(), file_save_upload() and _file_save_upload_from_form() and replace with a service Needs review

I rolled back the changes to FileUploadHandler.

Probably the most complex bc handling constructor I have tried so far.

Created a new issue for 📌 Add file upload lock handling to FileUploadHandler Needs review

Seems failure could be related to change.

Sweet seeing all green! Tagging for CR

Updated change record.

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 📌 [PP-1] Simplify CKEditor5ImageController once #2940383 lands Postponed obsolete too, or is that a further evolution of Drupal core's file upload handling?

Addressed all feedback.

All threads have been resolved.

RTBC++

I think we should postpone on 📌 Create an UploadedFile validator and deprecate error checking methods on UploadedFileInterface RTBC as we won't need to do the exception handling.

Blocker is in

Updated to use constraint violations.

Postponing 📌 Add file upload lock handling to FileUploadHandler Needs review on this.

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

Back to RTBC (assuming green).

While this is RTBC it is only a refactoring; ✨ Allow sites to programmatically opt in to accept more image type uploads in CKEditor 5: TIFF, SVG… Needs work 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.

Well, this is disappointing given its blocked on a feature request, while this is removing duplicate code which has been the cause of numerous bugs (including security issues).

I would argue this has higher priority.

Yes, this fixes 🐛 [PP-1] CKEditor file upload sets file URI prior to validation, causing validators to be unable to find the file. Postponed

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

We might need to unpostpone 🐛 [PP-1] CKEditor file upload sets file URI prior to validation, causing validators to be unable to find the file. Postponed and reinstate the old approach there

I think there's been a misunderstanding here. 📌 Create an UploadedFile validator and deprecate error checking methods on UploadedFileInterface RTBC 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 😅

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 ✨ Allow sites to programmatically opt in to accept more image type uploads in CKEditor 5: TIFF, SVG… Needs work 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...

See https://www.drupal.org/project/drupal/issues/3375447#comment-15284269 📌 Create an UploadedFile validator and deprecate error checking methods on UploadedFileInterface RTBC the other issue was 11.x only, I confirmed with git

I'll unpostpone 🐛 [PP-1] CKEditor file upload sets file URI prior to validation, causing validators to be unable to find the file. Postponed

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 ✨ Allow sites to programmatically opt in to accept more image type uploads in CKEditor 5: TIFF, SVG… Needs work 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 📌 Add file upload lock handling to FileUploadHandler Needs review

larowlan → closed merge request !4846

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

codebymikey → changed the visibility of the branch 3388985-make-ckeditor5imagecontroller-reuse-10.1.x to hidden.

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

Created 📌 Add an interface for FileUploadHandler Active