Problem/Motivation

As part of #2940383: [META] Unify file upload logic of REST and JSON:API and a follow up to #3401734: Refactor FileUploadResource to use FileUploadHandler we want to factor out common file upload code. Duplicated code has been the source of numerous bugs and security issues in the past.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3444748

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
Status: Active » Postponed

This is postponed on #3401734: Refactor FileUploadResource to use FileUploadHandler as we are adding some new classes there that we will need here.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Title: [PP-1] Refactor JSON-API file uploads to use FileUploadHandler » Refactor JSON-API file uploads to use FileUploadHandler
Status: Postponed » Active

#3401734: Refactor FileUploadResource to use FileUploadHandler is in so this is unblocked. 🙌

kim.pepper’s picture

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

kim.pepper opened merge request !7915

kim.pepper’s picture

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

andypost
he/him
Primary language Russian
commented
Status: Needs review » Needs work

one test failed and I left a question for constructor's BC

kim.pepper opened merge request !7918

kim.pepper’s picture

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

Created MR for 10.4.x with BC layer.

kim.pepper’s picture

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

Feedback addressed and MR for 10.4.x created with BC layer.

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs review » Needs work

Awesome to see this moving!

Quick contrib search:

  1. jsonapi/Controller/FileUpload - 0 usage
  2. jsonapi\Controller\TemporaryJsonapiFileFieldUploader - 0 usage
  3. jsonapi.file_upload - 1 usage

Seems we are not really doing anything impactfull against contrib.

I've checked the CR, its small, but fine i think.

BC paths make sense for new arguments for the controller.

Gone though the code, have some questions :) This feels rather close, and i'll be quite happy to have the "Temporary" class killed :D

kim.pepper’s picture

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

Thanks for the review. I've addressed all feedback.

This feels rather close, and i'll be quite happy to have the "Temporary" class killed :D

Committed on 20 Mar 2019 so temporary for 5 years. 🤣

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs review » Reviewed & tested by the community

Everything in #11 has been adressed.

Think this is fine. BC is a bit hard to fully consider, but i think we have enough. All threads have been resolved, RTBC for me <3

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Can we change the 10.4.x deprecations to be for 10.3.x - I think it is the best option because I think doing 10.3.x deprecation and remove in 11 would put us in a better situation wrt to security. Having something that can do file uploads around for the whole 11.x cycle that is unused and untested feels like a v bad idea.

bbrala’s picture

bbrala
Location Netherlands
commented

Sounds like a playing. I'll wait for Kim to update and review the changes.

kim.pepper’s picture

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

Changed to `drupal:10.3.0` for removal in `drupal:11.0.0`.

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs review » Needs work

You still need to chnage the target of the mr I think. It's still targeting 10.4

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Reviewed & tested by the community

@bbrala as long as it applies to 10.3.x we're fine - and it will. Given this only text changes I think the rtbc from #13 stands.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed a52cdfad63 to 11.x and 007556a098 to 11.0.x. Thanks!
Committed and pushed b10e96de15 to 10.4.x and be2b4ec313 to 10.3.x. Thanks!

  • alexpott committed be2b4ec3 on 10.3.x 
    Issue #3444748 by kim.pepper, bbrala, andypost: Refactor JSON-API file...

  • alexpott committed b10e96de on 10.4.x 
    Issue #3444748 by kim.pepper, bbrala, andypost: Refactor JSON-API file...

  • alexpott committed 007556a0 on 11.0.x 
    Issue #3444748 by kim.pepper, bbrala, andypost: Refactor JSON-API file...

  • alexpott committed a52cdfad on 11.x 
    Issue #3444748 by kim.pepper, bbrala, andypost: Refactor JSON-API file...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
kim.pepper’s picture

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

Oh wow. This wraps up years of effort unifying file uploads. 🥲

bbrala’s picture

bbrala
Location Netherlands
commented

Hehe, thanks Alex :)

Congrats Kim! :)

xjm’s picture

xjm
she/her
Primary language English
commented
Version: 11.x-dev » 10.3.x-dev

Status: Fixed » Closed (fixed)

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

kim.pepper closed merge request !7915

kim.pepper closed merge request !7918