Problem/Motivation

In #3221796: [META] Modernise file upload logic we identified a common need for REST and JSON API file uploads is to stream uploads from php://input.

Let's create a service to handle that.

Steps to reproduce

Proposed resolution

Remaining tasks

  • Create a InputStreamFileWriter
  • Add usage in REST
  • Add usage in JSON API
  • Add a change record

User interface changes

API changes

A new InputStreamFileWriter service is available for writing the input stream to a temporary file.

Data model changes

Release notes snippet

Issue fork drupal-3380345

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

kim.pepper created an issue. See original summary.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Title: Create a InputStreamUploader for non-form uploads » Create anStreamFileUploader for non-form uploads
Issue summary: View changes
Status: Active » Needs review

Remaining tasks:

  • Create a StreamFileUploader
  • Add usage in REST
  • Add usage in JSON API
kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Title: Create anStreamFileUploader for non-form uploads » Create a StreamFileUploader for non-form uploads

kim.pepper opened merge request !4573

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Issue summary: View changes
  • Added usage for REST
  • Added usage for JSON API
kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Issue summary: View changes
Issue tags: +Needs change record

A couple of remaining tasks:

  • Add a change record
  • Add deprecations
kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Issue summary: View changes
Issue tags: -Needs change record

Added a change record. There is nothing to deprecate as we are just replacing usage in protected methods.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Change looks good. Updated CR to include the name of the new service.

quietone’s picture

quietone commented

I'm triaging RTBC issues. I read the IS, the comments and the comments in the MR. I didn't find any unanswered questions or other work to do.

Leaving at RTBC.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Title: Create a StreamFileUploader for non-form uploads » Create a InputStreamFileWriter for writing the input stream to a file
Issue summary: View changes

Working through all the issues under #3221796: [META] Modernise file upload logic the name "uploader" is a very over-used term. Using it here "StreamFileUploader" is a bit confusing. I think InputStreamFileWriter::writeStreamToFile() is a much clearer name to describe what it does.

Updated the code, issue title & summary and change record to match.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Priority: Normal » Critical

Blocking a critical #2940383: [META] Unify file upload logic of REST and JSON:API therefore this is also critical.

quietone’s picture

quietone commented

@kim.pepper, thanks for not adding more instances of the word 'uploaders'. That word is in dictionary.txt and we are trying to remove all the misspellings from that file.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs work

This needs a reroll.

Conceptually it looks like a great cleanup, duplicated code from the two modules is consolidated. This probably should have happened before JSON:API went into core, but better late than never.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Reviewed & tested by the community

Re-rolled.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs work

Left some comments on the MR

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Needs review

Resolved feedback.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Bummer about missing 10.2 but feedback appears to have been addressed.

  • larowlan committed 4d6f38f6 on 11.x 
    Issue #3380345 by kim.pepper, smustgrave, quietone, larowlan: Create a...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Title: Create a InputStreamFileWriter for writing the input stream to a file » Create an InputStreamFileWriter for writing the input stream to a file
Status: Reviewed & tested by the community » Fixed

Committed to 11.x and published the change record.

Thanks!

kim.pepper closed merge request !4573

Status: Fixed » Closed (fixed)

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

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Related issues: +#3415422: Fix incorrect change record link in FileUploadResource and TemporaryJsonapiFileFieldUploader constructors

We forgot to update the CR links in the deprecation trigger. Created #3415422: Fix incorrect change record link in FileUploadResource and TemporaryJsonapiFileFieldUploader constructors

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

Created another follow up for deprecating BYTES_TO_READ #3415706: Deprecate unused constant BYTES_TO_READ in FileUploadResource and TemporaryJsonapiFileFieldUploader