Request should be more flexible

[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');).

[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).

[Tomasz Jakut]

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

[Piotr Jasiun]

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:

• ​https://github.com/cksource/ckeditor-dev/blob/c02ca17a2cf6e33e471e9f9a2c9f35998d8ab0cc/plugins/filetools/plugin.js#L50-L51 this is messy that formData is not evt.data.formData. Maybe we should rename evt.data.formData or call native FormData object $formData? (usually in the CKEditor if there is native and custom object the native one starts with $).
• is there any reason to put this check here if you create this object when you fire the event: ​https://github.com/cksource/ckeditor-dev/blob/c02ca17a2cf6e33e471e9f9a2c9f35998d8ab0cc/plugins/filetools/plugin.js#L53-L54
• we do not used to check it there is nothing added to the object prototype; if one add something the editor will be broken in so make places that this check will not help ​https://github.com/cksource/ckeditor-dev/blob/c02ca17a2cf6e33e471e9f9a2c9f35998d8ab0cc/plugins/filetools/plugin.js#L57-L58. In general note that code size matter so adding too many checks will make the editor unnecessarily big.
• ​https://github.com/cksource/ckeditor-dev/blob/c02ca17a2cf6e33e471e9f9a2c9f35998d8ab0cc/plugins/filetools/plugin.js#L64-L67 put the single lines into brackets; the rule in the JSCS is disabled because we changed it recently (about a year ago) and there are still some remaining single lines without brackets in the code.
• ​https://github.com/cksource/ckeditor-dev/blob/c02ca17a2cf6e33e471e9f9a2c9f35998d8ab0cc/plugins/filetools/plugin.js#L65 it would be great to put filename together with the file into evt.data.formData so all files in the formData would be treat the same way, user could rename them the same way and fileLoader will not be used in this fileUploadRequest listener (so all changes of the upload data can be done using formData). It is the small change of the API, but because it is a major release we can do it.
• ​https://github.com/cksource/ckeditor-dev/blob/c02ca17a2cf6e33e471e9f9a2c9f35998d8ab0cc/tests/plugins/filetools/fileloader.js#L425-L443 this behavior is bad. Errors should never be hidden.

[Tomasz Jakut]

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

[Piotr Jasiun]

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.

[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.

[Tomasz Jakut]

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

Pushed ​branch:t/13518

[Piotr Jasiun]

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.