Ticket #952 (closed Defect: fixed)

Opened 9 years ago

Last modified 8 years ago

dm4-images: adapt ckeditor image upload and browsing extension to new image plugin

Reported by: Malte Owned by:
Priority: Major Milestone: Release 4.8
Component: DeepaMehta Standard Distribution Version: 4.7
Keywords: Cc: jri
Complexity: 3 Area: GUI / Usability
Module:

Description

Before 4.7 the images plugin, as far as i know, extended the "ckeditor_image" plugin to provide a custom upload and image browsing dialog.

This is somehow broken as of 4.8 but without any visible error messages.

After some searching i could track down that there actually has been a change in dependencies from a CKEditor Plugin named "CKEditor Image - Enhanced Image Plugin" [1] to the one named "CKEditor Image2 - Enhanced Image plugin with HTML5 caption support" [2].

See https://trac.deepamehta.de/ticket/700#comment:2

I hope we can solve this through a different way of configuring (hooking in) the "image browsing" and "image upload" facilities/features (through our plugin.js in of the "dm4-images" module). We'll see.

[1] https://www.drupal.org/project/ckeditor_image
[2] https://www.drupal.org/project/ckeditor_image2

Change History

comment:1 Changed 9 years ago by jri

Yes, in DM 4.8 the CKEditor "Image" plugin is replaced by "Enhanced Image".

http://ckeditor.com/addon/image
http://ckeditor.com/addon/image2

Changing your config (plugin.js) should solve the issue; note Image2 here:

CKEDITOR.config.filebrowserImage2BrowseUrl = '/de.deepamehta.images/browse.html'
CKEDITOR.config.filebrowserImage2UploadUrl = '/images/upload'

See:
http://docs.ckeditor.com/#!/guide/dev_file_manager_configuration-section-adding-file-manager-scripts-for-selected-dialog-windows

Thank you for adapting the dm4-images plugin!

comment:2 Changed 9 years ago by Malte

I now found out that the "image2" actually copies / wraps these configuration options, so there is no need to adapt the configuration option name.

    CKEDITOR.config.filebrowserImageBrowseUrl = '/de.deepamehta.images/browse.html'
    CKEDITOR.config.filebrowserImageUploadUrl = '/images/upload'

See:
https://github.com/ckeditor/ckeditor-dev/blob/12f0de314fd6fbee0bc4d35d541123d283fdecc9/plugins/image2/plugin.js#L81

But instead the newly "image2" plugin has three unmet dependencies:

  1. The complete list of "image2"-Dependencies, as sugested by the resp. "Download" dialogs at the corresponding module pages (see bottom line of download dialog):
  1. Additinoally Upload tab seems to depend on mere the existence of the "filebrowser" plugin but that is not stated in the dependencies on the "download dialog" (as this is not strictly required so to speak):

And i found out that this plugin is not included in the "Basic" but the "Standard" plugin set of CKEditor. See URL for customizing a CKEDitor build at http://ckeditor.com/builder

I found out about this dependency after readin these two pages:
http://docs.ckeditor.com/#!/guide/dev_captionedimage
http://docs.ckeditor.com/#!/guide/dev_file_browse_upload

On the latter is stated that: "CKEditor can be easily integrated with an external file manager (file browser/uploader) thanks to the File Browser plugin which by default is included in every preset." which led me to investigate the presets and observe that "File Browser" was missing on the "Basic" preset...

And finally,. the filebrowser plugin depends on the "popup" plugin.
http://ckeditor.com/addon/popup


So the complete fix for the standard webclient ckeditor configuration must be (and works for me):

Adding the following to "ckeditor/config.js" in the dm4-webclient module:

config.extraPlugins = 'lineutils,widget,popup,filebrowser';

And adding the following bundles into the "ckeditor/plugins" directory:

http://ckeditor.com/addon/lineutils
http://ckeditor.com/addon/widget (existent but incomplete in dm4-webclient)
http://ckeditor.com/addon/filebrowser
http://ckeditor.com/addon/popup

comment:3 Changed 9 years ago by Malte

The docs on CKeditor.com on all dependencies per plugin is not very nice.

After everything until now i can at least confirm that the "File Browser" addon was also a dependency when we successfully extended the old (formerly used) "image" plugin of the CKEditor. Visiting the add-ons website does not state so.

Having setup a successful image upload and browse dialog with the new CKEditor and both images plugins now - i have difficulties to understand this completely new exception (triggered by an upload request tested with both types image modules, "image" and "image2").

19.04.2016 17:40:29 de.deepamehta.files.provider.UploadedFileProvider parseMultiPart
INFO: ### Parsing multipart/form-data request (2 parts)
19.04.2016 17:40:29 de.deepamehta.files.provider.UploadedFileProvider parseMultiPart
INFO: ### field "upload" => file "CgEFfl4UUAAl6Qb.jpg:large.jpeg" (image/jpeg, 126734 bytes)
19.04.2016 17:40:29 de.deepamehta.files.provider.UploadedFileProvider parseMultiPart
INFO: ### field "ckCsrfToken" => "r1rMJrTebz9qbY5GrUNDpzy55IFAVRcyN39VSdrf"
19.04.2016 17:40:29 de.deepamehta.core.util.UniversalExceptionMapper logException
SCHWERWIEGEND: Request "POST /images/upload?CKEditor=editor1&CKEditorFuncNum=0&langCode=en" failed. Responding with 500 (Internal Server Error). The original exception/error is:
java.lang.RuntimeException: Creating UploadedFile from message body failed
	at de.deepamehta.files.provider.UploadedFileProvider.readFrom(UploadedFileProvider.java:62)
	at de.deepamehta.files.provider.UploadedFileProvider.readFrom(UploadedFileProvider.java:35)
	at com.sun.jersey.spi.container.ContainerRequest.getEntity(ContainerRequest.java:488)
	at com.sun.jersey.server.impl.model.method.dispatch.EntityParamDispatchProvider$EntityInjectable.getValue(EntityParamDispatchProvider.java:123)
	at com.sun.jersey.server.impl.inject.InjectableValuesProvider.getInjectableValues(InjectableValuesProvider.java:46)
	at com.sun.jersey.server.impl.model.method.dispatch.AbstractResourceMethodDispatchProvider$EntityParamInInvoker.getParams(AbstractResourceMethodDispatchProvider.java:153)
	at com.sun.jersey.server.impl.model.method.dispatch.AbstractResourceMethodDispatchProvider$TypeOutInvoker._dispatch(AbstractResourceMethodDispatchProvider.java:183)
	at com.sun.jersey.server.impl.model.method.dispatch.ResourceJavaMethodDispatcher.dispatch(ResourceJavaMethodDispatcher.java:75)
	at com.sun.jersey.server.impl.uri.rules.HttpMethodRule.accept(HttpMethodRule.java:302)
	at com.sun.jersey.server.impl.uri.rules.RightHandPathRule.accept(RightHandPathRule.java:147)
	at com.sun.jersey.server.impl.uri.rules.ResourceObjectRule.accept(ResourceObjectRule.java:100)
	at com.sun.jersey.server.impl.uri.rules.RightHandPathRule.accept(RightHandPathRule.java:147)
	at com.sun.jersey.server.impl.uri.rules.RootResourceClassesRule.accept(RootResourceClassesRule.java:84)
	at com.sun.jersey.server.impl.application.WebApplicationImpl._handleRequest(WebApplicationImpl.java:1480)
	at com.sun.jersey.server.impl.application.WebApplicationImpl._handleRequest(WebApplicationImpl.java:1411)
	at com.sun.jersey.server.impl.application.WebApplicationImpl.handleRequest(WebApplicationImpl.java:1360)
	at com.sun.jersey.server.impl.application.WebApplicationImpl.handleRequest(WebApplicationImpl.java:1350)
	at com.sun.jersey.spi.container.servlet.WebComponent.service(WebComponent.java:416)
	at com.sun.jersey.spi.container.servlet.ServletContainer.service(ServletContainer.java:538)
	at com.sun.jersey.spi.container.servlet.ServletContainer.service(ServletContainer.java:716)
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:722)
	at org.apache.felix.http.base.internal.handler.ServletHandler.doHandle(ServletHandler.java:339)
	at org.apache.felix.http.base.internal.handler.ServletHandler.handle(ServletHandler.java:300)
	at org.apache.felix.http.base.internal.dispatch.ServletPipeline.handle(ServletPipeline.java:93)
	at org.apache.felix.http.base.internal.dispatch.InvocationFilterChain.doFilter(InvocationFilterChain.java:50)
	at org.apache.felix.http.base.internal.dispatch.HttpFilterChain.doFilter(HttpFilterChain.java:31)
	at org.apache.felix.http.base.internal.dispatch.FilterPipeline.dispatch(FilterPipeline.java:76)
	at org.apache.felix.http.base.internal.dispatch.Dispatcher.dispatch(Dispatcher.java:49)
	at org.apache.felix.http.base.internal.DispatcherServlet.service(DispatcherServlet.java:67)
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:722)
	at org.eclipse.jetty.servlet.ServletHolder.handle(ServletHolder.java:684)
	at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:501)
	at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:229)
	at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1086)
	at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:428)
	at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:193)
	at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:1020)
	at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:135)
	at org.eclipse.jetty.server.handler.ContextHandlerCollection.handle(ContextHandlerCollection.java:255)
	at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:116)
	at org.eclipse.jetty.server.Server.handle(Server.java:370)
	at org.eclipse.jetty.server.AbstractHttpConnection.handleRequest(AbstractHttpConnection.java:494)
	at org.eclipse.jetty.server.AbstractHttpConnection.content(AbstractHttpConnection.java:982)
	at org.eclipse.jetty.server.AbstractHttpConnection$RequestHandler.content(AbstractHttpConnection.java:1043)
	at org.eclipse.jetty.http.HttpParser.parseNext(HttpParser.java:865)
	at org.eclipse.jetty.http.HttpParser.parseAvailable(HttpParser.java:240)
	at org.eclipse.jetty.server.AsyncHttpConnection.handle(AsyncHttpConnection.java:82)
	at org.eclipse.jetty.io.nio.SelectChannelEndPoint.handle(SelectChannelEndPoint.java:667)
	at org.eclipse.jetty.io.nio.SelectChannelEndPoint$1.run(SelectChannelEndPoint.java:52)
	at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:608)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$3.run(QueuedThreadPool.java:543)
	at java.lang.Thread.run(Thread.java:701)
Caused by: java.lang.RuntimeException: Parsing multipart/form-data request failed
	at de.deepamehta.files.provider.UploadedFileProvider.parseMultiPart(UploadedFileProvider.java:110)
	at de.deepamehta.files.provider.UploadedFileProvider.readFrom(UploadedFileProvider.java:60)
	... 51 more
Caused by: java.lang.RuntimeException: "ckCsrfToken" is an unexpected field (value="r1rMJrTebz9qbY5GrUNDpzy55IFAVRcyN39VSdrf")
	at de.deepamehta.files.provider.UploadedFileProvider.parseMultiPart(UploadedFileProvider.java:95)
	... 52 more
19.04.2016 17:40:29 de.deepamehta.core.impl.TransactionFactory$TransactionResourceFilter$2 filter
WARNUNG: ### Rollback transaction of de.deepamehta.plugins.images.ImagePlugin#upload()

Any ideas how this could happen? Is this new field part of a changed HTML Form used in CKEditors upload dialog or what? Thankful for any hints.

comment:4 Changed 9 years ago by jri

Yes, indeed, the File Browser plugin is not included in DM 4.8 as 4.8 uses CKEditor's Basic preset while former DM versions used Standard preset.

FYI: DM's CKEditor is always build with http://ckeditor.com/builder, so every hard dependency plugins are included automatically. Thus Widget and Line Utils are included DM 4.8. Builder is also useful for investigating all the (transitive) dependencies. DM's Builder configuration is documented here:
https://github.com/jri/deepamehta/blob/master/modules/dm4-webclient/src/main/resources/web/script/vendor/ckeditor/ckeditor-deepamehta-customization.txt

I will include the File Browser plugin in 4.8.1

Thank you for finding out!

Regarding the server exception I still have to investigate it.

comment:5 Changed 9 years ago by Jörg Richter <jri@…>

In 1a5910397e390192e5acc36b2f3ebf02646702d1/deepamehta:

CKEditor: include File Browser plugin (#952).

This is required for the DM4 Images plugin to work with DM 4.8.

See #952.

comment:6 Changed 9 years ago by Jörg Richter <jri@…>

In a8c322d8d4e2250dc5feaf52be30ccacab0c003e/deepamehta:

Files module: fix image upload (#952).

Uploading an image file via DM4 Images plugin works in DM 4.8.

This exception does not occur anymore:

java.lang.RuntimeException: "ckCsrfToken" is an unexpected field (value="r1rMJrTebz9qbY5GrUNDpzy55IFAVRcyN39VSdrf")

See #952.

comment:7 Changed 9 years ago by Jörg Richter <jri@…>

In cf7da3dd8d7cd28d8e393e00ff9f3a83c7fdce30/deepamehta:

Webclient: inline CKEditor config (#952).

ckeditor/config.js is dropped.

Updating CKEditor becomes more easy this way.

See Webclient's html_renderer.js.

ckeditor/contents.css and ckeditor/styles.js are dropped as well. They were not in use anyway.

See #952.

comment:8 Changed 9 years ago by Malte

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

The images plugin is now apdated to the changes introduced with DeepaMehta 4.8 and 4.8.1. In particular browsing images does now work, too.

Adapted the image plugin to a BREAKING CHANGE in the FileService? as its behaviour is now one that affords mannual setup of workspace folders in a filerepository (if per-workspace=true) (see #815 for more).

comment:9 Changed 8 years ago by jri

No, there is no breaking change. The concept now and then is that the developer must not care about creating the workspace folders. This is supposed to happen automatically. But there was a bug which is fixed meanwhile. See ticket:965#comment:6 and following.

comment:10 Changed 8 years ago by Malte

Yes, API wise it is probably not correct to call this a breaking change.

After #965 i removed my manual fix (setting up workspaces folders manually) again and now the plugin integrates all the new features of the FilesPlugin? completely (incl. pathPrefix, and fileExists #884).

I am just glad that we got the dm4-images upload working again and relying on the image2 plugin seems to have been an improvement too. Thanks for your support!

comment:11 Changed 8 years ago by jri

  • Status changed from closed to reopened
  • Resolution fixed deleted

Uploading and displaying an image doesn't work properly if per-workspace repos are enabled.
The image is not displayed as the <img> tag's URL causes a 500 server error:

java.lang.RuntimeException: No workspace recognized in repository path "workspace-4035/images/ffn_banner_allg_234x60.gif"

The path obtained from the request URL misses a leading slash.
The wrong URL is:

http://localhost:8080/filerepo/workspace-4035/images/ffn_banner_allg_234x60.gif

With a double slash after /filerepo it would work in most cases, but CAUTION: it will not work if a file or folder name contains e.g. a + or ? character.

http://localhost:8080/filerepo//workspace-4035/images/ffn_banner_allg_234x60.gif

SOLUTION: since DM 4.7 the entire file repo path (of a /files or /filerepo request) must be encoded as a single URI component, that is the slashes (and other special characters like plus +) must be URL-encoded (see #819).

A correct request looks like this:

http://localhost:8080/filerepo/%2Fworkspace-4035%2Fimages%2Fffn_banner_allg_234x60.gif

When you construct a files/filerepo request at client-side you can use Javascript's encodeURIComponent() function:

"/filerepo/" + encodeURIComponent(repoPath)

Note: in a files/filerepo request you actually pass an URI (the repo path) as part (=component) of the request URI. That's why you must use encodeURIComponent, which encodes the characters which are special in URLs, like /, +, ?, & as well. (encodeURI() on the other hand would not).

Otherwise congratulations for the great work at the myDM installation!

comment:12 Changed 8 years ago by jri

Please don't put your private paths in public poms.

<properties>
    <dm4.deploy.dir>/home/malted/Downloads/deepamehta-4.8.1/bundle-deploy</dm4.deploy.dir>
</properties>

Other users will not be able to build your plugin.

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-resources-plugin:2.7:copy-resources (hot-deploy) on project dm48-images: Cannot create resource output directory: /home/malted/Downloads/deepamehta-4.8.1/bundle-deploy

Consider cherry-picking before committing.

comment:13 Changed 8 years ago by jri

Thanks for the URL encoding fix!
https://github.com/mukil/dm4-images/commit/7e2b58feed

It works fine now.

comment:14 follow-up: ↓ 15 Changed 8 years ago by Malte

Thanks for your comment, you provided the hint for the fix and i missed this last check/test before the 0.9.8 release. The new SNAPSHOT now contains a fix and i will create a follow up release of the dm4-images module, version 0.9.9, i guess.

In general i always uses encodeURIComponent() when i am working with javascript but didnt know about its difference in relation to encodeURI. Thanks for your explanation

Consider cherry-picking before committing.

I always do git commit via "cherry picking" (if i understand you correctly) but this time it wouldn't have helped me as the affected file (pom.xml) also contained a change to be commited (change in Version Nr.) and commenting the dm4.deploy.dir prop would have been the only thing preventing your local development build to fail, i guess.

Otherwise congratulations for the great work at the myDM installation!

You're welcome.

comment:15 in reply to: ↑ 14 Changed 8 years ago by jri

Replying to Malte:

I always do git commit via "cherry picking" (if i understand you correctly) but this time it wouldn't have helped me as the affected file (pom.xml) also contained a change to be commited (change in Version Nr.) and commenting the dm4.deploy.dir prop would have been the only thing preventing your local development build to fail, i guess.

Yes, sure, but I expect the build to run successful without fixing the pom before.

With cherry-picking I meant git's "Interactive mode". You can commit single file modifications instead of all or nothing. See "git add -p" resp. "Interactive mode".

comment:16 Changed 8 years ago by Malte

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

Yes, sure, but I expect the build to run successful without fixing the pom before.

Of course, jenkins and me would expect so too.

And thanks a lot for your explanation, i just learned about the interactive mode of git and could use.
The new dm4-images plugin is now released and i will move it onto the download server asap.

comment:17 Changed 8 years ago by jri

  • Status changed from closed to reopened
  • Resolution fixed deleted

The file name encoding is still not correct. Displaying an uploaded image doesn't work if the file name contains spaces.

To fix it you can use Core Util's encodeURIComponent() instead of Java's URLEncoder.encode()

import de.deepamehta.core.util.JavaUtils;

JavaUtils.encodeURIComponent(path)

This method relies on URLEncoder.encode() but handles spaces properly.
Furthermore it catches the UnsupportedEncodingException so you don't need to.

comment:18 Changed 8 years ago by Malte

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

Thanks for your comment. i did not really pay attention when choosing URLEncoder.encode() to url encode the filenames, i did not check before using it and i did not test it. Checking or testing it might have prevented this issue from popping up again... i am sorry.

Here is the new build relying on your JavaUtils to also properly URLEncode space characters in filenames: https://download.deepamehta.de/nightly/dm48-images-0.9.10.jar

Note: See TracTickets for help on using tickets.