$meta['path'] value is expected to be a directory in wysiwyg.admin.inc:116, and file elsewhere

@ungeek - Thanks for reporting.

It seems to be a Wysiwyg API issue - there is an inconsistency between Wysiwyg API defining 'path' as pointing to editor_plugin.js file (since August 2009? see API doc patch in http://drupal.org/node/372826#comment-1903492) and icons code in wysiwyg.admin.inc line 116 expecting 'path' to be plugin's directory. Was this code missed in the API update?

function wysiwyg_profile_form($form_state, $profile) {
...
  foreach ($plugins as $name => $meta) {
    if (isset($meta['buttons']) && is_array($meta['buttons'])) {
      foreach ($meta['buttons'] as $button => $title) {
        $icon = '';
line 116:
        if (!empty($meta['path'])) {
          // @todo Button icon locations are different in editors, editor versions,
          //   and contrib/custom plugins (like Image Assist, f.e.).
          $img_src = $meta['path'] . "/images/$name.gif";
          // Handle plugins that have more than one button.
          if (!file_exists($img_src)) {
            $img_src = $meta['path'] . "/images/$button.gif";
          }
          $icon = file_exists($img_src) ? '<img src="' . base_path() . $img_src . '" title="' . $button . '" style="border: 1px solid grey; vertical-align: middle;" />' : '';
        }
...

Someone from Wysiwyg team - please confirm.

I think this is a typo in the docs. The editors which we have native plugin support for can accept either a path (folders only) or a path/filename.js when loading plugins. They default to appending the filename themselves ('pluginName.js', 'editor_plugin.js' or similar depending on their conventions) when given just a path, and at least all the plugins I know of follow these conventions. That's why the key is named 'path', and should be given a path without the filename.

(Note that "Drupal plugins" provided via the other plugin hook are a different matter.)

@TwoD - I would also assume this just from the 'path' name, but a simple test (removing filename from 'path') breaks otherwise working spellchecker plugin. I also tried using 'js path', 'js file' and nothing else seem to make it work, only 'path' => '<path>/editor_plugin.js'. So inconsistency is there, the question is where a fix is needed. If you decide to fix it so that 'path' becomes a path, not a filename, please let me know so I can update two plugin modules that I maintain.

I took another look at the code today and what I said above was spoken too soon. TinyMCE requires a the filename to be present, FCKeditor assumes it's 'fckplugin.js' and CKEditor defauls to 'plugin.js' but allows you to specify a different name using a separate parameter.

I think we should let path be just the path, and add the key filename which defaults to 'editor_plugin.js' for TinyMCE, 'fckplugin.js' for FCKeditor and nothing [internally] for CKEditor (which is effectively the same as defaulting to 'plugin.js').

As there's already several modules out there providing hook implementations for native TinyMCE plugins (all which must be ending path with '/editor_plugin.js') we should also check if the last part of the URL doesn't look like a folder. If it doesn't, use that part as the filename (overriding any definition of the filename key) and strip it from path

Thoughts?

Very good plan. Besides making perfect sense that would also resolve the original poster's issue.

A small patch to get this going.
It implements and somewhat documents what was outlined in #5 - but the filename setting overrides any filename set in path instead of the other way around.

Having the filename in path and leaving out filename still works for backwards compatibility with existing plugin wrappers for TinyMCE.
TinyMCE appears to be the only editor where using both path and filename in path was required - and worked - so that's the only one which I cared to do the extra string processing for.

The FCKeditor plugin examples I found never included the filename in the plugin path, and I don't think our implementation worked with it there either, so all this patch does there is to add a trailing slash to path for consistency with the FCKeditor examples and wysiwyg.api.php.

The CKEditor implementation already has support for filename so this was just adjusted to match the examples and docs, as was done for FCKeditor.

This patch does not attempt to change any other "oddities" about the plugin handling, like how the paths for the button images next to the "Buttons and plugins" checkboxes are handled. Those are different issues that probably require some more extensive changes.

Anyone who's got time to confirm this patch is working and/or review it?
Would be interesting to hear how this change affected the integration of as many plugins as possible.

+++ editors/fckeditor.inc	27 Oct 2010 22:18:42 -0000
@@ -182,7 +182,8 @@ function wysiwyg_fckeditor_plugin_settin
-        $settings[$name]['path'] = base_path() . $plugin['path'];
+        // All native FCKeditor plugins must use the filename fckplugin.js.
+        $settings[$name] = base_path() . $plugin['path'] . '/';

Why is the ['path'] key removed from $settings?

+++ editors/tinymce.inc	27 Oct 2010 22:18:42 -0000
@@ -317,7 +317,18 @@ function wysiwyg_tinymce_plugin_settings
+        if (($name_start = strpos($path, '/' . $filename)) !== FALSE) {

!== FALSE is not necessary here, as we're only interested in the $name_start > 0 case. Btw, can we rename that variable to $pos ?

Powered by Dreditor.

Why is the ['path'] key removed from $settings?
Sorry, typo.

!== FALSE is not necessary here, as we're only interested in the $name_start > 0 case.
Hmm, maybe. Follow up question: Should we allow the plugin file to be placed anywhere, like in the root? If so, this patch needs to change slightly to not add slashes when there is no path.

Let's also incorporate #873900: TinyMCE Plugin trying to use non existent CSS file

I kept the strict FALSE check and added a check for path being just the filename, just in case anyone had the crazy idea of putting a plugin at the root... Could also have done it by removing the slash in the position lookup, but then someone might have called a folder in the path 'editor_plugin.js' and we'd get it wrong. Yes, I'm getting paranoid...

Feel free to decrease the paranoia level of the path if you'd like. Anyone not placing the plugin in a subfolder will just have to adapt.

EDIT: And I'm starting to second guess myself... I probably should have used === or strcmp() for that string comparison... I have a feeling this isn't going to end well... :s

+++ editors/ckeditor.inc	13 Nov 2010 20:07:04 -0000
@@ -250,13 +250,14 @@ function wysiwyg_ckeditor_plugin_setting
-        $settings[$name]['path'] = base_path() . $plugin['path'];
+        $settings[$name]['path'] = base_path() . $plugin['path'] . '/';

+++ editors/fckeditor.inc	13 Nov 2010 20:07:04 -0000
@@ -180,7 +180,8 @@ function wysiwyg_fckeditor_plugin_settin
-        $settings[$name]['path'] = base_path() . $plugin['path'];
+        // All native FCKeditor plugins must use the filename fckplugin.js.
+        $settings[$name]['path'] = base_path() . $plugin['path'] . '/';

The only remaining problem I have with this patch is that it changes the 'path' values to contain a trailing slash, and it doesn't seem like this has been actually tested/confirmed via API docs. I at least know that some of the editors will append a slash on their own, so this would result in a duplicate directory separator...

Powered by Dreditor.

Looking at surrounding code, it seems like CKEditor should always get a trailing slash.

For TinyMCE, we're passing a full filepath anyway.

For FCKeditor, I'm not sure and couldn't find any API docs on the web site, but I don't think that there's any native external plugin in the wild anyway, so "who cares"...

Simplified the TinyMCE BC code.

+++ editors/tinymce.inc	19 Dec 2010 23:08:47 -0000
@@ -316,7 +316,17 @@ function wysiwyg_tinymce_plugin_settings
       if (empty($plugin['internal']) && isset($plugin['path'])) {
...
+        // TinyMCE plugins commonly use the filename editor_plugin.js, but there
+        // is no default. Previously, Wysiwyg's API documentation suggested to
+        // have the 'path' contain the 'filename' property, so if 'filename' is
+        // not defined, automatically extract it from 'path' for backwards
+        // compatibility.
+        if (!isset($plugin['filename'])) {
+          $parts = explode('/', $plugin['path']);
+          $plugin['filename'] = array_pop($parts);
+          $plugin['path'] = implode('/', $parts);
+        }

For D7, we're not going to commit this BC code. Instead, we'll change the primary condition to test for 'filename' instead of 'path'.

Powered by Dreditor.

Thanks for reporting, reviewing, and testing! Committed to all branches.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

--

D7 requires $plugin['filename'] to be properly defined.