One goal of the patch is to more easily enable other modules to add their own plugins. However, CKEditor also provides its own plugins. We should remove any plugins that are module dependent but keep the ones that are internal.

Additionally it moves the plugin definitions to a new file ckeditor.ckeditor.inc and documents the new hook_ckeditor_plugins_alter in ckeditor.api.php.

Original report

A patch for Media was recently committed - it came with a companion patch for CKEditor that does many things (declares hooks in hook_hook_info, adds an alter function, moves code around, etc). It was originally written by Devin Carlson. See: #1504696: Integration with CKEditor module

Specifically, where a maintainer of this module asks for the patch to be filed against CKEditor. https://drupal.org/comment/8243115#comment-8243115

The patch in question is already uploaded to D.O. here: https://drupal.org/files/issues/ckeditor-accomodate-latest-media-changes...

I am reposting it to this issue so that it will go up against any automated tests that may be set up.

If this patch is committed, proper attribution should be to Devin Carlson: https://drupal.org/user/290182

CommentFileSizeAuthor
#166 interdiff-2159403-141-166.txt6.5 KBaDarkling
#166 make_ckeditor_plugin-2159403-166.patch23.66 KBaDarkling
#164 interdiff-2159403-141-164.txt6.46 KBaDarkling
#164 make_ckeditor_plugin-2159403-164.patch23.67 KBaDarkling
#161 interdiff-2159403-141-161.txt6.47 KBaDarkling
#161 make_ckeditor_plugin-2159403-161.patch23.67 KBaDarkling
#153 make_ckeditor_plugin-2159403-153.patch14.32 KBaDarkling
#141 interdiff-2159403-134-141.txt2.71 KBDevin Carlson
#141 make_ckeditor_plugin-2159403-141.patch22.75 KBDevin Carlson
#137 ckeditor-working-clean.png168.32 KBsaltednut
#134 make_ckeditor_plugin-2159403-134.patch22.69 KBDevin Carlson
#134 interdiff-2159403-107-134.txt17.28 KBDevin Carlson
#115 filtered-html.png65.9 KBsaltednut
#115 ckeditor-config.png27.07 KBsaltednut
#107 interdiff-2159403-100-107.txt3.93 KBwwalc
#107 make_ckeditor_plugin-2159403-107.patch39.42 KBwwalc
#101 ckeditor_plugin_system_cleanup-2159403-100.patch38.57 KBwwalc
#99 interdiff-2159403-90-99.txt3.72 KBwwalc
#99 ckeditor_plugin_system_cleanup-2159403-99.patch38.36 KBwwalc
#91 make_ckeditor_plugin-2159403-90.patch38.84 KBmicbar
#89 interdiff.txt2.83 KBmicbar
#89 make_ckeditor_plugin-2159403-89.patch38.62 KBmicbar
#81 ckeditor-accomodate-latest-media-changes-81.patch37.13 KBagoradesign
#81 interdiff-2159403-68-81.txt2.77 KBagoradesign
#75 ckeditor-accomodate-latest-media-changes-75.patch36.92 KBagoradesign
#68 ckeditor-accomodate-latest-media-changes-68.patch37.75 KBAngry Dan
#65 ckeditor-accomodate-latest-media-changes-65.patch37.53 KBAngry Dan
#65 interdiff.txt1.38 KBAngry Dan
ckeditor-accomodate-latest-media-changes-0.patch17.81 KBsaltednut
#6 ckeditor-accomodate-latest-media-changes-2159403-6.patch17.85 KBsaltednut
#15 media-ckeditor-button-poc.patch761 bytesdas-peter
#15 media-ckeditor-plugin-definition-fix.patch1.42 KBdas-peter
#20 interdiff-2159403-6-20-do-not-test.diff507 bytesdas-peter
#20 ckeditor-accomodate-latest-media-changes-2159403-20.patch18.1 KBdas-peter
#23 ckeditor-accomodate-latest-media-changes-2159403-20.patch17.94 KBdas-peter
#39 ckeditor-accomodate-latest-media-changes-2159403-39.patch18.19 KBiKb
#44 ckeditor-accomodate-latest-media-changes-2159403-44.patch18 KBjoelpittet
#44 interdiff.txt2.11 KBjoelpittet
#47 available_buttons_current.png7.2 KBwwalc
#47 available_buttons_after_patch.png5.77 KBwwalc
#47 plugins_current_code.png15.77 KBwwalc
#47 plugins_after_patch.png11 KBwwalc
#48 ckeditor-accomodate-latest-media-changes-48.patch20.5 KBDevin Carlson
#53 ckeditor-accomodate-latest-media-changes-53.patch22.21 KBDevin Carlson
#56 2159403-56-ckeditor-accomodate-latest-media-changes.patch22.84 KBbrockfanning
#56 interdiff-2159403-56-ckeditor-accomodate-latest-media-changes.txt645 bytesbrockfanning
#62 interdiff.txt15.06 KBAngry Dan
#62 ckeditor-accomodate-latest-media-changes-62.patch37.67 KBAngry Dan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

deggertsen’s picture

Status: Needs review » Reviewed & tested by the community

I think this should be RTBC since it has already been reviewed and tested. See #1504696: Integration with CKEditor module

jcisio’s picture

Status: Reviewed & tested by the community » Needs review

I was trying to review the patch, but it turned out that it would took more time than I'd thought. I don't think this patch was reviewed enough in CKEditor as it has a large impact on how it touches files. It needs test and review on admin UI and Features export to see if there are changes.

saltednut’s picture

I have seen threee issues with this patch, 2 that are minor, 1 major:

1. When editing CKEditor appearance, ie: adding or removing buttons. After saving, the 'Media' button disappears from the actual rendered WYSIWYG. I can see it on the CKEditor profile settings, but its gone from the Editor itself.

The only way I've found to get around this is to go into Features and export the CKEditor profile. I then can update the export via text editing, and add in 'Media' manually to the exportable. Then reverting the Feature makes the 'Media' button return to the WYSIWYG. Unsure if that is something Media has to fix or if its on the CKEditor side of things.

2. There are now two checkboxes for inserting Drupal media in the plugin settings. One that is checked by default, "Plugin for inserting images from Drupal media module" and another that is not checked, and is seemingly unnecessary, "Plugin for inserting Drupal embeded media"

There is also a somewhat major issue with the ACF.

3. Basically, the Media embed codes get wiped upon editing an existing node with media inside it. Workflow to repeat:

  • Create a new article with an embedded media in it.
  • Save the article and you'll see your node.
  • Click 'Edit' tab and load the node/edit screen.
  • You'll see the media embed code flash and then disappear in CKEditor.

This can be avoided by simply turning off the Advanced Content Filter.

das-peter’s picture

@brantwynn I've tested the patch and after some patching it seemed to work as intended. Created patches:
#1995030: Support for WYSIWYG in summary
#2164823: Fix for replacePlaceholderWithToken in media_wysiwyg.filter.js
#2164831: Fix for CKEditor ACF (Advanced Content Filtering) configuration

I did a visual review of the patch too, but I found just some nitpicks. The rest looks quite good to me.
Could you check if the problems you've described are gone with the patched I've created? (1 & 2 weren't occurring in my installation, but 3, did)

Nitpicks:

  1. +++ b/ckeditor.ckeditor.inc
    @@ -0,0 +1,169 @@
    +      'default' => 't'
    ...
    +      'default' => 't'
    ...
    +      'default' => 't'
    ...
    +          'icon' => 'linkit.png'
    +        )
    ...
    +      'default' => 't'
    ...
    +      'default' => 'f'
    ...
    +      'default' => 'f'
    ...
    +            'default' => 'f'
    ...
    +            'default' => 'f'
    ...
    +            'default' => 'f'
    ...
    +            'default' => 'f'
    

    A comma should follow the last multiline array item.

  2. +++ b/ckeditor.ckeditor.inc
    @@ -0,0 +1,169 @@
    +    while (false !== ($file = readdir($handle))) {
    ...
    +    while (false !== ($file = readdir($handle))) {
    ...
    +  //remove page break button if there is no module to do this
    

    TRUE, FALSE and NULL must be uppercase; expected "FALSE" but
    | | found "false"

  3. +++ b/ckeditor.ckeditor.inc
    @@ -0,0 +1,169 @@
    +            'desc' => t('Plugin file: ' . $file),
    

    Concatenating translatable strings is not allowed, use
    placeholders instead and only one string literal

  4. +++ b/ckeditor.ckeditor.inc
    @@ -0,0 +1,169 @@
    +  //remove page break button if there is no module to do this
    

    - No space before comment text; expected
    - Inline comments must start with a capital letter.
    - Inline comments must end in full-stops, exclamation marks, or question marks.

jcisio’s picture

I don't think we bother anything else except that whether the CKEditor plugin system, the admin UI and exported config continue to work after this patch. This patch includes API changes and zero modification in JS files.

If 1/ and 2/ are not reproducible (but I think we need more confirmation), then with those nitpicks fixed, the patch is good to go.

saltednut’s picture

I am still seeing errors 1 & 2 from my earlier posts, but this may have something to do with Features. If no one else can reproduce, I assume its environmental.

I've attached a new patch that should fix the nitpicks from #4.

jlyon’s picture

@brantwynn:

I am also experiencing the same issue as your #1 in comment #3. Both the Insert Media and Insert Drupal Teaser Break buttons are not showing up on the CKEditor profile edit page on the Appearance drag and drop. I have been able to get around this by inspecing the title of that field and disabling the display: none css code, but it's definitely annoying. I did not see any JS or PHP errors anywhere.

I am not able to reproduce the issues in your #2 comment after upgrading to the latest media-dev with #2164831: Fix for CKEditor ACF (Advanced Content Filtering) configuration

jlyon’s picture

I am also experiencing a similar minor bug where CKEditor seems to remove the <mediawrapper> element wrapping the {fid} code.

Steps to reproduce:
1. Insert a picture
2. Click on the picture. The path at the bottom of the image includes a mediawrapper element.
3. Click on view source, then go back to the WYSIWYG.
4. Click on the image again. The path at the bottom of the image no longer includes the mediawrapper element.

I have all three patches from das-peter's comment in #4. I have tried with ACF turned both on (with #2164831: Fix for CKEditor ACF (Advanced Content Filtering) configuration) and off.

I believe a similar issue is at play if you try to link an image that was inserted via media. Steps to reproduce:
1. Insert a picture
2. Click on the picture. Click on the link button, add a link.
3. Click view source. The mediawrapper element is gone, as is the a element.
4. Go back into wysiwyg, create a new link, view source, the link appears.
4+. Similarly, if you view source between inserting the media and clicking the link button, the link will work.
It appears that the presence of the mediawrapper element in this instance breaks the link button.

philipz’s picture

I see no errors. Patch #6 works fine. I've tested adding and uploading files in CKEditor.

PI_Ron’s picture

I've tried the following and can't get the media embed button to appear in ckeditor profile settings:

  1. Fresh install of Drupal 7.25
  2. Latest media dev
  3. Latest file_entity dev
  4. Latest ckeditor dev with patch from #6 and -0 patch from https://drupal.org/node/2158741
  5. Ckeditor 4.3.1 full
  6. [x] Convert media tags to markup ticked
  7. [x] Plugin for inserting images from Drupal media module ticked.

Thanks for all your work on this and apologies if I am missing something simple here!

saltednut’s picture

Re #10 - I don't think you're doing anything wrong.

I assume you have a custom appearance, no? That is to say, you've configured the buttons in a specific order?

From my experience using this patch, the button only shows up if the button configuration is left default - OR - one conducts this work-around.

1. Install Features - you should be using it anyway.
2. Export your CKEditor profile into a Feature
3. Add the [media] button to the profile manually.

Here is a link to an example Features export that does this. It is the only way that I am able to get the Media button to show up in custom configurations.
-- http://drupalcode.org/project/curate.git/blob/refs/heads/7.x-1.x:/curate...

jlyon’s picture

Note: To fix the issues with the media icon not appearing in the admin ui (actually any third party icon not appearing, including the Drupal Teaser Break, and the ), you need to apply the patch at http://drupal.org/files/issues/ckeditor-remove-external-plugin-declarati... from #2158741: External plugin declarations are redundant. (Linkit, ckeditor_link, ckeditor_swf not working).

I could re-roll this patch with that applied, but that doesn't seem like a wise idea. I have the following patches applied and everything is now working for me:
- http://drupal.org/files/issues/ckeditor-accomodate-latest-media-changes-...
- http://drupal.org/files/issues/ckeditor-remove-external-plugin-declarati...
- http://drupal.org/files/issues/ckeditor-hook_into_media_admin-1649464-81...

saltednut’s picture

Hi @jlyon - I actually wrote the patch in #2158741: External plugin declarations are redundant. (Linkit, ckeditor_link, ckeditor_swf not working) and we are currently using it - but still experiencing the bug.

So... I don't think that patch fixes the bug. Perhaps its something else about your configuration that's fixing this error.

PI_Ron’s picture

Ok without changing any of the default button orders under appearance the button appears when using wysiwyg. I'm getting the following error though at the third step of media button process:

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

**Edit** Forget this, I just had to enabled media_wysiwyg module

das-peter’s picture

I just came across a case in which no media button was displayed.
It really looks like we've quite a mess with the declaration of the ckeditor media plugin (or the ckeditor plugin definitions at all).
The patch media-ckeditor-button-poc.patch shows the minimal changes I had to do to get the button.
The patch media-ckeditor-plugin-definition-fix.patch is the approach I would prefer to clean this mess.
Btw. features has indeed impact on the whole issue. It looks like "missing" plugin definitions are resurrected by hook_ckeditor_profile_defaults(). This means even if the plugins defined in 'loadPlugins' => array() aren't available anymore they are added to the plugin list. That "somewhat" works because 'loadPlugins' => array() not only contains the name of the plugin to load but the whole definition of the plugin - what seems to be totally wrong in my eyes...

Status: Needs review » Needs work

The last submitted patch, 15: media-ckeditor-plugin-definition-fix.patch, failed testing.

The last submitted patch, 15: media-ckeditor-button-poc.patch, failed testing.

deggertsen’s picture

@das-peter, your patches are for the media module, not ckeditor. So they should probably be posted in a separate issue over there. However, the second patch fixes the problem of the media button now showing up. So thanks for that!

media-ckeditor-plugin-definition-fix.patch

PI_Ron’s picture

@deggertsen so do we apply that patch to latest media dev?

das-peter’s picture

@deggertsen thanks for letting me know that the patch helps - now it's worth to open a dedicated media issue.

I've found another issue with the plugin button handling in ckeditor itself. It has a regex pattern to detected buttons in the plugin code, but unfortunately it doesn't seem to work always.
The attached patch extends the patch from #6 with an fixed pattern. Check the interdiff to see what I've changed - it's just a minimal change to ensure it properly works with multi-line styling of the js code.
This change might fix the media button issue without patching media itself, however I still think we should patch media to ensure media_wysiwyg is taking absolute control if present.

Status: Needs review » Needs work

The last submitted patch, 20: ckeditor-accomodate-latest-media-changes-2159403-20.patch, failed testing.

das-peter’s picture

das-peter’s picture

Ouch, looks like somehow the line-endings in the patch were messed up. Fixed patch.

das-peter’s picture

Status: Needs work » Needs review
mikgreen’s picture

Can you please verify this bug. We encountered this on a project that uses latest madia module and this patch.

1. Select some text in Ckeditor.
2. Click "Add/Remove Numbered List" or "Add/Remove Bulleted List" button to make a list.
3. Click "Source" button to view html.
4. Click "Source" button to get back.

Result:
1. All content in editor field is gone.
2. All buttons in editor are grayed out.
3. Console shows error: "Uncaught RangeError: Maximum call stack size exceeded ckeditor.js"

I'd appeciate if someone could test if this is happening with your setup as well. Thanks!

kevinchampion’s picture

I can confirm the problem in #8. Every time the editor binds/unbinds the mediawrapper is stripped. I was hoping that part of this refactor was meant to address this problem and make it easier to deal with. However, is this issue now likely due to the media side of things? It looks like the relevant code might be in the media_wysiwyg module now.

das-peter’s picture

@mikgreen / @kevinchampion Please specify the exact CKEditor version as well as the enabled ckeditor plugins (especially if the widgets plugin is enabled). Otherwise can be very hard to reproduce issues since we're dealing with a version set from 3.6 up to 4.1.x.

saltednut’s picture

I believe turning off CKEditor's ACF provides a workaround for the issue described in #8 - I have seen the issue but not when the Advanced Content Filter is off.

mikgreen’s picture

For #25 I tried CKEditor versions 4.3 and 4.3.1. Happens in both versions.

Also: The browser is Chrome 32.0.1700.77.
Haven't tried with anything else.

I'd appreciate if someone would check this out and try to reproduce it. Maybe I'm doing something wrong.

kevinchampion’s picture

I can confirm #8 with the following ckeditor configuration:

https://dl.dropboxusercontent.com/u/57694/Screenshots/ckeditor_config.png

and the following module versions:

https://gist.github.com/kevinchampion/8514861

I've also tried downloading a custom version of ckeditor library with the widget plugin included in the bundle and get the same type of results.

Inspecting the javascript loaded on the page and plugin.js from media_wysiwyg isn't loaded on the page. Is it supposed to be? Update: nevermind, plugin.js is there.

kevinchampion’s picture

Created a new issue and patch to deal with issues I've seen that correspond with #8:

https://drupal.org/node/2177893

saltednut’s picture

Quick question, has anyone tested #23? Also, @das-peter, does #23 address the missing button issue?

ayalon’s picture

The patch #23 does not work at all. media_wysiwyg_include_browser_js() is called to late in the function ckeditor_profile_settings_compile().

The javascript settings which should be added to the page whitin the function media_wysiwyg_include_browser_js() do not appear in the source code and therefore the module is not saving overridden attributes and so on.

hefox’s picture

  1. +++ b/ckeditor.ckeditor.inc
    @@ -0,0 +1,169 @@
    +  /*
    

    **

  2. +++ b/ckeditor.ckeditor.inc
    @@ -0,0 +1,169 @@
    +  if (module_exists('ckeditor_swf') && file_exists(drupal_get_path('module', 'ckeditor_swf') . '/plugins/swf/plugin.js')) {
    

    Why are these two modules not doing their own hook_ckeditor_plugin?

  3. +++ b/ckeditor.ckeditor.inc
    @@ -0,0 +1,169 @@
    +            'desc' => t('Plugin file: ' . $file),
    

    This isn't really correct usage of t function

  4. +++ b/ckeditor.ckeditor.inc
    @@ -0,0 +1,169 @@
    +  if (isset($plugins['media']) && module_exists('media') == FALSE) {
    

    !module_exists is more common style wise than == FALSE

saltednut’s picture

Re: 34.2, that issue is addressed in #2158741: External plugin declarations are redundant. (Linkit, ckeditor_link, ckeditor_swf not working) which is an accompaniment to this patch. (To be applied after, hence the failing test on it)

Edit: I think a Meta issue is in order. #2217579: [Meta] Getting this Module working with latest Media 2.x-dev

saltednut’s picture

rooby’s picture

Status: Needs review » Needs work

Patch (#23) no longer applies. Needs re-roll.

rooby’s picture

Also seems like this could be a bug and higher priority if media integration is broken.

iKb’s picture

Patch based on the latest dev.

iKb’s picture

Status: Needs work » Needs review
rooby’s picture

Awesome thanks, it's working now.
I will report back if I find any bugs.

jcisio’s picture

Status: Needs review » Needs work

Nice work! I review again the patch and it looks good. I think we just need to wait a few days for feedback and I'll commit it.

Also, #39 did not address #34, could it be fixed? I know it is basically code move (#34.3) however it does not hurt to fix.

brockfanning’s picture

I don't know specifically what the patch fixes, but I was getting javascript errors when I clicked the final "Submit" button to insert Media-module media into Ckeditor, and after applying this patch I'm no longer getting the errors. So, I am grateful, thanks! :)

joelpittet’s picture

Here is a reroll (they removed the EOF ?> already on ckeditor.api.php).
Plus the changes in #39 and the addressing review from @hefox in #34.

1) May cause conflicts but did it anyways...
2) Don't know the answer to this one... maybe you can post what this should be?
3) Fixed.
4) Agreed and fixed.

joelpittet’s picture

Status: Needs work » Needs review

Testbot rollout!

rooby’s picture

You can ignore point 2. See #35.

wwalc’s picture

Status: Needs review » Needs work
FileSize
7.2 KB
5.77 KB
15.77 KB
11 KB

@all - thanks a lot for your hard work on this patch. I’m sure it wasn’t easy to work with ugly legacy code in many places and with no tests.

I’m not going to give a full review of this as I have very little knowledge about the Media module, however I just wanted to point out some mistakes that I noticed in the latest patch.

1. The code in ckeditor.ckeditor.inc (ckeditor_ckeditor_plugin()) does not include the latest changes introduced in CKEditor module to support loading CKEditor from a remote URL (and the official CDN)

For example:

if ($_editor_path != '<URL>') {
    if (file_exists($_editor_path . '/plugins/tableresize/plugin.js')) {
      $arr['tableresize'] = array(
        'name' => 'tableresize',
        'desc' => t('Table Resize plugin'),
        'path' => $editor_path . '/plugins/tableresize/',
        'buttons' => FALSE,
        'default' => 't'
      );
    }
(...)
}
else {
    $_editor_url = ckeditor_path('url');
    if (preg_match("/\/(standard|full)-all/", $_editor_url)) {
      $arr['tableresize'] = array(
        'name' => 'tableresize',
        'desc' => t('Table Resize plugin'),
        'path' => $_editor_url . '/plugins/tableresize/',
        'buttons' => FALSE,
        'default' => 't'
      );
(...)

has been replaced with just:

$_editor_path = ltrim($editor_path, './');
  if (file_exists($editor_path . 'plugins/tableresize/plugin.js')) {
    $plugins['tableresize'] = array(
      'name' => 'tableresize',
      'desc' => t('Table Resize plugin'),
      'path' => $_editor_path . 'plugins/tableresize/',
      'buttons' => FALSE,
      'default' => 't',
    );
  }

You can easily verify this by downloading the latest dev version of CKEditor module. The path to CKEditor is set to a remote CDN, to a package with all available plugins. The tableresize or autogrow plugins may be enabled on demand in the CKEditor profile. After applying the patch and making sure that Drupal is using the updated code, it is no longer possible.

Before:

After:

2. The code for registering “CKEditor module plugins” and “CKEditor module plugins - additional directory” has been simplified without any explanation why was it done. Basically, the code that tries to find the buttons (icons) provided by a plugin has been removed.

  /*
   * CKEditor module plugins
   */
  $_plugin_dir = ckeditor_module_path('local') . '/plugins/';
  if ($handle = opendir($_plugin_dir)) {
    while (false !== ($file = readdir($handle))) {
      if (is_dir($_plugin_dir . $file) && file_exists($_plugin_dir . $file . '/plugin.js')) {
        $source = file_get_contents($_plugin_dir . $file . '/plugin.js');
        $buttons = array();
        if (preg_match_all($pattern, $source, $matches)) {
          foreach ($matches[1] as $i => $button_name) {
            if (preg_match('#(icon)[\s]*\:[\s]*([^\,\n]*)#', $matches[2][$i], $matches2)) {
              $buttons[$button_name] = array();
              $buttons[$button_name]['label'] = $button_name;
              $matches2[2] = str_replace(array('this.path', '+', '\'', '"'), array('', '', '', ''), $matches2[2]);
              $buttons[$button_name]['icon'] = trim($matches2[2]);
            }
          }
        }
        if (preg_match('#@file ([^\n\r]*)#', $source, $matches)) {
          $arr[$file] = array(
            'name' => $file,
            'desc' => t($matches[1]),
            'path' => $plugin_dir . $file . '/',
            'buttons' => (count($buttons) > 0) ? $buttons : FALSE,
            'default' => 'f'
          );
        }

has been replaced with:

  /**
   * CKEditor module plugins
   */
  $_plugin_dir = ltrim($plugin_dir, './');
  if ($handle = opendir($plugin_dir)) {
    while (FALSE !== ($file = readdir($handle))) {
      if (is_dir($plugin_dir . $file) && file_exists($plugin_dir . $file . '/plugin.js')) {
        $source = file_get_contents($plugin_dir . $file . '/plugin.js');
        if (preg_match('#@file ([^\n\r]*)#', $source, $matches)) {
          $plugins[$file] = array(
            'name' => $file,
            'desc' => t($matches[1]),
            'path' => $_plugin_dir . $file . '/',
            'buttons' => FALSE,
            'default' => 'f',
          );
        }

The main idea behind this feature was to allow users to simply unpack additional JavaScript plugins to the “plugins” folder in the CKEditor module directory and be able to enable them in the toolbar without writing any additional module.

You may test this by using the current version of CKEditor module, and unpacking the youtube plugin in sites/all/modules/ckeditor/plugins folder. The button will magically appear then in the CKEditor administration area in the toolbar configuration tool.

Actually, you don’t have to download this additional plugin, because you can test it as well using e.g. imce/mediaembed plugins that are in this folder (for imce, you need to install that module first). You can see that these additional buttons are gone after applying the patch.

Before:

After:

Considering that this feature has been already for ages and might be used by many existing instances, I would not remove it. It should not conflict in any way with the Media support which is introduced here and in general I believe that such changes in module behavior should be handled in separate issues, to have a clean track of why eventually something has been dropped.

I hope you agree with me and I keep my fingers crossed!

Devin Carlson’s picture

Status: Needs work » Needs review
FileSize
20.5 KB

@wwalc, thanks for the feedback!

An updated version of the original patch which addresses the points from #47.

The patch is mostly moving code around and the automatic parsing of plugins is left alone with the exception of the final check for plugin.js and buttons. I believe the final check is redundant since plugins in the ckeditor library are already detected and plugins provided by modules already have to declare their buttons. The latter can lead to tricky problems such as the hook implementation specifying an incorrect path but the plugin buttons are still displaying because of the auto detection.

I also haven't cleaned up any existing plugin definitions, as much of them will go away with #2158741: External plugin declarations are redundant. (Linkit, ckeditor_link, ckeditor_swf not working).

martindilling’s picture

Status: Needs review » Needs work

The last submitted patch, 48: ckeditor-accomodate-latest-media-changes-48.patch, failed testing.

martindilling’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 48: ckeditor-accomodate-latest-media-changes-48.patch, failed testing.

Devin Carlson’s picture

Status: Needs work » Needs review
FileSize
22.21 KB

A reroll of #48.

hefox’s picture

+++ b/ckeditor.api.php
@@ -45,10 +45,10 @@ function hook_ckeditor_plugin() {
+      'path' => '/' . drupal_get_path('module', 'my_module') . '/plugin_dir/',

Is this correct, '/'? shouldn't it be base_path()?

agoradesign’s picture

Today I wanted to try CKEditor module instead of WYSIWYG, but unfortunately had the problem, that media tokens are immediately converted to img tags.

I've posted more info here in this thread: https://drupal.org/node/2126755#comment-8758547

brockfanning’s picture

Please forgive if this is not the appropriate place/procedure for this. This (much appreciated) patch seems to be all about making CKeditor and Media play nice together, and although it gets me almost there, I'm still getting javascript errors when I try to embed non-image media, such as audio files. The attached change fixes my problem, but I'm a media/ckeditor newbie, so I hope someone with more experience can review. Thanks.

saltednut’s picture

#53 is working for me in every scenario except for when I am doing inline editing using http://drupal.org/project/edit <-- Backport from D8.

In that scenario, the Media button shows up, but nothing happens when I click on it.

I think that's a total edge case though, and should be a followup.

jcisio’s picture

Title: CKEditor accomodate latest Media changes » Make CKEditor plugin system modular and clean

Change the issue title to make it clear. Otherwise we won't finish.

As described in #2, what needs to be tested (before and after) is:
1. CKEditor admin UI still works (plugin settings)
2. Features integration still works (features update/revert, no feature change before and after the patch).
3. Front end still works: basic buttons and CKEditor behavior.

Then it should be OK to go. The patch is rolled with that idea in mind, thus I keep the issus status as "needs review".

DON'T test this patch with another patch and another patch and another moving patch.

PI_Ron’s picture

@brantwynn Do you know of any way to have media button working with edit module?

saltednut’s picture

Re #59 - I have had mixed results, actually. It seems to function since updating my profiles recently and using the latest Edit dev.

brockfanning’s picture

Going to hide my patch, since it's not really related to what's described in #58.

Angry Dan’s picture

I think this issue's description needs to be updated to reflect what the patch actually does (and I mean everything), to cover off motivations and to be clear on what's left to do.

Referring to #58/#2, as far as I can tell:

> 1. CKEditor admin UI still works (plugin settings)
Works for me.
> 2. Features integration still works (features update/revert, no feature change before and after the patch).
Features integration does still work and there is no diff in the feature, although that is a problem as I'll explain in a moment.
> 3. Front end still works: basic buttons and CKEditor behaviour.
Not quite. If the old CKEditor media plugin was enabled before applying the patch, then it won't work afterwards. This is because we're actually storing a reference to the plugin path in the database. The plugin is now duplicated which isn't compatible with module_invoke_all(), but even if you remove the plugins/media folder the database still references the now missing folder.

So, the attached patch does two things: a) Remove the now redundant media plugin from this module, and b) provide an update hook to fix the invalid path issue. Which brings me back to point 2 - features - we have to fix the references in the database, and therefore we have to mark the feature overridden.

The last submitted patch, 62: ckeditor-accomodate-latest-media-changes-62.patch, failed testing.

moonray’s picture

Status: Needs review » Needs work

The patch in #62 removes plugins/media/library.js but it's still being referenced in ckeditor_profile_settings_compile() (in ckeditor/includes/ckeditor.lib.inc ).
Seems things were missed.

Since I'm not entirely sure of the why and how, I'm not able to update the patch properly.

Angry Dan’s picture

As far as I can tell that reference is no longer necessary, and the attached patch removes it.

I'm actually a little confused by the code in ckeditor_profile_settings_compile():

      if (module_exists('media_wysiwyg')) {
        media_wysiwyg_include_browser_js();
        drupal_add_js(drupal_get_path('module', 'media_wysiwyg') . '/js/media_wysiwyg.filter.js');
        drupal_add_js(drupal_get_path('module', 'media_wysiwyg') . '/wysiwyg_plugins/media_ckeditor/library.js', array('scope' => 'footer', 'weight' => -20));
      }

Is it not possible for media_wysiwyg to do this for us? Why do we have to do it? We're introducing a cross dependency here that seems unnecessary. I'll do some digging, but it's possible that CKEditor is going to have to provide some kind of hook to each module that's providing an active plugin, or some other method of allowing them to including their ancillary files.

      else {
        module_load_include('inc', 'media', 'includes/media.browser');
        $javascript = media_browser_js();
        foreach ($javascript as $key => $definitions) {
          foreach ($definitions as $definition) {
            $function = 'drupal_add_' . $key;
            call_user_func_array($function, $definition);
          }
        }
      }

As far as I can tell, this code is the legacy stuff tied to the older media browser we had in CKEditor before now, so I'm removing it given that I've removed the rest of CKEditor's built in support for the media module.

If I'm honest I don't think that the CKEditor module's media functionality has ever worked properly anyway, so stripping it all out is probably for the best.

Angry Dan’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 65: ckeditor-accomodate-latest-media-changes-65.patch, failed testing.

Angry Dan’s picture

Status: Needs work » Needs review
FileSize
37.75 KB

So just realised this should have been a --binary patch; otherwise it's identical to #65.

drclaw’s picture

Hi,

Tested out the patch today. As far as I can tell it's working as expected, but there are a couple quirks using it on a site that already has other ckeditor plugins installed. Any existing hook_ckeditor_plugins implementations will need to have the path updated to include the base_path, otherwise it will be made relative to the ckeditor library directory. If you're getting errors that look something like the following:

Failed to load resource: the server responded with a status of 404 (Not Found) http://mysite.localhost/sites/all/libraries/ckeditor/sites/all/modules/contrib/ckeditor_tabber/tabber/plugin.js?t=E4KA

...you'll need patch the module providing the plugin HOWEVER, after fixing the path, you'll need to save the ckeditor settings page again. I also needed to clear my browser's file cache as it was caching some of the ckeditor configuration. And you know... maybe clear Drupal's cache as well... for good measure...

On a related note to the above, I think Comment #54 is still valid. The ckeditor.api.php example has a hard coded / but should probably be base_path()... unless there's some reason I'm unaware of?

tanc’s picture

I concur with drclaw's findings. Media embedding works as it should from my quick tests, with the token being used rather than the img tag. The linkit (7.x-3.x-dev) module was causing ckeditor's javascript to break but disabling that module allowed ckeditor to load.

agoradesign’s picture

I've just tested the Media integration on a clean install, with the following setup:

* Drupal 7.30
* File Entity 7.x-2.0-alpha3+32-dev (2014-Jul-31)
* Media 7.x-2.0-alpha3+100-dev (2014-Jul-29) including media_wysiwyg sub module
* CKEditor (module) 1.15 with the patch from #68
* CKEditor library 4.4.3 (15 Jul 2014) - full package

active text filters (weighted order):

* filter_html
* media_filter
* invisimail_encode_html_entities (from Invisimail module)
* filter_url

I can confirm, that it also works for me now! :-))

Without the patch from #68, it wouldn't work.

Only disadvantage, I've experienced: it seems, that the patch breaks the integration with CKEditor LInk module - which sounds similar to the problem that tanc described with linkit integration.

Angry Dan’s picture

We should probably do some brief discovery on that linkit/link module bug, to see what the problem is and how easy it is to fix, but given that the current state of the wysiwyg/media plugin is that it's essentially broken I think we should move towards a commit sooner rather than later?

I don't know if this module has a release schedule but assuming that the change won't go out as a stable release immediately, having the -dev exposure or creating a beta release will help us unearth any of the (inevitable) issues that such a large patch is likely to throw up?

tanc’s picture

Sounds extremely sensible to me. I'll try and look at the linkit issue early next week as its part of our company install profile so would be useful to fix.

agoradesign’s picture

I know the problem, and I have found a solution!

It seems, that the patch not only addresses the media plugin integration, but also tries to clean up generally the way, how plugins are loaded. Before, the ckeditor_load_plugins() function was 292 lines long: first, several plugins were defined on behalf of other modules, e.g. CKEditor Link and LinkIt. Later on, the 'ckeditor_plugin' hook was called, giving other modules the chance to define a CKE plugin. These hook implementations were merged with the array of plugins defined before.

It was a good idea, that this patch reduced complexity dramatically. The ckeditor_load_plugins() function is now only 13 lines long! Because all the plugins that CKE defines on behalf of other modules, are now being done in a self-implementation of the 'ckeditor_plugin' hook (function ckeditor_ckeditor_plugin() in the new ckeditor.ckeditor.inc file). This is best practice, like it should be done, and practiced by many other modules. All this implementation stuff was just moved to another place.

Normally, this shouldn't cause any problems. But the devil is in the detail: first of all, CKEditor Link already implements this hook by itself. But the way, how module_invoke_all() handles the duplicate plugins is different from an array_merge() call: you would have arrays for name and path instead of a single string now. That caused problem number one.

If we would just throw out CKEditor's on-behalf implementation, we would run into the next problem: the "default" key is not set (not sure, if it's needed or not), and the path is wrong. CKEditor would treat it relative to the library path. That's why, it prepends the path with '%base_path%' in its on-behalf implementations. As far as I have seen in the Code repository of LinkIt, we have exactly the same situation over there.

As I have to leave in a couple of minutes, I can't provide a patch until tomorrow, but I leave you a detailed solution here:

1. Go to ckeditor.ckeditor.inc file and drop lines 32 to 55 from ckeditor_ckeditor_plugin() function
2. In the same file, go to the end of the ckeditor_ckeditor_plugin_alter() function and add the following lines:

  if (isset($plugins['ckeditor_link'])) {
    $plugins['ckeditor_link']['default'] = 't';
    $plugins['ckeditor_link']['path'] = '%base_path%' . $plugins['ckeditor_link']['path'];
  }
  if (isset($plugins['linkit'])) {
    $plugins['linkit']['default'] = 't';
    $plugins['linkit']['path'] = '%base_path%' . $plugins['linkit']['path'];
  }

Step 2 is only a workaround, we should contact both maintainers to adapt their hook implementations, so that we don't have to do the code alteration.

Have a nice evening!

agoradesign’s picture

I've updated Angry Dan's patch from #68 and moved all the stuff relating external plugins from other modules (ckeditor_link, linkit and ckeditor_swf) from hook_ckeditor_plugin() to hook_ckeditor_plugin_alter(), adding 'default' key and adapting the path.

I've done some quick tests. Media integration still works, CKEditor Link also works for me. However, I did neither try ckeditor_swf nor linkit. But normally there shouldn't be any problems.

I totally agree with Angry Dan, that we should get this committed asap. The media integration is very important. Additionally, cleaning up the whole plugin system is essential! And if this clean new approach to the plugin system has some unwanted side effects on existing plugins of other modules, than we have to accept that consequences and open up a new issue, or e.g. discuss it in this one: External plugin declarations are redundant. (Linkit, ckeditor_link, ckeditor_swf not working) (which hopefully has become obsolete now)

Status: Needs review » Needs work

The last submitted patch, 75: ckeditor-accomodate-latest-media-changes-75.patch, failed testing.

rooby’s picture

I'm also plus one for committing this.

I've been using it for a while on a few sites and it seems to be fine (100% better than without the patch) and it would be good for people to not have to keep rerolling this all the time.

Angry Dan’s picture

@agoradesign if you could provide an interdiff for your patch it would help us review your changes, it's easier than it looks and saves time when we're browsing the queue - https://www.drupal.org/documentation/git/interdiff.

If we're agreed that we should be moving towards commit, then let's push ahead now for RTBC. I'd love to test the patch in #78 myself but I'm going to be away from Drupal until the end of August.

Are there any active module commiters listening in on this issue? It's really important that we get your backing on how close you think this is to commit, even if the answer is "nowhere near".

Thanks all,
Dan

jcisio’s picture

Are there any active module commiters listening in on this issue? It's really important that we get your backing on how close you think this is to commit, even if the answer is "nowhere near".

I'm actively following this issue, and even gave feedback a few times. However, I don't have a consistent feedback: many people report error or incompatibility. Just a few "it works for me" without neither review nor modules that you are using (that relate to this patch i.e. the modules that provide plugins). Nor without saying if the export features changes. Nor without saying if the admin UI changes.

This issue is to clean up (rearrangement of plugins). Please review and RTBC. I haven't seen anyone doing it. The issue is always "needs work". The patch is not adjusted to take into account of comments (like #54, #69, #70).

PS: I haven't reviewed the patch since a while. I'll do that when the patch is RTBC and I have some free time.

Angry Dan’s picture

Status: Needs work » Needs review

Thanks for your comments jcisio. Can somebody please update the issue summary with a current status report (there's a template somewhere isn't there?) so that new visitors are able to help with the testing?

agoradesign’s picture

Here's the interdiff, plus I've uploaded the patch again, hopefully satisfy the testbot now (binary patch)

heddn’s picture

Added tag to update issue summary. Its really not clear what this issue is going to fix or what functionality it will provide.

bkosborne’s picture

Can someone more familiar with this issue update the issue summary to reflect what's being changed? Right now the title of the issue and the summary don't really match.

I noticed a few issues with the CKEditor integration, but they are likely unrelated to this issue:
- Entering alt or title text for an existing image doesn't actually do anything - the data isn't put into the media token
- When re-opening the media dialog for an image that was already embedded, the previously selected view mode is no longer selected and must be reselected. It always goes back to "Default" for me when I had selected a different one when embedding it.

saltednut’s picture

Status: Needs review » Reviewed & tested by the community

I have tested #81 extensively and it is working correctly. Here is the recipe I used to test.

api = 2
core = 7.x
projects[drupal][type] = core
projects[drupal][version] = 7.31
projects[ckeditor][version] = "1.x-dev"
projects[ckeditor][type] = "module"
projects[ckeditor][subdir] = "contrib"
projects[ckeditor][download][type] = "git"
projects[ckeditor][download][revision] = "b0de255"
projects[ckeditor][download][branch] = "7.x-1.x"
projects[ckeditor][patch][2159403] = "http://drupal.org/files/issues/ckeditor-accomodate-latest-media-changes-81.patch"

projects[media][version] = "2.x-dev"
projects[media][type] = "module"
projects[media][subdir] = "contrib"
projects[media][download][type] = "git"
projects[media][download][revision] = "b2ba2da"
projects[media][download][branch] = "7.x-2.x"
projects[media][patch][2272567] = "http://www.drupal.org/files/issues/media-dialog-zindex-2272567-1.patch"

projects[jquery_update][version] = "2.4"
projects[jquery_update][type] = "module"
projects[jquery_update][subdir] = "contrib"

libraries[ckeditor][download][type] = "get"
libraries[ckeditor][download][url] = "http://download.cksource.com/CKEditor%20for%20Drupal/edit/ckeditor_4.4.3_edit.zip"

CKEditor plugin config: http://cgit.drupalcode.org/curate/tree/curate.features.ckeditor_profile....

I think we should move forward here. There is always room to follow up with meta issues if there are lingering problems. This is a big step forward for Drupal in the media space where CKEditor integration is largely accepted as patently broken.

micbar’s picture

The goal of the patch in #81 is to move the plugin definitions to a new file ckeditor.ckeditor.inc and to document the new hook_ckeditor_plugins_alter in ckeditor.api.php.
Additionally, this patch removes the old media plugin which seems to be a duplicate of #2140155: Remove media plugin. Media handling has been transferred to media_wysiwyg module a long time ago in #1504696: Integration with CKEditor module.

I set this to needs work because the patch misses some new plugins. it removes

-      $arr['codesnippet'] = array(
-        'name' => 'codesnippet',
-        'desc' => t('Plugin for inserting Code Snippets. See !link for more details. See !help for additional instructions.', array(
-            '!link' => l(t('addon page'), 'http://ckeditor.com/addon/codesnippet'),
-            '!help' => l(t('help'), 'admin/help/ckeditor', array('fragment' => 'codesnippet'))
-          )),
-        'path' => $_editor_url . '/plugins/codesnippet/',
-        'buttons' => array(
-          'CodeSnippet' => array(
-            'icon' => 'icons/codesnippet.png',
-            'label' => 'Insert Code Snippet',
-          )
-        ),
-        'default' => 'f'
-      );
-      $arr['mathjax'] = array(
-        'name' => 'mathjax',
-        'desc' => t('Plugin for inserting Mathematical Formula (MathJax). See !link for more details. See !help for additional instructions.', array(
-          '!link' => l(t('addon page'), 'http://ckeditor.com/addon/mathjax'),
-          '!help' => l(t('help'), 'admin/help/ckeditor', array('fragment' => 'mathjax'))
-        )),
-        'path' => $_editor_url . '/plugins/mathjax/',
-        'buttons' => array(
-          'Mathjax' => array(
-            'icon' => 'icons/mathjax.png',
-            'label' => 'Insert Mathematical Formulas',
-          )
-        ),
-        'default' => 'f'
-      );
-    }
-  }
-

from ckeditor.lib.inc
and doesn't add it to ckeditor.ckeditor.inc.

I try to provide an updated patch soon.

micbar’s picture

Status: Reviewed & tested by the community » Needs work
saltednut’s picture

Status: Needs work » Reviewed & tested by the community

One goal of the patch is to more easily enable other modules to add their own plugins. However, CKEditor also provides its own plugins. We should remove any plugins that are module dependent but keep the ones that are internal.

saltednut’s picture

Status: Reviewed & tested by the community » Needs work
micbar’s picture

Status: Needs work » Needs review
FileSize
38.62 KB
2.83 KB

I made the suggested changes from #85. See interdiff.txt

Status: Needs review » Needs work

The last submitted patch, 89: make_ckeditor_plugin-2159403-89.patch, failed testing.

micbar’s picture

Status: Needs work » Needs review
FileSize
38.84 KB

My fault, forgot the --binary option.

micbar’s picture

Could somebody review this patch, that we can move further with the follow-ups?
It has already been on RTBC (see #84). I made minor fixes in #89. See interdiff.

MediaFormat’s picture

wwalc’s picture

Thanks to all of you for working hard on this issue.

@micbar - I saw your previous comment in #2159403-85: Make CKEditor plugin system modular and clean where you noticed missing CKEditor plugins, this was one of the reappearing problems and I was happy to see that you did not miss it.

I'll thoroughly check your patch next week, in the meantime if anyone wants to add any feedback regarding the latest patch, for example "seems to work" if you are affraid of saying that "it's perfect" and RTBC, then please feel free to do so.

tanc’s picture

I've tested #91 and am happily using it on a development site at the moment. Will report back if I notice anything unusual.

jcisio’s picture

We needs someone using modules like Linkit, CKEditor Link, Media (1.x) to report what happens before/after the patch, for which version of each module. Removing of the integration with these modules is out of scope of this issue.

Ideally, the patch should only move thing around and add examples. We aren't "fixing" anything here, other than make the plugin system extensible.

rooby’s picture

I use linkit with media 2.x and have had no problems.

ckeditor link and media 1.x I haven't tried.

wwalc’s picture

Alright, that was fun! Congrats to all of you, we're close to RTBC, there are just some tiny details to be cleaned up.
At the same time apologies for the delay, I was hoping that reviewing will take me less time.

Plugin paths

1) I did check the patch together with a couple of available modules that integrate with CKEditor using hook_ckeditor_plugin():

2) I tested CKEditor loaded from sites/all/libraries and from CDN.

It turned out that the "hack" in ckeditor_ckeditor_plugin_alter() for three plugins which were previously hardcoded is wrong, because it targets just these three plugins while all the external modules turned out to be broken. This is not surprising, because the old code did the following:

$plugins = module_invoke_all('ckeditor_plugin');
foreach ($plugins as $i => $plugin) {
...
  $plugins[$i]['path'] = $base_path . $plugin['path'];
...
}

So the old code assumed that $base_path isn't provided, while the new one suggests doing it. I'm quite fine with that, but to maintain backwards compatibility we still have to add base_path() to plugins registered in the "old way". I did this by checking whether the path starts with "%" (indicates a placeholder like %base_path%), http://, https://, // or "/" - in all other cases I still append base_path(). Hence the new patch.

ckeditor_update_7006

I have doubts regarding this code in ckeditor.install. The old plugin should be removed from profiles automatically - I agree, but can we really assume that anyone who had the old plugin enabled can instantly enable media_wysiwyg module which is available only in version 2.x (alpha)?
For me such suggestion should be available in a changelog / release notes.

I did keep this in the patch, but would appreciate any additional feedback on this.

Notes (ignore)

In case I was wondering some day about why this part of code was gone, there were at least 4 reasons for this:

  foreach ($plugins as $i => $plugin) {
    // *** Reason #1 paths are now URLs, does not make sense.
    if (file_exists($plugin['path'] . 'plugin.js')) {
      $source = file_get_contents($plugin['path'] . 'plugin.js');
      $plugins[$i]['path'] = $base_path . $plugin['path'];
      // *** Reason #2 hooks should return buttons, if the module did not register any, do not look for them!!!
      if (!isset($plugin['buttons']) || count($plugin['buttons']) == 0) {
        $buttons = array();
        if (preg_match_all($pattern, $source, $matches)) {
          foreach ($matches[1] as $j => $button_name) {
            // *** Reason #3 even if you look for buttons, you may fail as to register a button the icon property is not needed even if it's used!
            if (preg_match('#(icon)[\s]*\:[\s]*([^\,\n]*)#', $matches[2][$j], $matches2)) {
              $buttons[$button_name] = array();
              $buttons[$button_name]['label'] = $button_name;
              $matches2[2] = str_replace(array('this.path', '+', '\'', '"'), array('', '', '', ''), $matches2[2]);
              $buttons[$button_name]['icon'] = trim($matches2[2]);
            }
          }
        }
        $plugins[$i]['buttons'] = (count($buttons) > 0) ? $buttons : FALSE;
      }
    }
    // *** Reason #4 if a module specified a plugin file that did not exists, blame the module and do not try to fix the situation.
    else {
      unset($plugins[$i]);
    }
  }

TL;DR

The attached patch fixes resolving paths for external modules. I'm pretty convinced that it works, but a double check would not hurt.
I'm for modifying ckeditor_update_7006() because of the things I wrote above, but maybe I'm missing something.

Status: Needs review » Needs work

The last submitted patch, 99: ckeditor_plugin_system_cleanup-2159403-99.patch, failed testing.

wwalc’s picture

Another attempt, with --binary this time ;-)

wwalc’s picture

Status: Needs work » Needs review
rooby’s picture

In relation to ckeditor_update_7006, if you want to go the message way there could be an update function that displays a message to the user on screen also.

rooby’s picture

Or we could also detect their version of the media module and act accordingly.

One thing I'm not sure about with that approach would be is it possible a person upgrades from media 1.x to 2.x at the same time as this update and we detect media 1.x before it has been updated to 2.x and then it updates to 2.x.

I can't remember exactly how the upgrade process runs so I'm not sure if that kind of thing would be an issue.

Plus that's getting pretty complicated and probably doesn't need to be.

joelpittet’s picture

Minor nitpicks, I don't actually follow what this issue is doing... but looks like it's cleaning things up so good on you:)

  1. +++ b/ckeditor.ckeditor.inc
    @@ -0,0 +1,232 @@
    +  /*
    +   * CKEditor build-in plugins
    +   */
    ...
    +  /*
    +   * CKEditor module plugins
    +   */
    ...
    +  /*
    +   * CKEditor module plugins - additional directory
    +   */
    

    Should be inline comments for coding standards.

  2. +++ b/ckeditor.ckeditor.inc
    @@ -0,0 +1,232 @@
    +  //die($editor_path);
    

    Remove debug code.

  3. +++ b/ckeditor.ckeditor.inc
    @@ -0,0 +1,232 @@
    +  //remove page break button if there is no module to do this
    

    Space, Captital, period.

dasjo’s picture

wwalc’s picture

The updated patch contains the following changes comparing to ckeditor_plugin_system_cleanup-2159403-100.patch:

  • Corrected typos etc. (and confusing description of an old mediaembed plugin, which has nothing to do with the Drupal media module).
  • ckeditor_update_7006 no longer throws an exception when running update. When media plugin was enabled in at least one CKEditor profile and media_wyswyg is not available, plugin is disabled automatically and a message is shown to the user with further instructions.

Any feedback is welcome!

I'd love to commit this soon to have more people testing it. After having the security release published last week, finally we can go ahead and fill the dev version with changes like this one, that were waiting for a while.

deggertsen’s picture

Status: Needs review » Reviewed & tested by the community

Just spent some good time testing and so far I haven't run into any problems! So excited to have this finally fixed!

marcusx’s picture

It works fine with images. But can someone confirm it works with PDFs / Documents? See: #2364559: Link / Icon to media document is stripped out by ckeditor before save or when switching to markup view

Devin Carlson’s picture

@marcusx that issue is on the Media side.

+1 on RTBC and committing #107. I haven't run into any issues with it during my testing as well.

partdigital’s picture

I can confirm that this works with Media and Media Youtube as well.

marcusx’s picture

@devin-carlson: Really? Which issue is that?

I wonder because I have content with media tokens added with an older version / combination of media / ckeditor.

e.g.

[[{"fid":"160","view_mode":"teaser","fields":{"format":"teaser"},"type":"media","link_text":"Detailed Docs","attributes":{"class":"file media-element file-teaser"}}]]

This media token is correctly handled as link with PDF icon no matter what I do. I can switch to "Source" in ckeditor. Or change to "Plain Text". It is not breaking. But when adding a new PDF from the media library I don't get the correct stuff embedded.

BrightBold’s picture

With the patch in #107 and the "Plugin for inserting embeded [sic] media" checked, my editor window failed to load - I just get a white area the size of the editor window, and the page loads scrolled to the text format details below the editor window. I reproduced this in both Chrome and Firefox under the following conditions:

  • CKEditor 7.x-1.16 with patch in #107
  • Latest dev of Media (or Media 7.x-2.0-alpha4, which I was originally using)
  • Latest dev of File Entity (or 7.x-2.0-beta1, which I tried first)
  • ACF is disabled
  • Plugin to count symbols is checked
  • Plugin for inserting imaged from Drupal media is checked
  • Support responsive images with the Picture module is checked

If I either uncheck the embedded media plugin or roll back this patch, the editor appears normally. I found this issue but it's for the WYSIWYG Media Embed module (which I'm not using).

I didn't want to set this back to needs work without asking if there's a configuration issue I should fix or whether this issue is really related to something else. If nothing else I'll save someone else hours trying to figure out why their editor window disappeared.

wwalc’s picture

@BrightBold - could you provide more details, for example all the JavaScript exceptions in the error console?
Make sure to clear the browser cache and perhaps check two different browsers.

saltednut’s picture

FileSize
27.07 KB
65.9 KB

@BrightBold - Media is currently adding an extra checkbox in the settings. Maybe that is where your problem lies?

A couple screenshots of what I have working in D7 with Media 2.x-dev, File Entity 2.x-dev, CKEditor HEAD (w/ patch).

BrightBold’s picture

@brantwynn - Yes, it definitely turned out to be an interaction between that extra checkbox, "Plugin for inserting embeded media" and this patch. Either one by itself was OK, but both together messed up the script.

@wwalc - I will redo my testing and provide JavaScript console info in the next few days.

saltednut’s picture

@BrightBold brings up a good point here. Should Media be providing the checkbox or should CKEditor? I believe right now they are both providing a checkbox.

BrightBold’s picture

@brantwynn From an architectural perspective I don't know which module should provide the link, but I will point out that the way they're written now, it's not clear that they perform the same function. I assumed that "plugin for inserting images" only inserted images, and that I needed the embedded media plugin to get video working, which was the problem I was trying to fix.

So the wording of "embedded media" is better since it more accurately reflects the button's function. But if that wording wins, we should be sure to spell "embedded" properly.

wwalc’s picture

"Plugin for inserting embeded media" has nothing to do with the Media module. It is a separate plugin which allows you to paste <iframe> from youtube services etc. It has been in CKEditor module for ages and it's really... "basic" let's say. I would not introduce it today, but considering that some people might be using it it's better to leave it and just discourage from using it.

Words "Embedding" and "Media" have such a broad meaning that indeed the naming is a bit confusing.

Let's just wait for BrightBold to confirm that the bug was indeed caused by conflict with ""Plugin for inserting embeded media" (or by that plugin being just broken) and we're ready to go. The issue can be then handled in a separate ticket (as it touches an unrelated plugin), in which we could also decide about marking that separate plugin as "deprecated" or sth so that people did not enable it accidentally.

saltednut’s picture

A fast grep (ack) for 'embeded' turns up /ckeditor/plugins/mediaembed/plugin.js

I am unsure if its in the scope of this patch to remove the 'embeded' media plugin within CKEditor module, but it definitely appears that we'd want the module providing the plugin (e.g. Media) to be the one providing the button.

wwalc’s picture

@BrightBold - if you confirm that the issue is caused by the plugin with the typo in its name ("Plugin for inserting embeded media"), please post the errors from the developer console anyway so that I could investigate this bug.

saltednut’s picture

Cross posted with #119 - thanks for the clarification @wwalc

rooby’s picture

I think removing the mediaembed plugin is outside the scope of this issue.

BrightBold’s picture

@brantwynn reminded me I'm holding up this issue - so sorry! I got caught up with a site launch. I will do some more testing this evening to provide the requested JavaScript exceptions. Is there any other information that I can give you at the same time that will be helpful?

BrightBold’s picture

Aargh. Well now I can't reproduce it. So either it was a conflict with something in Media alpha4 or File Entity beta1 and I didn't clear the cache when I updated those to the dev versions, or it was a conflict with a Media patch that I later rolled back. So you should go ahead with this. Sorry to hold it up and then not be able to figure out what I changed to make it irreproducible.

saltednut’s picture

@BrightBold Thanks for looking into it!

@wwalc is there anything else we need to do here?

jcisio’s picture

I tested patch 107 with Features export: global + advanced profiles (plugins: auto grow, code snippets, Scald dnd, Drupal teaser, MathJax, embeded media). The export before and after the patch were identical.

heyyo’s picture

Not sure what I'm doing wrong.
I could successfully input a youtube or vimeo inside the field "File URL or media resource *" which is accepting oembed provider.
After submitting my video, I could see it in Ckeditor correctly but as soon as I save the node or even if Switch to plain text editor, the embeded code is not there anymore.

PS: regular images works nicely.

I have the following installed:
file_entity-7.x-2.0-beta1
media-7.x-2.0-alpha4
media_oembed-7.x-2.2
ckeditor-7.x-1.16 + patch#107
ckeditor library: //cdn.ckeditor.com/4.4.3/full-all
drupal 7.34

Enabled:
ckeditor
file + image
media_internet
media_wysiwyg
media
media_oembed

Input filter in this order:

  1. Convert URLs into links
  2. Convert line breaks into HTML
  3. Convert Media tags to markup

CKeditor
Plugin for inserting images from Drupal media module is checked and in the toolbar
Advanced content filter is disabled

jcisio’s picture

Status: Reviewed & tested by the community » Needs work

The patch has been RTBC since more than a month so I was going to commit it. But when I look at it, the media plugin is removed. What happens? Does it mean that CKEditor does suddenly not support Media 1.x? Or if the plugin is broken, then since which version of Media 1.x?

I think that plugin should be kept (it has nothing to do with making the plugin system modular and clean). And if we don't want it in Media 2.x, that module implements hook_ckeditor_plugin_alter() to replace it with its own version.

jcisio’s picture

BTW the issue summary needs update. We have tags, but I think a comment is necessary.

heyyo’s picture

@jcisio any idea on my issue #128 ?

jcisio’s picture

#131 It's out of scope. Only thing that works before the patch and does not work after is in the scope of a refactoring.

msmithcti’s picture

For those (@heyyo and @marcusx) concerned about how this works with media other than images, I found I was running into the issue described in #2364559: Link / Icon to media document is stripped out by ckeditor before save or when switching to markup view and have just posted a patch that fixes the issues for me, alongside the patch from this issue.

Devin Carlson’s picture

Status: Needs work » Needs review
FileSize
17.28 KB
22.69 KB

An updated version of #107 which:

  • Keeps the Media plugin
  • Removes the update function since the plugin is still available
  • Removes all changes made to ckeditor_profile_settings_compile(). An single additional check was added to verify that the JS is only added when Media 1.x is installed (Media 1.x implements the WYSIWYG text filter itself while Media 2.x moves the filter into the Media: WYSIWYG submodule).

This leaves altering/removing the default plugin up to the Media module.

saltednut’s picture

Status: Needs review » Needs work

With the standard profile, I am able to load and configure CKEditor and its profiles correctly for Media but I see this JS error when I try to use the Media button:

Uncaught TypeError: Cannot read property 'invoke' of undefined

Within my install profile (Lightning) I am able to use CKEditor correctly, but I see a number of errors during install and also when I visit the CKEditor profiles pages.

strpos() expects parameter 1 to be string, array given ckeditor.ckeditor.inc:227                                                                                                                                                   
preg_match() expects parameter 2 to be string, array given ckeditor.ckeditor.inc:227
saltednut’s picture

Status: Needs work » Needs review

My apologies: It appears Media integration is now out of scope for this patch. Safely ignore #135 as it will be addressed in a followup in the Media module's queue.

saltednut’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
168.32 KB

Testing #134 inside the standard profile with CKEditor module shows all standard buttons operating correctly. See screenshot.

agoradesign’s picture

Status: Reviewed & tested by the community » Needs work

Sorry, I've to revert the status, The patch from #134 fully broke a site, where #107 works.

After experiencing some loading problems with CKE on a couple of sites, when having more than one CKE-enabled field, I decided to give the latest dev versions of File Entity, Media, CKEditor including the updated patch from #134 a try. I've tried to use it on a site that already had the patch from #107 applied.

First, I've experienced a couple of "Array to string conversion in ckeditor_ckeditor_plugin_alter()" errors, because both CKEditor and Media WYSIWYG register a "media" plugin. Because of that, the media plugin definition contains an array as path, not a string. Ok, this could be avoided with a simple if condition. But the next and bigger problem is, that Media WYSIWYG also implements the ckeditor_plugin_alter hook and runs after CKE. There the path is again modified. So the JS files provided by CKEditor module never get called. And therefore the editor won't load because of a "Drupal.media is undefined" JS error.

Is there a special reason for the re-inclusion of Media plugin in #134? Because in this case brantwynn's comment in #136 is wrong and Media integration is back in scope.

jcisio’s picture

#138 This issue does not fix anything, it just reorganizes the plugin system. The media plugin is back, because it has been always there. But you are correct about the plugin alter hook. CKEditor should not use that hook and move the unset condition into the plugin hook itself, so that Media module can easily alter it.

agoradesign’s picture

But even if Media alters it, it goes wrong (the JS error I described). I'm not sure, but I think it is either because Media does alter the path and override CKEditor's Media plugin path, or the fact, that these three lines were dropped off between the patches:

      media_wysiwyg_include_browser_js();
      drupal_add_js(drupal_get_path('module', 'media_wysiwyg') . '/js/media_wysiwyg.filter.js');
      drupal_add_js(drupal_get_path('module', 'media_wysiwyg') . '/wysiwyg_plugins/media_ckeditor/library.js', array('scope' => 'footer', 'weight' => -20));

If yes, what must Media WYSIWYG do, that these files get loaded and everything works as expected?

Devin Carlson’s picture

Status: Needs work » Needs review
FileSize
22.75 KB
2.71 KB

An updated patch which moves the code out of ckeditor_ckeditor_plugin_alter() and into ckeditor_ckeditor_plugin() (except for the compatibility shim which should exist in ckeditor_load_plugins()).

I also added a few extra code comments.

agoradesign’s picture

Thanks for the updated patch. Looks good - no more PHP errors, and with latest Media dev also no more JS errors nor problems

saltednut’s picture

Status: Needs review » Reviewed & tested by the community

#141 is working great for fresh installs of our distribution and even fixed #2331293: Quickedit module compatibility on my end. I'd say this is RTBC as long as @jcisio et al are happy with it.

DamienMcKenna’s picture

daften’s picture

For me the patch doesn't work properly. By applying the patch and trying to use the media plugin, clicking the button gives the following error:

Uncaught TypeError: Cannot read property 'invoke' of undefined  (modules/contrib/ckeditor/plugins/media/plugin.js?t=E6FC:37)

I know that the media plugin packaged is deprecated and should be replaced by enabling the media_wysiwyg module, but when I do that, the following errors pop up on the configuration page for the ckeditor profile:

Warning: strpos() expects parameter 1 to be string, array given in ckeditor_load_plugins() (line 338 of /Users/daften/Workspaces/Freelance/Palermo/www/profiles/palermo/modules/contrib/ckeditor/includes/ckeditor.lib.inc).
Warning: preg_match() expects parameter 2 to be string, array given in ckeditor_load_plugins() (line 338 of /Users/daften/Workspaces/Freelance/Palermo/www/profiles/palermo/modules/contrib/ckeditor/includes/ckeditor.lib.inc).
Notice: Array to string conversion in ckeditor_load_plugins() (line 339 of /Users/daften/Workspaces/Freelance/Palermo/www/profiles/palermo/modules/contrib/ckeditor/includes/ckeditor.lib.inc).

This seems to be because the plugin with the same name is declared twice (once in ckeditor itself, once in media_wysiwyg). I don't see any way to fix this problem from the media_wysiwyg module's end.

aDarkling’s picture

+1
#141 made ckeditor actually useable for my purposes now! at
Now that media tags are being generated, I'm working out ckEditor/Media/Picture intergration.
That shouldn't stop this from getting commited, though. The issue queue's already kinda congested.
This patch also covers #2292575: "Naming of mediaembed plugin is confusing".

hass’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/ckeditor.ckeditor.inc
    @@ -0,0 +1,222 @@
    +        'desc' => t('Table Resize plugin. See !link for more details.', array(
    +          '!link' => l(t('addon page'), 'http://ckeditor.com/addon/tableresize')
    +        )),
    

    This is incorrect way to add links into translatable string as it is no context sensitive.

  2. +++ b/ckeditor.ckeditor.inc
    @@ -0,0 +1,222 @@
    +        'desc' => t('Auto Grow plugin. See !link for more details.', array(
    +          '!link' => l(t('addon page'), 'http://ckeditor.com/addon/autogrow')
    +        )),
    

    Same

  3. +++ b/ckeditor.ckeditor.inc
    @@ -0,0 +1,222 @@
    +        'desc' => t('Stylesheet Parser plugin. See !link for more details.', array(
    +          '!link' => l(t('addon page'), 'http://ckeditor.com/addon/stylesheetparser')
    +        )),
    

    Same

  4. +++ b/ckeditor.ckeditor.inc
    @@ -0,0 +1,222 @@
    +        'desc' => t('Plugin for inserting Code Snippets. See !link for more details. See !help for additional instructions.', array(
    +          '!link' => l(t('addon page'), 'http://ckeditor.com/addon/codesnippet'),
    +          '!help' => l(t('help'), 'admin/help/ckeditor', array('fragment' => 'codesnippet'))
    +        )),
    

    Same

  5. +++ b/ckeditor.ckeditor.inc
    @@ -0,0 +1,222 @@
    +            'label' => 'Insert Code Snippet',
    

    Translatable string?

  6. +++ b/ckeditor.ckeditor.inc
    @@ -0,0 +1,222 @@
    +        'desc' => t('Plugin for inserting Mathematical Formula (MathJax). See !link for more details. See !help for additional instructions.', array(
    +          '!link' => l(t('addon page'), 'http://ckeditor.com/addon/mathjax'),
    +          '!help' => l(t('help'), 'admin/help/ckeditor', array('fragment' => 'mathjax'))
    +        )),
    

    Same

  7. +++ b/ckeditor.ckeditor.inc
    @@ -0,0 +1,222 @@
    +            'label' => 'Insert Mathematical Formulas',
    

    Translatable string?

  8. +++ b/ckeditor.ckeditor.inc
    @@ -0,0 +1,222 @@
    +            'desc' => t($matches[1]),
    

    What are we translating here? This can be a security issue.

  9. +++ b/ckeditor.ckeditor.inc
    @@ -0,0 +1,222 @@
    +            'desc' => t('Plugin file: ' . $file),
    

    Translatable string bug. Requires placeholders.

This is far away from being RTBC.

jcisio’s picture

@hass those are (or mostly are) just code moving around, so I guess we can live with that, and fix them in follow-ups. This issue has been hold for too long.

If there is no more functional problem, I'll try to review and commit it this weekend.

hass’s picture

These should be fixed while we are already on this buggy code. It's not really difficult to fix.

hass’s picture

These should be fixed while we are already on this buggy code. It's not really difficult to fix.

daften’s picture

The latest version of media fixes the issue, this patch works perfectly for me now :)

dougn7’s picture

Hi everyone. Im apply this patches on dev versions on Media (2.x) and CKEditor and still the last submit button doesnt work. Got some Hunk messages on Git, but the rest is ok. I put "config.allowedContent = true;" on both CKEditor profiles and such. Any help?

aDarkling’s picture

Priority: Normal » Major
Status: Needs work » Needs review
FileSize
14.32 KB

Regarding comments in #147
1-4. Fixed
5. I Agree. Fixed.
6. Fixed
7. I Agree. Fixed.
8. You're right. Unless someone wants to go through the entire ckeditor/addons listing and pull out every description, t()'ing this would be a waste of time. Changed it to run through check_plain() so there won't be any security issues.
9. This should be translated to accommodate the right-to-left folks as well, so I just changed it to use a placeholder.

Included is the updated patch. I also added in support for the Image2 plugin that's required if you want to use ckeditor's captioning widget. See? Feature Bloat. Let's get this reviewed & out the door before it happens again! :)

jcisio’s picture

Status: Needs review » Needs work

1. Please post interdiff. The attached patch misses a file.

2. We should be careful with the coding standard corrections that modify strings. All modified strings will become untranslated in non English websites. They are mostly in the back office, so it could be acceptable, but needs review if we want to change them.

hass’s picture

Translatable trings that are already broken cannot break more. Everyone is running l10n_update and will get the new translations asap.

jcisio’s picture

#155 only if someone translated all new strings first...

We don't follow coding standard just for the sake of coding standard, if it breaks current websites for no good reasons.

daften’s picture

#156 Yes, current websites might have English instead of a localized string, that is no reason to not follow coding standards which are there for many reasons, security as the main one.
What I'd propose is to either fix this here, or to create a follow-up issue with a patch which fixes this. But IMO it needs to be fixed at some point.

jcisio’s picture

Sure, but it depends how they affect security. I can't say anything until I see the patch, which was missing in #153.

bneil’s picture

Here's an interdiff between the patch in 141 and the one in 153.

hass’s picture

I have one question about this plugin hook. Is it possible for a theme to define the hook and implement per theme plugins? e.g. image2 is a very good candidate for this. Some themes may be compatible with image2 where others are not.

aDarkling’s picture

Status: Needs work » Needs review
FileSize
23.67 KB
6.47 KB

Cripes! So embarrassing!

Ignore #153. I forgot to git-add the only file that I worked on!
Here's a complete patch & associated interdiff.

Status: Needs review » Needs work

The last submitted patch, 161: make_ckeditor_plugin-2159403-161.patch, failed testing.

hass’s picture

  1. +++ b/ckeditor.ckeditor.inc
    @@ -55,61 +55,78 @@ function ckeditor_ckeditor_plugin() {
    +        'desc' => t('Table Resize plugin. See  <a href="@ckdocs-tableresize">addon page</a> for more details.', ¶
    ...
    +        'desc' => t('Auto Grow plugin. See  <a href="@ckdocs-autogrow">addon page</a> for more details.', ¶
    

    Two spaces and a trailing space?

  2. +++ b/ckeditor.ckeditor.inc
    @@ -55,61 +55,78 @@ function ckeditor_ckeditor_plugin() {
    +        'desc' => t('Stylesheet Parser plugin. See <a href="@ckdocs-stylesheetparser">addon page</a> for more details.', ¶
    

    trailing space?

  3. +++ b/ckeditor.ckeditor.inc
    @@ -55,61 +55,78 @@ function ckeditor_ckeditor_plugin() {
    +            '@ckdocs-codesnippet' => 'http://ckeditor.com/addon/codesnippet', ¶
    +            '@ckdocs-codesnippet-help' => url('admin/help/ckeditor', array('fragment' => 'codesnippet')), ¶
    

    trailing space?

aDarkling’s picture

Status: Needs work » Needs review
FileSize
23.67 KB
6.46 KB

Sorry. Didn't know trailing spaces was a thing.
Here's one that works. Reverted & re-patched locally to be sure.

hass’s picture

Status: Needs review » Needs work

Still two spaces in several strings.

aDarkling’s picture

Status: Needs work » Needs review
FileSize
23.66 KB
6.5 KB

OK, this should do it.
Should those other posts get deleted?

rooby’s picture

rooby’s picture

Oops, missed some.

hass’s picture

bneil’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Updated the issue summary based on variety of comments on this issue.

Devin Carlson’s picture

Status: Needs review » Reviewed & tested by the community

I think this is back to RTBC per #169.

izmeez’s picture

The patch in #169 applies cleanly. Not sure what is holding this patch back.

The media browser comes on and media can be selected. It initially appears correct in the wysiwyg editor as an icon and link but when it is saved it is saved as just text. This is an issue even without this patch. I'm still digging through the issues to find the correct issue for the problem. Didn't know if this patch was going to fix that bit too. [edit] Found #2177893: Custom wrapper breaks tokens with CKEditor >=4.0 without widget plugin includes patch that solves this problem. [/edit]

rooby’s picture

Status: Reviewed & tested by the community » Needs work

I hope I'm wrong about this because I've been waiting for this to be committed for ages and it makes me sad to have to change the status back again but...

It seems like this patch is the cause of this bug where I cannot use ckeditor styles to apply classes to images inserted using the media module: #2458939: CKEditor styles are not working with media

If it is confirmed a bug in this patch I can close that issue as cannot reproduce and it can be fixed here.

izmeez’s picture

The patch in #166 will need to be re-rolled as result of #2455391: Call to undefined function media_browser_js() latest Media-7.x-2.x-dev arising from #2454933: Remove JS helper functions

Patching file includes/ckeditor.lib.inc using Plan A...
Hunk #1 succeeded at 330.
Hunk #2 FAILED at 627.
1 out of 2 hunks FAILED -- saving rejects to file includes/ckeditor.lib.inc.rej

izmeez’s picture

The change is around line 630 of includes/ckeditor.lib.inc

    if (array_key_exists('media', $settings['loadPlugins']) && module_exists('media')) {
      drupal_add_library('media', 'media_browser');
      drupal_add_library('media', 'media_browser_settings');
      drupal_add_js(drupal_get_path('module', 'ckeditor') . '/plugins/media/library.js', array('scope' => 'footer', 'weight' => -20));

The line needs to be changed to
if (array_key_exists('media', $settings['loadPlugins']) && module_exists('media') && module_hook('media', 'filter_info')) {

After #2455391: Call to undefined function media_browser_js() latest Media-7.x-2.x-dev is committed.

rooby’s picture

@izmeez: It will only have to be rerolled if that patch is committed.
It seems to me that if this patch is committed that patch is no longer required, in whihc case this patch doesn't need a reroll.

aDarkling’s picture

@rooby, addressed your problem in https://www.drupal.org/node/2458939

As I mentioned there, there are a few other things that need to be addressed. However, there's no point in working on them while the basic functionality of Ckeditor->media_wysiwyg remains broken because this patch isn't committed.

rooby’s picture

Status: Needs work » Reviewed & tested by the community

I agree, setting status back and keeping that issue separate to befixed once this goes in.

jcisio’s picture

Category: Feature request » Task
Status: Reviewed & tested by the community » Fixed

Well... I check the patch a few times and do not find anything. It has been already tested a lot and recent feedback has been solid, so I decide to commit it, with some minor wording and coding standard fixes.

Thanks all for this yearlong issue.

  • jcisio committed 60a129c on 7.x-1.x authored by aDarkling
    Issue #2159403 by das-peter, aDarkling, Devin Carlson, wwalc, Angry Dan...
rooby’s picture

Yay!

Thanks to everyone that worked on this, now we're a step closer to an out of the box inline media solution :)

marcusx’s picture

Wooooha! Awesome that this finally got committed!

saltednut’s picture

Amazing! I was so busy working yesterday that I missed this going in... Great work everyone.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.