[META] Unify file upload logic of REST and JSON:API

I was testing #150 using json api + private files upload + anonymous user and it works for 1 file (i can see deprecations warning) but for many files it looks like upload override files id in "anonymous_allowed_file_ids" so there is no option to save a entity because validation fails.

The good news is I think I found why the `file_validate_extensions` was reverting back to default extensions instead of allowing any. The bad news is this opened up a whole new list of test fails.

Fix spelling typo.

The last submitted patch, 164: 2940383-164.patch, failed testing. View results →

I've added a specific kernel test to check the access in \Drupal\file\Upload\FileFieldUploadAccessChecker. The code is taken from \Drupal\Tests\jsonapi\Kernel\Controller\TemporaryJsonapiFileFieldUploaderTest.

I've created an abstract base class in response to the feedback in #154. I hope this addresses those concerns.

We still need to work out the test fails which appear to be access related. I'm hoping to get help with these.

The last submitted patch, 166: 2940383-166.patch, failed testing. View results →

Code in \Drupal\jsonapi\Controller\TemporaryJsonapiFileFieldUploader::checkFileUploadAccess had changed since it was copied over into \Drupal\file\Upload\FileFieldUploadAccessChecker::checkFileUploadAccess. The new test picked that up. :-D

The last submitted patch, 168: 2940383-168.patch, failed testing. View results →

Reroll and attempt to address the failures

Remaining tasks are to address feedback in:

• #151
• #154
• #157

Added those to issue summary, fixed some more tests that failed locally with new changes

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

Addressing phpstan issues

Excited to see this moving forward again!

All of the interdiff between #146 and #150 looks like wrong “fixes” that introduced bugs. Wish Kim had re-started from 146, instead. 😅 I wonder if those bugs have subsequently been fixed, or if they mean we’re still missing test coverage. 😉 hope to look more closely when I’m not on my phone.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

Adds deprecation to \Drupal\jsonapi\Controller\TemporaryJsonapiFileFieldUploader and service. Fixes phpstan issue.

Looping back to address feedback in #151.

1. I moved FileFieldDefinitionResolver::resolveFieldDefinition() back to \Drupal\jsonapi\Controller\FileUpload::resolveFieldDefinition. I think I assumed it would be used in more places than just the jsonapi module.
2. no longer applicable
3. This was copied straight from the existing code:
   

   +++ b/core/modules/jsonapi/src/Controller/FileUpload.php
   @@ -213,38 +315,4 @@ protected static function ensureFileUploadAccess(AccountInterface $account, Fiel
   -    if ($field_definition->getSetting('target_type') !== 'file') {
   

4. changed this to 'file entity'
5. fixed earlier
6. This seems overly nitpicky. Have a look at every other throws comment in core.
7. Again, this is taken from the original code. I don't know where that decision was originally made.
8. Fixed
9. Fixed earlier.

Looks like there was a problem uploading the patch files. Will try again...

Seems #180 is adding

php-http/discovery

is that intentional? Don't see it in the issue summary about adding a new package.

#182 that was unintentional. Removed.

Sorry for not catching this before but CR could use another look. Seems it was started in 8.8

Also can the deprecation be updated for 10.2, think 10.1 was missed.

Adding another reason to have a unified uploader. 🐛 [PP-1] CKEditor file upload sets file URI prior to validation, causing validators to be unable to find the file. Postponed

@kim.pepper++

I don't think this should hold up a bug fix that could potentially be backported.

Although I have spent hours on this issue, a lot of it has been discovery. I think we should make sure we have a vigorous architectural review before moving ahead.

I've added a sequence diagram showing the complexity of the current state of form uploads to the meta issue 📌 [META] Modernise file upload logic Active

Re-titling the issue as cores file upload doesn't follow the same logic. It uses UploadedFile objects from the request, and not a direct stream upload like REST and JSON API.

For core file upload refactoring see 📌 Deprecate file_managed_file_save_upload(), file_save_upload() and _file_save_upload_from_form() and replace with a service Needs review

Rather than eating the elephant in one go, I'm breaking this up into more bite-sized pieces.

I think we should postpone this issue on the following:

• 📌 Create an UploadedFile validator and deprecate error checking methods on UploadedFileInterface RTBC
• 📌 Create a InputStreamFileWriter for writing the input stream to a file RTBC
• 📌 Create content disposition filename extractor and deprecate duplicate code in REST and JSON API RTBC
• 📌 Provide a trait to create file upload validators from settings Needs review

One of the blockers is in

Incredible 🤯

Two more of the blockers are in:

• 📌 Create a InputStreamFileWriter for writing the input stream to a file RTBC
• 📌 Create content disposition filename extractor and deprecate duplicate code in REST and JSON API RTBC

Added additional task from https://git.drupalcode.org/project/drupal/-/merge_requests/4866#note_230080

Last remaining blocker 📌 Provide a trait to create file upload validators from settings Needs review is in so we are un-blocked!

🤩🙏🙏🙏🙏

The next step forward for this effort is now ready for reviews: 📌 Refactor FileUploadResource to use FileUploadHandler RTBC

Marking this as a meta issue now have two child issues that we want to do one at a time:

• 📌 Refactor FileUploadResource to use FileUploadHandler RTBC
• 📌 [PP-1] Refactor JSON-API file uploads to use FileUploadHandler Postponed

📌 Refactor FileUploadResource to use FileUploadHandler RTBC is in 🎉 which unblocks 📌 [PP-1] Refactor JSON-API file uploads to use FileUploadHandler Postponed

📌 [PP-1] Refactor JSON-API file uploads to use FileUploadHandler Postponed Just landed so I think we are safe to mark this fixed now.

Thanks to everyone here for the years of work and support to get us to this point. Much appreciated. 🫶

No doubt there will be follow up issues to further refine and improve the code re-use here, but I think we are now in a much more secure situation that before.

WOWWWWWWWWWWWWWWWWWWWW!!!!!!!!!!!!!!!!!

We all owe @kim.pepper thanks, beers, lemonades, applause and gratitude for many years to come, because thanks to his unrelenting effort Drupal is now finally consistent in this very security-sensitive area! 🤩🥳

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