Implement proper version checking everywhere

Here's a patch.

Hi, Joseph. I assumed the version checking was sound. My patch just expands on where it is being used.

I did have a specific issue after updating to latest CKEditor but it was fixed by #3007950: Improper version checking causes CKEditor 4.10.1 to break inserting images so isn't relevant to this patch.

Thanks!

*Edit: fixed issue link

@joseph.olstad - Sure, happy to provide more details if it helps.

When I updated our sites to CKEditor 4.11.4 from 4.9.2 (from our internal issue queue):

"Image properties" in right context menu not present on newly added images. It still exists and is functional on images that have been previously added. There is no discernible difference in "Source" mode (images are tokenized), and toggling source activates the menu item on the new image.

This was happening because 4.11.4 wasn't passing the version check fixed in #3007950: Improper version checking causes CKEditor 4.10.1 to break inserting images.

That being said, as part of our own "recipe" to get things working (at least to 90%) we've had to disable the widget functionality. We do that by removing the 'OnLoad' property in plugin.js which forces it to fall back to old token handling. (It looks like you've recently had a similar revelation over here?: #2813413-35: Unable to resize images inserted with Media). This resolves issues for us regarding accessing the "Image properties" options.

This doesn't fix everything, however. When a caption is added (by checking "caption" in Image properties) and the node is saved, when the node is subsequently edited we lose access to the media browser (in the image context) and instead get an error in the modal:

The requested page "/media/undefined/format-form?render=media-popup&fields=undefined" could not be found.

Anyway, despite the issues we've managed to keep 30+ schools reasonably happy for several years, so that's not bad. :)

Let me know if you need any more info.

A bit more info:

Forgot to mention that another issue with captioned images is that they can't be resized (either by dragging or via "Image properties"). Non-captioned images are fine.

CKEditor profile settings:

Selected plugins:
[x] Plugin for inserting Drupal teaser and page breaks.
[x] Enhanced Image plugin. See addon page for more details.
[x] Plugin for embedding files using Media CKEditor
[x] Plugin for embedding HTML snippets
[x] Table Resize plugin. See addon page for more details.

ACF: Disabled

Custom Javascript configuration:

config.format_tags = 'p;h2;h3;h4;h5;h6;div';
config.bodyClass = 'ckeditor-textarea';
config.autoParagraph = true;
config.forcePasteAsPlainText = true;
config.justifyClasses = [ 'text-left', 'text-center', 'text-right'];
config.extraPlugins = '';
config.removePlugins = 'image';

I added config.removePlugins = 'image'; since the update to 4.11.4 to deal with warnings in the console (below) and it doesn't seem to make a difference otherwise.

[CKEDITOR] Error code: editor-plugin-conflict. {plugin: "image", replacedWith: "image2"}
[CKEDITOR] For more information about this error go to https://ckeditor.com/docs/ckeditor4/latest/guide/dev_errors.html#editor-...

Versions:

- drupal 7.67
- media_ckeditor (latest dev)
- media 7.x-2.19
- ckeditor 7.x-1.18
- file_entity 7.x-2.21

Cheers -

**Edit - added project versions

Huh?

Again, I did not introduce the version checking in this patch. It was introduced by yourself and @ron_s in this issue: #3007950: Improper version checking causes CKEditor 4.10.1 to break inserting images

Maybe you will find some answers to your concerns here, as it looks to be where this technique originated. It also has tests: https://stackoverflow.com/a/16187766/4483099

If implementing calls to a version check that *you* committed for the sake of being thorough "smells fishy", then I can't be of any assistance here. I honestly don't care if you use it or not.

Regarding #15 - Unless I fall back to the old token handling (as I described in #9) I don't get "Image properties" in the menu in either 4.9.x or 4.11.x. I can't easily roll back to 4.6.2 as I get unrelated JS errors that I don't have time to look into, sorry.

This applies to newly added images only, not images that are already embedded and saved.

So what you describe in #15 does not surprise me at all.

To answer your question...

Are you able to get image properties with 4.11.4 ?

Yes, but only by disabling widget support as described in #9.

Glad you found a use for the patch. :)

Thanks, yes I do but I'm not having issues. I was just giving you feedback on #15.

I just noticed that the patch you just committed (49fbf58d) does the exact same thing as what I described in #9. It disables widget support, which means it falls back to old token handling. Our own patch removes the onLoad altogether. The end result is the same.

Actually, you might as well remove the onLoad as well as any code that follows that requires hasWidgetSupport to evaluate to true, since without that one line it will never run.

@joseph.olstad

does this mean it falls back to legacy token ?

I don't think so, but I don't know enough about how the media module works to know for sure. (Ours have always been set to "Full file entity rendering".)

What I meant is, in mediaPluginDefinition.init() (plugin.js), any code that requires hasWidgetSupport to be true will never be run because it will never be true.

So this bit will never be run:

      // Ensure the tokens are replaced by placeholders while editing.
      // Check for widget support.
      if (mediaPluginDefinition.hasWidgetSupport) {
        editor.widgets.add( 'mediabox',
        {
          button: 'Create a mediabox',
          editables: {},
          allowedContent: '*',
          upcast: function( element ) {
            // Ensure media tokens are converted to media placeholders.
            html = Drupal.media.filter.replaceTokenWithPlaceholder(element.getHtml());
            // Only replace html if it's different
            if (html != element.getHtml()) {
              element.setHtml(html);
              // CKEditor's setHtml() method automatically fixes the HTML that
              // it receives, which means that if it gets an <li> without a
              // <ul> or <ol> parent, it adds a <ul>. These extra <ul> tags just
              // keep piling up every time this function runs. So check here to
              // see if we may need to fix this.
              if (element.children && element.children[0]) {
                // We identify this by looking for a <ul> inside a <ul> or <ol>.
                if (('ul' === element.name && 'ul' === element.children[0].name) ||
                    ('ol' === element.name && 'ul' === element.children[0].name)) {
                  // If this did happen, fix it by promoting the grandchildren
                  // (ie, actual list items) to children.
                  element.children = element.children[0].children;
                }
              }
            }
            return element.name == 'mediawrapper' || 'data-media-element' in element.attributes;
          },

          downcast: function( widgetElement ) {
            var token = Drupal.media.filter.replacePlaceholderWithToken(widgetElement.getOuterHtml());
            if (token) {
              return new CKEDITOR.htmlParser.text(token);
            }
            return false;
          },

          init: function() {
            // Add double-click functionality to the widget.
            this.on('doubleclick', function(evt) {
              editor.execCommand('media');
            }, null, null, 5 );
          }
        });
      }
      // ...

I wonder if it's worth looking at whether it can be fixed before removing it altogether? I honestly have no idea. Beyond occasionally tweaking to make things work I'm not experienced with CKEditor plugins.