From fabe02dc7bb2d647fd1f4ae3ff9cd5b460ef5e21 Mon Sep 17 00:00:00 2001 From: Tomasz Jakut Date: Mon, 26 Mar 2018 11:09:46 +0200 Subject: [PATCH 01/17] Add changelog entry. --- CHANGES.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index 18ab45073b6..e7f0055019a 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -3,6 +3,10 @@ ## CKEditor 4.9.1 +Fixed Issues: + +* [#1835](https://github.com/ckeditor/ckeditor-dev/issues/1835): Fixed: Integration between [CKFinder](https://ckeditor.com/ckeditor-4/ckfinder/) and [File Browser](https://ckeditor.com/cke4/addon/filebrowser) plugin does not work. + ## CKEditor 4.9 New Features: From 0846d2f9cdf50c10caac319b3a03d67384ffc6a3 Mon Sep 17 00:00:00 2001 From: Tomasz Jakut Date: Mon, 26 Mar 2018 13:15:21 +0200 Subject: [PATCH 02/17] Add unit tests. --- tests/plugins/filebrowser/uploadbutton.js | 122 ++++++++++++++++++++++ 1 file changed, 122 insertions(+) diff --git a/tests/plugins/filebrowser/uploadbutton.js b/tests/plugins/filebrowser/uploadbutton.js index de6fbe00262..04ea45cbb8c 100644 --- a/tests/plugins/filebrowser/uploadbutton.js +++ b/tests/plugins/filebrowser/uploadbutton.js @@ -44,6 +44,26 @@ language: 'en' } }, + + xhrCkf: { + config: { + filebrowserUploadUrl: 'upload/?command=QuickUpload' + } + }, + + xhrCkf2: { + config: { + filebrowserUploadUrl: 'upload/?command=QuickUpload&responseType=json' + } + }, + + submitCkf: { + config: { + filebrowserUploadUrl: 'upload/?command=QuickUpload', + filebrowserUploadMethod: 'form' + } + }, + submit: { config: { filebrowserUploadUrl: 'foo', @@ -126,6 +146,93 @@ } ); }, + // (#1835) + 'test for XHR request to CKFinder (no responseType parameter)': function() { + if ( !CKEDITOR.fileTools.isFileUploadSupported ) { + assert.ignore(); + } + + var editor = this.editors.xhrCkf, + bot = this.editorBots.xhrCkf; + + editor.addCommand( 'testDialog', new CKEDITOR.dialogCommand( 'testDialog' ) ); + bot.dialog( 'testDialog', function( dialog ) { + var sendButton = dialog.getContentElement( 'Upload', 'uploadButton' ), + inputStub = mockInput( dialog ); + + // Execute just after XHR request is generated; + editor.once( 'fileUploadRequest', function() { + resume( function() { + assert.isNotNull( this.requests[ 0 ].url.match( /&responseType=json$/ ), + 'responseType parameter' ); + dialog.hide(); + inputStub.restore(); + } ); + }, null, null, 1000 ); + + sendButton.click(); + wait(); + } ); + }, + + // (#1835) + 'test for XHR request to CKFinder (responseType parameter present)': function() { + if ( !CKEDITOR.fileTools.isFileUploadSupported ) { + assert.ignore(); + } + + var editor = this.editors.xhrCkf2, + bot = this.editorBots.xhrCkf2; + + editor.addCommand( 'testDialog', new CKEDITOR.dialogCommand( 'testDialog' ) ); + bot.dialog( 'testDialog', function( dialog ) { + var sendButton = dialog.getContentElement( 'Upload', 'uploadButton' ), + inputStub = mockInput( dialog ); + + // Execute just after XHR request is generated; + editor.once( 'fileUploadRequest', function() { + resume( function() { + assert.areSame( 1, this.requests[ 0 ].url.match( /responseType=json/g ).length, + 'responseType parameter count' ); + dialog.hide(); + inputStub.restore(); + } ); + }, null, null, 1000 ); + + sendButton.click(); + wait(); + } ); + }, + + // (#1835) + 'test for XHR request not to CKFinder (responseType parameter)': function() { + if ( !CKEDITOR.fileTools.isFileUploadSupported ) { + assert.ignore(); + } + + var editor = this.editors.xhr, + bot = this.editorBots.xhr; + + editor.addCommand( 'testDialog', new CKEDITOR.dialogCommand( 'testDialog' ) ); + bot.dialog( 'testDialog', function( dialog ) { + var sendButton = dialog.getContentElement( 'Upload', 'uploadButton' ), + inputStub = mockInput( dialog ); + + // Execute just after XHR request is generated; + editor.once( 'fileUploadRequest', function() { + resume( function() { + assert.isNull( this.requests[ 0 ].url.match( /&responseType=json$/ ), + 'responseType parameter' ); + dialog.hide(); + inputStub.restore(); + } ); + }, null, null, 1000 ); + + sendButton.click(); + wait(); + } ); + }, + 'test for submit form': function() { var editor = this.editors.submit, bot = this.editorBots.submit; @@ -143,6 +250,21 @@ } ); }, + // (#1835) + 'test form action with CKFinder': function() { + var editor = this.editors.submitCkf, + bot = this.editorBots.submitCkf; + + editor.addCommand( 'testDialog', new CKEDITOR.dialogCommand( 'testDialog' ) ); + bot.dialog( 'testDialog', function( dialog ) { + var input = dialog.getContentElement( 'Upload', 'upload' ); + + assert.isNull( input.action.match( /responseType=json/ ), + 'responseType parameter' ); + dialog.hide(); + } ); + }, + 'test for xhr loader error': function() { if ( !CKEDITOR.fileTools.isFileUploadSupported ) { assert.ignore(); From ba339d06aa43224f9c6290d576db3e6845a13b69 Mon Sep 17 00:00:00 2001 From: Tomasz Jakut Date: Mon, 26 Mar 2018 13:24:00 +0200 Subject: [PATCH 03/17] Add function to add missing parameters. --- plugins/filebrowser/plugin.js | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/plugins/filebrowser/plugin.js b/plugins/filebrowser/plugin.js index 0a630538d70..36d0d9ce088 100644 --- a/plugins/filebrowser/plugin.js +++ b/plugins/filebrowser/plugin.js @@ -137,6 +137,18 @@ return url + ( ( url.indexOf( '?' ) != -1 ) ? '&' : '?' ) + queryString.join( '&' ); } + // Adds missing required parameters to CKFinder's url (#1835). + // + // @since 4.9.1 + // @param {String} url CKFinder's url. + function addMissingParams( url ) { + if ( !url.match( /command=QuickUpload/ ) ) { + return url; + } + + return addQueryString( url, { responseType: 'json' } ); + } + // Make a string's first character uppercase. // // @param {String} @@ -328,7 +340,7 @@ loader.on( 'error', xhrUploadErrorHandler.bind( this ) ); loader.on( 'abort', xhrUploadErrorHandler.bind( this ) ); - loader.loadAndUpload( url ); + loader.loadAndUpload( addMissingParams( url ) ); return 'xhr'; } From cc2343414689ade1215e4421ede384de56c1c7e7 Mon Sep 17 00:00:00 2001 From: Tomasz Jakut Date: Mon, 26 Mar 2018 13:29:02 +0200 Subject: [PATCH 04/17] Ensure that parameter is added only if needed. --- plugins/filebrowser/plugin.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/filebrowser/plugin.js b/plugins/filebrowser/plugin.js index 36d0d9ce088..1a6fea11a13 100644 --- a/plugins/filebrowser/plugin.js +++ b/plugins/filebrowser/plugin.js @@ -142,7 +142,7 @@ // @since 4.9.1 // @param {String} url CKFinder's url. function addMissingParams( url ) { - if ( !url.match( /command=QuickUpload/ ) ) { + if ( !url.match( /command=QuickUpload/ ) || url.match( /(\?|&)responseType=json/ ) ) { return url; } From 3ec17c2a9aed8c0fcfe2e499568dda6ee7eff693 Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Mon, 26 Mar 2018 12:20:21 +0200 Subject: [PATCH 05/17] Added tests for CKFinder integration issue. --- .../filebrowser/manual/uploadckfinder.html | 19 +++++++++++++++ .../filebrowser/manual/uploadckfinder.md | 23 +++++++++++++++++++ 2 files changed, 42 insertions(+) create mode 100644 tests/plugins/filebrowser/manual/uploadckfinder.html create mode 100644 tests/plugins/filebrowser/manual/uploadckfinder.md diff --git a/tests/plugins/filebrowser/manual/uploadckfinder.html b/tests/plugins/filebrowser/manual/uploadckfinder.html new file mode 100644 index 00000000000..51a2832c105 --- /dev/null +++ b/tests/plugins/filebrowser/manual/uploadckfinder.html @@ -0,0 +1,19 @@ +

Classic editor

+ + + +

Inline editor

+ +
+

Click me

+
+ + diff --git a/tests/plugins/filebrowser/manual/uploadckfinder.md b/tests/plugins/filebrowser/manual/uploadckfinder.md new file mode 100644 index 00000000000..ae3a5573169 --- /dev/null +++ b/tests/plugins/filebrowser/manual/uploadckfinder.md @@ -0,0 +1,23 @@ +@bender-tags: 4.9.1, bug, 1835 +@bender-ui: collapsed +@bender-ckeditor-plugins: wysiwygarea, toolbar, filebrowser, filetools, image, link, flash, floatingspace + +# CKFinder upload + +1. Open Image dialog using the Image button. +2. Go to the Upload tab. +3. Choose an image to be uploaded. +4. Click the "Send it to the Server" button. + +## Expected + +No errors are displayed. + +CKFinder _might_ report an alert like: "A file with the same name already exists. The uploaded file was renamed to "city-wallpaper-47(8).jpg"." - that's fine it only means that the file was renamed. + +## Unexpected + +There should be no errors, e.g. like (but not limited to): + +* "Network error occurred during file upload." +* "Incorrect server response." \ No newline at end of file From 4b1bc8147e0fede77b359efc8d82382324fb5c2f Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Mon, 26 Mar 2018 13:43:00 +0200 Subject: [PATCH 06/17] Tests: removed forced JSON responseType. --- tests/plugins/filebrowser/manual/uploadckfinder.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/plugins/filebrowser/manual/uploadckfinder.html b/tests/plugins/filebrowser/manual/uploadckfinder.html index 51a2832c105..ad9cbd21f79 100644 --- a/tests/plugins/filebrowser/manual/uploadckfinder.html +++ b/tests/plugins/filebrowser/manual/uploadckfinder.html @@ -11,7 +11,7 @@

Inline editor

From 0d11d0105dc36fb3696daef27d6d8b739bf7c917 Mon Sep 17 00:00:00 2001 From: Tomasz Jakut Date: Mon, 26 Mar 2018 16:35:36 +0200 Subject: [PATCH 15/17] Update ignored flag after ignoring test. --- tests/plugins/filebrowser/manual/uploadckfinder.html | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/plugins/filebrowser/manual/uploadckfinder.html b/tests/plugins/filebrowser/manual/uploadckfinder.html index 90e674b5173..5ccc6e6b3bf 100644 --- a/tests/plugins/filebrowser/manual/uploadckfinder.html +++ b/tests/plugins/filebrowser/manual/uploadckfinder.html @@ -17,6 +17,7 @@

Inline editor

instanceReady: function() { if ( !CKEDITOR.fileTools.isFileUploadSupported && !ignored ) { bender.ignore(); + ignored = true; } } } From 0323caa108a31a6d8178461a8d7ef6adc4e6f584 Mon Sep 17 00:00:00 2001 From: Tomasz Jakut Date: Mon, 26 Mar 2018 17:15:56 +0200 Subject: [PATCH 16/17] Fetch error message directly from XHR object. --- plugins/filebrowser/plugin.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/plugins/filebrowser/plugin.js b/plugins/filebrowser/plugin.js index 287247145ae..95c86a211e7 100644 --- a/plugins/filebrowser/plugin.js +++ b/plugins/filebrowser/plugin.js @@ -358,9 +358,15 @@ } function xhrUploadErrorHandler( evt ) { + var response = {}; + + try { + response = JSON.parse( evt.sender.xhr.response ); + } catch ( e ) {} + // `this` is a reference to ui.dialog.fileButton. this.enable(); - alert( evt.sender.message ); // jshint ignore:line + alert( response.error ? response.error.message : evt.sender.message ); // jshint ignore:line } // Updates the target element with the url of uploaded/selected file. From 1ae994a22bcf2c090927a17276c6f6fdd34dc7e0 Mon Sep 17 00:00:00 2001 From: Tomasz Jakut Date: Mon, 26 Mar 2018 17:32:09 +0200 Subject: [PATCH 17/17] Secure before null in XHR response. --- plugins/filebrowser/plugin.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/filebrowser/plugin.js b/plugins/filebrowser/plugin.js index 95c86a211e7..fe5899d3685 100644 --- a/plugins/filebrowser/plugin.js +++ b/plugins/filebrowser/plugin.js @@ -361,7 +361,7 @@ var response = {}; try { - response = JSON.parse( evt.sender.xhr.response ); + response = JSON.parse( evt.sender.xhr.response ) || {}; } catch ( e ) {} // `this` is a reference to ui.dialog.fileButton.