Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CKFinder Quickupload integration got broken due to latest changes in CKEditor 4.9.0 #1835

Closed
jswiderski opened this issue Mar 23, 2018 · 8 comments
Labels
plugin:filebrowser The plugin which probably causes the issue. regression This issue is a regression. status:confirmed An issue confirmed by the development team. target:minor Any docs related issue that can be merged into a master or major branch. type:bug A bug.
Milestone

Comments

@jswiderski
Copy link
Contributor

Are you reporting a feature request or a bug?

Bug

Provide detailed reproduction steps (if any)

  1. Download CKFinder 3.4.2 for PHP or ASP.NET and install it on a server
  2. Open ckfinder/samples/ckeditor.html and change editor version from 4.5.10 to 4.9.0
  3. Upload a file through upload tab

Expected result

The request should look more or less like below and upload should finish with success:
http://test.dev/ckfinder-aspnet-342/connector?command=QuickUpload&type=Files&CKEditor=editor1&CKEditorFuncNum=0&langCode=pl

Actual result

The request looks like below and CKFinder returns 109 Invalid Request error what results in HTTP 400 being returned:
http://test.dev/ckfinder-aspnet-342/connector?command=QuickUpload&type=Files

Other details

  • Browser: Any
  • OS: Any
  • CKEditor version: 4.9.0
  • Installed CKEditor plugins: filebrowser

CKEditor 4.9.0
cke49

CKEditor 4.8.0
cke48

@jswiderski jswiderski added type:bug A bug. status:confirmed An issue confirmed by the development team. labels Mar 23, 2018
@AnnaTomanek
Copy link
Contributor

AnnaTomanek commented Mar 23, 2018

Can confirm, something's wrong, can be reproduced on the SDK samples as well. When trying to upload files through the Upload tab, an error is returned.
screen shot 2018-03-23 at 15 01 30
The console:
screen shot 2018-03-23 at 15 03 39

Drag&drop upload seems to be working OK and it's also possible to add images previously uploaded to CKFinder.

@wwalc wwalc added the regression This issue is a regression. label Mar 24, 2018
@mlewand mlewand added the target:minor Any docs related issue that can be merged into a master or major branch. label Mar 24, 2018
@Comandeer
Copy link
Member

It seems that the issue is connected with configuration. Setting config to:

config.filebrowserBrowseUrl = '/ckfinder/ckfinder.html';
config.filebrowserUploadUrl = '/ckfinder/core/connector/php/connector.php?command=QuickUpload&type=Files&responseType=json';

or to

config.filebrowserBrowseUrl = '/ckfinder/ckfinder.html';
config.filebrowserUploadMethod = 'form';
config.filebrowserUploadUrl = '/ckfinder/core/connector/php/connector.php?command=QuickUpload&type=Files';

fixes the issue.

What's more, even when CKEditor shows "Incorrect server response", the upload is done. The only issue is with reading the server's answer due to incorrect format (CKFinder responds with JS code by default and XHR uploader is waiting for JSON).

@wwalc
Copy link
Member

wwalc commented Mar 25, 2018

Unfortunately, it looks like the only reasonable (from the end user point of view) solution will be quite ugly:

  1. In CKEditor, if the upload method is xhr check the destination URL for upload (I intentionally did not use the exact configuration option name as there are plenty of ways to set it).
  2. If the URL (query string) contains command=QuickUpload and does not contain responseType=json then append &responseType=json to it.

This way all URLs that point to CKFinder will be correctly adjusted. There is no other reasonable way to fix the issue.

Note: searching for command=QuickUpload is a good enough check to understand if it's a URL to CKFinder uploader. Do not try searching for "connector", "ckfinder" etc. as this part of the URL is configurable and completely virtual e.g. in case of Java. Even if some third party code is using same parameter/value (command=QuickUpload) chances that appending &responseType=json will break something are close to zero.

@Comandeer Comandeer added the plugin:filebrowser The plugin which probably causes the issue. label Mar 26, 2018
@mlewand mlewand added this to the 4.9.1 milestone Mar 26, 2018
@mlewand
Copy link
Contributor

mlewand commented Mar 26, 2018

Fixed by #1840.

@jonnott
Copy link

jonnott commented Apr 6, 2018

Am I right in thinking this only fixes CKFinder v3.x, and not CKFinder v2.x ?
Doesn't seem to make a different to v2.4 or v2.4.2 - same error 'Incorrect server response' occurs.
Guess there's no responseType query string param in v2.x ?

@jonnott
Copy link

jonnott commented Apr 6, 2018

As integration with CKFinder v2.x worked perfectly fine until CKEditor v4.9.x, I'd consider this to be a BC break/regression still, and therefore this merge not a satisfactory resolution, unless there has been a point previously where CKEditor's support for CKFinder v2.x has been officially dropped in a major version release...?

@jonnott
Copy link

jonnott commented Apr 6, 2018

@mlewand motioning to re-open the issue ;)

@mlewand
Copy link
Contributor

mlewand commented Apr 9, 2018

@jonnott it should be tracked in a separate issue, thanks for creating #1869.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin:filebrowser The plugin which probably causes the issue. regression This issue is a regression. status:confirmed An issue confirmed by the development team. target:minor Any docs related issue that can be merged into a master or major branch. type:bug A bug.
Projects
None yet
Development

No branches or pull requests

6 participants