Opened 4 years ago

Closed 4 years ago

#12952 closed New Feature (fixed)

[FileLoader] Customization of the communication with the server should be simpler

Reported by: Artur Delura Owned by: Piotr Jasiun
Priority: Normal Milestone: CKEditor 4.5.0 Beta
Component: General Version:
Keywords: Cc:

Description (last modified by Artur Delura)

Working on ticket:12612 we found out that we need to allow user to add flag withCredentials and some extra headers to XHR object in filetools plugins. Use case for this is to allow cross domain requests.

Following this example I tried to customize a bit my request object. I end up with:

CKEDITOR.replace( 'editor', function() {
  CKEDITOR.fileTools.fileLoader.prototype.sendRequest = function() {
	  var formData = new FormData(),
			xhr = this.xhr;

		formData.append( 'upload', this.file, this.fileName );
		xhr.withCredentials = true; // Extra lines
		xhr.setRequestHeader( 'X-PINGOTHER', 'pingpong' ); // Extra lines
		xhr.open( 'POST', this.uploadUrl, true );
		xhr.send( formData );
	};
} );

Shortcomings of this solution are:

  • We overwrite global function sendRequest which influence all editor instances while optionimageUploadUrl is unique for all editor isntances.
  • We have to wait for plugin to be loaded and editor instance as well to overwrite this method. If user have got a few editors he have to impelement special mechanism to overwrite this function when first editor instance is loaded. Or maybe there is a global event which fire when plugin is loaded?

We got at least two sulutions for this:

  • We can add extra config option like:
    xhrData =  {
        withCredentials: true,
        headers: {
            'X-HEADER-NAME': "X-HEADER VALUE"
        }
    };
    

And function send request will take into consideration xhrData config options and set flag and headers. This is simples solution but don't allow user whole freedom.

  • Second option is to fire events like prerequest which will have an xhr object as a property and user would make whatever he wants with this.

Change History (12)

comment:1 Changed 4 years ago by Artur Delura

Description: modified (diff)

comment:2 Changed 4 years ago by Piotr Jasiun

First of all we should talk general, about the pair of the methods: sendRequest and handleResponse. Most probably they will be redefined together to adapt the request to the server needs and handle the response. Note the by default data are send as a form with hardcoded field name and response needs to be a JSON with defined structure, so user may want to change it.

At the first approach we decided that we will not use events, but user will overwrite these methods, because:

  1. this is the simplest solution (KISS),
  2. he will do it only once (do not need attach multiple listeners),
  3. we would want to overwrite whole method, not only a part of it.

Now we know that:

  1. this is not that simple, the method is not available until the filetools plugin is loaded;
  2. I can imaging that one part of users code or some plugin may want to add some headers (like in this case, add withCredentials) and another part want to define format of the data;
  3. adding withCredentials property is an example where we do not want to overwrite whole method, but only add one header.

I think that both sendRequest and handleResponse (they both should work the same way, otherwise our API will be inconsistent) should fire events on CKEDITOR object with the loader as a parameter.

CKEDITOR is better then editor instance because:

  • it is possible that some request will be send when the editor is creating,
  • I believe all instances will communicate with the server the same way,
  • it is simpler to add listener to CKEDITOR instead of every instance.

The only problem is that user should not change read-only loaders properties in handle response event, so they should be set as a event parameter and overwrite when the event is done (the same way evt.data.dataValue works).

comment:3 Changed 4 years ago by Piotr Jasiun

Milestone: CKEditor 4.5.0
Summary: Modifying XHR object in filetools plugin is painfull.[FileLoader] Customization of the communication with the server should be simpler

comment:4 Changed 4 years ago by Piotr Jasiun

Type: BugNew Feature

comment:5 Changed 4 years ago by Jakub Ś

Status: newconfirmed

comment:6 Changed 4 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

comment:7 Changed 4 years ago by Artur Delura

Status: assignedreview

I removed both sendRequest and handleResponse private method and added fileUploadRequest and fileUploadResponse events with appropriate documentation and examples. Tests has been updated as well. Changes in branch:t/12952.

comment:8 Changed 4 years ago by Piotr Jasiun

Owner: changed from Artur Delura to Piotr Jasiun
Status: reviewassigned

This ticket needs some API improvements.

comment:9 Changed 4 years ago by Piotr Jasiun

Status: assignedreview

Changes done and waiting for the review. Changes in t/12952.

comment:10 Changed 4 years ago by Piotrek Koszuliński

Status: reviewreview_failed

I pushed rebased branch:t/12952 plus one additional commit. We decided to move events to the editor instance, though, so R- for now.

comment:11 Changed 4 years ago by Piotr Jasiun

Status: review_failedreview

Events moved to editor.

comment:12 Changed 4 years ago by Piotrek Koszuliński

Resolution: fixed
Status: reviewclosed

Merged to major with git:a5d0bf2.

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy