search

backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 38 forks source link

Plan to upgrade CKEditor to 4.5.x. #1560

Closed ghost closed 8 years ago

ghost commented 8 years ago

I'm developping a project that relies on CKEditor 4.5.x (specifically : the uploadwidget plugin). After a week of intensive testing, I can confirm that the backdrop plugins are fully compatible with CKEditor 4.5.5 (and probably with 4.5.6). Typically, the following sequence : var widget = editor.widgets.initOn(element, "image", {"data-file-id": fid}); creates a perfectly valid backdropimagecaptionwidget. Is there any point to delay upgrading ?

quicksketch commented 8 years ago

Is there any point to delay upgrading ?

Hi @gifad! Great question. I think our policy around minor updates to CKEditor should mirror our own release cycles. That is, a bug fixing release of CKEditor can go into a bug fixing release of Backdrop. We're currently running 4.4.7, so the jump to 4.5.x would be a minor increment and should wait until Backdrop 1.4.0. If there are bug fix releases (e.g. 4.4.8) we could put those into Backdrop 1.3.x.

I'd love to get CKEditor updated as soon as possible to make sure it gets included in the 1.4.0 release. Would you be up making a pull request?

ghost commented 8 years ago

Would you be up making a pull request?

I (and google translate) do not know of this use of "to be up", but yes, I think I could make a PR. But I think a lot of points must be discussed before, that I could not find in this issue queue :

  1. The option to use CKEditor CDN.
  2. If not, use a "native" CKeditor build, or a "backdrop optimized" build ? (currently, it's a "drupal optimized", to use with a working drupallink plugin...)
  3. Make backdroplink work with images, and handle anchors (did you use backdrop to type https://backdropcms.org/roadmap ?) - BTW, I can't see the point of using a specific link plugin...)
  4. Bring back the save plugin, and button.
  5. Use of "div" vs "iframe" mode (ideally coupled with "Use the administration theme when editing or creating content").
  6. Bring back autogrow plugin (for iframe mode), and add somewhere (where ?) the dozen of js lines to manage CKEditor area height (for inline mode)
  7. and last, but not least, include uploadwidget, and provide a backdropuloadimage plugin, based on uploadimage and a ptch in file.module to implement it's required uploadurl.

Any way, I was interested by the challenge, and built a (huge) pull request :

https://github.com/backdrop/backdrop/pull/1226

I made the mistake to issue a unique commit, github cannot show the interesting parts; they are at

core/modules/ckeditor/ckeditor.module core/modules/ckeditor/js/plugins/backdropuploadimage/plugin.js core/modules/file/file.module

You can also try it at my demo site : http://p1347.phpnet.org/backdrop/l/3rgz8UIEd0Vd/ (be sure to create/edit a "Basic Page" : using it with "Article" has unexpected (although interesting) effects)

Graham-72 commented 8 years ago

:+1: I had a quick try on your demo site - very nice. Thank you.

ghost commented 8 years ago

Thanks @Graham-72, I promoted your swan to the front page :smiley:

Graham-72 commented 8 years ago

You are welcome to one of our swans! Does this mean one can find and reuse an image file that has previously been uploaded to the server?

ghost commented 8 years ago

Does this mean one can find and reuse an image file

Yes you can ! but this is far beyond this little PR, which is just a prerequisite to the backdrop-mediakit project I was talking about at #1448: Support fieldable file types and a file management interface

What you have seen at the right of the screen is due to a misconfiguration of my "Basic page" (The media library is appealed by my "Draft" field, which I left configured to embed my media entities (but don't work as a draft for other reasons))

BTW, the tests on my PR failed, just because they found that the list of extraPlugins has changed - no comment

ghost commented 8 years ago

Updated my PR with the following :

  1. Switch to CKEditor CDN.
  2. Fix small points in backdropimage, to ack that external plugins (namely image2) are not loaded during beforeInit:.
  3. Bring back the save plugin and button (at the cost of 716 bytes, but we just saved 2.9Mb at 1.)
  4. Bring the autogrow plugin (at no cost).
  5. Switch to inline (vs iframe) mode, by bringing the divarea plugin, if and only if the checkbox "Use the administration theme when editing or creating content" is unchecked, to provide true wysiwyg (at least at css level)

Bonus : CKEditor from CDN is automagically translated in local language, even is Backdrop is not; I think it uses the browser setting (?)

Todo: Provide a permission for using the uploadImage plugin - this is stupid, but this is the only way to let the user know that it exists : By definition, it has no setting, no dialog, not even a button !

ghost commented 8 years ago

Changed issue title, as it's no more a question !

quicksketch commented 8 years ago

Tagging this for 1.4.0 so we make sure to get it updated before the next release.

klonos commented 8 years ago

:+1:

jenlampton commented 8 years ago

Thanks for all the great work on this @gifad!

Switch to CKEditor CDN.

Can you please change this back? We need all software in Backdrop to continue functioning even if the computer has no connection to the internet. That means all libraries must be bundled and not included from external sources.

Bonus : CKEditor from CDN is automagically translated in local language, even is Backdrop is not; I think it uses the browser setting (?)

If that is what we are after, perhaps we should provide an option for admins to switch to the CDN version, but add a warning about depending on external sources?

include uploadwidget, and provide a backdropuloadimage plugin, based on uploadimage and a ptch in file.module to implement it's required uploadurl.

Will this fix the bug we have currently with added images not being uploaded automatically? https://github.com/backdrop/backdrop-issues/issues/1199 If not, what is the intended purpose of this change?

@quicksketch can you weigh in on whether these changes should be included in this PR to update the library, or if we should discuss any or all all non-update related changes on separate issues, perhaps as part of https://github.com/backdrop/backdrop-issues/issues/1087

ghost commented 8 years ago

Switch to CKEditor CDN Can you please change this back?

Nope. If you need to use CKEditor offline (that is on your development machine), you are such a hard core developper that you already know how to edit your etc/hosts to redirect cdn.ckeditor.com to 127.0.0.1

CKEditor in local language

This not dependant on CKEditor hosted at cdn.ckeditor.com or at your.site.com. CKEditor cdn loads lang files in user's language, drupal provides an option, backdrop prevents it.

backdropuloadimage plugin

This is not at all a "change" to the current backdropimage plugin : this is an alternate way to upload images : instead of click "Browse", select a file, click "Upload", you just drag and drop a file from your local explorer/finder into the CKEditor window, and everything is done automatically (no click, no dialog); the only justification of the legacy "Browse" method is for disabled persons who cannot physically perform the mouse operation.

1199 is not a bug : when a file has been selected, no event is fired by the browser, and no software can react on that; it works on fields, because the whole form is built and posted by the browser.

The point I'd like you to check and comment, is about use of bacdrop for "paper like" sites - I mean papers like Science, Nature or other professional/amateur publications, where perfect typography and image/text relations are critical. To stay in Adobe's world, this is InDesign vs Photoshop, and I'd like a direct choice of one of these two modes, instead of entering a dozen of structure/configuration screens (half of which are missing) (and unfortunately, the backdrops defaults are not usable for the kind of sites I'm working on/for/with)

Sorry for such a long boring post...

Edit : https://drupalthemes.stanford.edu/ is a good example, in your area, of what I wish backdrop could provide...

mikemccaffrey commented 8 years ago

Nope. If you need to use CKEditor offline (that is on your development machine), you are such a hard core developper that you already know how to edit your etc/hosts to redirect cdn.ckeditor.com to 127.0.0.1

Personally, while I can go through that trouble, it is not something I want to do. And I feel like we actually have a deeper philosophical decision to make: do we want any libraries or scripts that are part of core pointing to CDNs, or whether everything you need to run Backdrop should be bundled for local development.

I'm a fan of the latter path. The more different systems we are relying on, the more weak links there are to bring all or part of a backdrop site down. I think that by default we should provide a local version of all of the required libraries, and then provide an option (either in core or contrib) to override that choice and point to a CDN whenever it is a possibility.

ghost commented 8 years ago

In a philosophical perspective, I'm not at all a fan of CDNs, but I do want a useable CKEditor, and this implies moving to 4.5.5+. Using the CDN, I can do it in minutes with my limited capabilities. When backdrop plugins will find a way to work reliably with a given CKEditor configuration, yes, it will be good to integrate this configuration into the backdrop distribution. Embedding an image in a link is something that fits in the backdrop strategy of 80/20, functioning even if the computer has no connection to the internet does not, hence my rude, I admit, reaction.

klonos commented 8 years ago

...or whether everything you need to run Backdrop should be bundled for local development.

I'm a fan of the latter path.

Me too.

...I think that by default we should provide a local version of all of the required libraries, and then provide an option (either in core or contrib) to override that choice and point to a CDN whenever it is a possibility.

Now this is a really great idea! Every 3rd party library could optionally declare a CDN URL and that switch could be something like "Switch to production environment". This would do things like switching to minified versions of libraries (something that the contrib jQuery module used to have) and loading things from CDNs.

klonos commented 8 years ago

@gifad "functioning even if the computer has no connection to the internet" might not fit the 80/20 use case as you say, but that statement is only valid for production sites (where it might be actually a 99/1 thing). Dev environments though might be a different case...

I believe that bundling everything that a dev needs to get going locally (think working on a plane/train while traveling) is the right way to go about having things. But as you say, perhaps best to implement features like allowing 3rd party libraries to either be easily replaced/overridden by custom ones (for example something like #159) or by loading them from CDNs. That should be an option though and not the default.

mikemccaffrey commented 8 years ago

I'm not at all a fan of CDNs, but I do want a useable CKEditor, and this implies moving to 4.5.5+. Using the CDN, I can do it in minutes with my limited capabilities.

Well, luckily when developing core we have access to our pooled capabilities, and don't need to depend on any single person's skills and time. And with something as unanimously supported, like this upgrade, it is pretty much guaranteed to be done without one person needing to push it through themselves.

Also, changing the version of CKEditor doesn't seem like something small that be pushed out in a bugfix release without breaking things. Therefore, we are going to have to make this change as part of the 1.4.0 version that will be released in May, so we have time to discuss how to make the change properly.

mikemccaffrey commented 8 years ago

Created #1593 to discuss whether we should create a way in core for libraries to be served via an external CDN. Let's keep this ticket focused on making the CKEditor upgrade in the next minor release.

dboulet commented 8 years ago

There are also security concerns with using a CDN, as the security of your site now depends on the security of a third-party, over which you have no control.

quicksketch commented 8 years ago

For this particular issue, we need to do one thing at a time. The PR at https://github.com/backdrop/backdrop/pull/1226 makes great strides, but does too many things at the same time:

Let's focus this issue on the primary task of simply updating CKEditor to the latest version. Note that we have a customized installation of CKEditor from CKEditor.com that contains fewer bundled plugins than the CDN version. To rebuild the Backdrop configuration you must:

ghost commented 8 years ago

indicating the plugins that are desired.

Just curious, what will be the protocol for the designation of the grand elected plugins ? The 80/20 rule ? Fortunately (as I obviously belong to the 20%), I managed to fullfill all my users needs just with CKEditor module hooks (it was much harder : one month vs one day for the 1226 PR) But the question stands for the 80% of the 80% (?), who just want feature parity with drupal 6/7, but don't want (or can't) develop a custom module...

quicksketch commented 8 years ago

Just curious, what will be the protocol for the designation of the grand elected plugins ? The 80/20 rule?

The original intention was just removing things that clearly didn't fit with Backdrop/Drupal concepts. e.g. removal of the font styles, background and forground colors, print button, form elements, flash embedding, etc. Those plugins may be useful for making a Word-like experience, but not so much inline with managing content and having a theme-provided styling through CSS. The full-featured demo at http://ckeditor.com/demo#full includes all kinds of things that probably aren't appropriate.

Some other plugins we need replacements for, like the Anchor plugin that is currently not working at all (#1231), that's a bug rather than intentional. Adding any new plugins should be done per-issue. I'm personally in favor of the auto image uploader plugin, had that existed when we first ported CKEditor it would have been included. But again, let's make a new issue for it.

serundeputy commented 8 years ago

PR Here: https://github.com/backdrop/backdrop/pull/1370 Adds ckeditor library 4.5.9

Testers away; Website: http://1370.backdrop.backdrop.qa.backdropcms.org Username: admin Password: h7Xpnoex

klonos commented 8 years ago

Testers away;

Do we have a list of things that need to be tested?

serundeputy commented 8 years ago

Do we have a list of things that need to be tested?

nope.

klonos commented 8 years ago

Then I say just go with it and see if things break :smile:

quicksketch commented 8 years ago

This PR looks great from a code standpoint. Nice job @serundeputy! There are no Backdrop-specific changes in the entire PR except for bumping up the version number in hook_library_info().

I tested out the sandbox (thanks again @Gormartsen!) and everything seems to be working perfectly.

So I can't find anything in my review. Unless you guys find anything, I think this is good to go.

quicksketch commented 8 years ago

I've pulled in https://github.com/backdrop/backdrop/pull/1370 to 1.x after being unable to find any problems or regressions in CKEditor after updating. An impressive feat for the CKEditor team maintaining compatibility between versions. I recall D8 having some issues with the admin interface for configuring CKEditor during the upgrade, but as our admin interface is implemented differently it seems we were spared that trouble.

Let's open new issues as needed if we encounter any bugs in the newest version.

klonos commented 8 years ago

Yay! :tada: Thanx everybody :clap:

jenlampton commented 8 years ago

I wanted to add a note here that upgrading the editor didn't fix the problem with Anchors not working: https://github.com/backdrop/backdrop-issues/issues/1231

jenlampton commented 7 years ago

... and it looks like regular links also got somehow removed? https://github.com/backdrop/backdrop-issues/issues/2142