Opened 9 years ago

Closed 9 years ago

#13518 closed New Feature (fixed)

Request should be more flexible

Reported by: Piotr Jasiun Owned by: Tomasz Jakut
Priority: Normal Milestone: CKEditor 4.6.0
Component: General Version: 4.5.0
Keywords: Cc:

Description (last modified by Piotr Jasiun)

It should be easy to add some additional parameters to the request. Parameters defined in the upload widget definition should be passed to the fileLoader, to the fileUploadRequest and finally to the request build by the default fileUploadRequest handler. So for example:

fileTools.addUploadWidget( editor, 'sepiauploadimage', {
    additionalRequestParameters: { sepia: true },

    // ...
} );

Will add evt.data.additionalRequestParameters.sepia = true to the fileUploadRequest and send it to the server (formData.append( 'sepia', 'true');).

Change History (15)

comment:1 Changed 9 years ago by Piotr Jasiun

Version: 4.5.04.5.0 Beta

comment:2 Changed 9 years ago by Piotr Jasiun

Description: modified (diff)

comment:3 Changed 9 years ago by Piotr Jasiun

Description: modified (diff)

comment:4 Changed 9 years ago by Piotr Jasiun

Additionally we could handle files the same way. Then evt.data.additionalRequestParameters chould be called simply evt.data.formData and contains all data which will be send by the default fileUploadRequest listener. One listener would add to it:

editor.on( 'fileUploadRequest', function( evt ) {
    evt.data.formData.upload = evt.data.fileLoader.file;
} );

Thanks to it fileUploadRequest would be even more generic and user would be able to easily change the name of the form data (see https://github.com/ckeditor/ckeditor-dev/pull/197).

comment:5 Changed 9 years ago by Piotr Jasiun

Version: 4.5.0 Beta

comment:6 Changed 9 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.6.0
Status: newconfirmed
Version: 4.5.0

comment:7 Changed 9 years ago by Tomasz Jakut

Owner: set to Tomasz Jakut
Status: confirmedassigned

comment:8 Changed 9 years ago by Tomasz Jakut

Status: assignedreview

Changed the way of adding data to the request to using evt.data.formData. Pushed branch:t/13518

comment:9 Changed 9 years ago by Piotr Jasiun

Status: reviewreview_failed

First of all, this ticket was about introducing new parameters on all levels: introduce the way of adding them to the upload widget, adding them to the fileLoader interface and finally passing to the fileUploadRequest, so different upload widgets can use different additional parameters.

I understand that adding formData to fileUploadRequest event is enough to add some parameters in the fileUploadRequest event listener, but if user create two upload widgets he may want to set different parameters for them. Recognizing which request comes from with widget may not be simple, especially if both of them use the same upload URL.

Beside that:

comment:10 Changed 9 years ago by Tomasz Jakut

Status: review_failedreview

Ok, fixed all of above & pushed branch:t/13518 again.

comment:11 Changed 9 years ago by Piotr Jasiun

Status: reviewreview_failed

Much better, but I still have some doubts.

  • additionalRequestParameters should have a sample in the documentation how to use it and how it works and some cross links to other parts of code this parameter is used. This is not trivial how to use it properly. Note that such features without comprehensive documentation are useless.
  • The same about data.formData - this parameter also need better documentations, Especially there is missing information how to handle files there. If one wants to add some additional data to all requests sent from the editor, this parameter is a perfect place to do it, what should be also mentioned with an example.
  • dataToUpload = evt.data.formData; I would prefer formData = evt.data.formData;, but dataToUpload also makes some sens not to have formData and $formData in the same place.
  • There is a full integration test missing. There is a test that setting widget definition property call upload with additionalRequestParameters and some tests if the additionalRequestParameters event works properly, but I am not able to check whole process easily. The integration test that prove that setting widget parameter adds the request parameter would be useful. It might be a manual test.

comment:12 Changed 9 years ago by Piotr Jasiun

In the #13519 I changed response property to responseData and I realize that we could call data here requestData instead of formData. There would be no naming conflict with the formData function and the API would be more consistent.

comment:13 Changed 9 years ago by Tomasz Jakut

  • data.formData was renamed to data.requestData
  • Docs for additionalRequestParameters and data.requestData added
  • Integration test added

Pushed branch:t/13518

comment:14 Changed 9 years ago by Tomasz Jakut

Status: review_failedreview

comment:15 Changed 9 years ago by Piotr Jasiun

Resolution: fixed
Status: reviewclosed

Closed with git:b01cee9 Good job.

But one more change is needed. You put requestData description in the fileUploadRequest parameter docs (https://github.com/cksource/ckeditor-dev/commit/218ddcccd151db5a4a09e8680addc0054f669602) but the whole fileUploadRequest docs and rest of examples are in the Uploading Dropped or Pasted Files Guide (https://github.com/ckeditor/ckeditor-docs/blob/master/guides/dev/configuration/file_browse_upload/file_upload/README.md#request-1) and this docs should goes there too. I makes more sens to keep all examples how to use fileUploadRequest together. I removed the description from the ckeditor-dev repo, please add it to the ckeditor-docs.

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy