#13518 closed New Feature (fixed)

Request should be more flexible

Reported by: pjasiun Owned by: t.jakut
Priority: Normal Milestone: CKEditor 4.6.0
Component: General Version: 4.5.0
Keywords: Cc:

Description (last modified by pjasiun)

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 22 months ago by pjasiun

  • Version changed from 4.5.0 to 4.5.0 Beta

comment:2 Changed 22 months ago by pjasiun

  • Description modified (diff)

comment:3 Changed 22 months ago by pjasiun

  • Description modified (diff)

comment:4 Changed 22 months ago by pjasiun

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 22 months ago by pjasiun

  • Version 4.5.0 Beta deleted

comment:6 Changed 21 months ago by Reinmar

  • Milestone set to CKEditor 4.6.0
  • Status changed from new to confirmed
  • Version set to 4.5.0

comment:7 Changed 19 months ago by t.jakut

  • Owner set to t.jakut
  • Status changed from confirmed to assigned

comment:8 Changed 19 months ago by t.jakut

  • Status changed from assigned to review

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

comment:9 Changed 19 months ago by pjasiun

  • Status changed from review to review_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 19 months ago by t.jakut

  • Status changed from review_failed to review

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

comment:11 Changed 19 months ago by pjasiun

  • Status changed from review to review_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 19 months ago by pjasiun

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 19 months ago by t.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 19 months ago by t.jakut

  • Status changed from review_failed to review

comment:15 Changed 18 months ago by pjasiun

  • Resolution set to fixed
  • Status changed from review to closed

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 – 2016 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy