Error when adding field items - Add another item

I tried doing this in firefox with the latest ckeditor 4.4.1 and didn't see this error.

Did you have to do it again while the first editor was still loading?

Perhaps mine loaded too quickly for me to reproduce.

Hmm, it already does some checks before even patching in the functions which do the looping, so it should never get there unless Drupal.settings.wysiwyg.plugins[params.format] exists. If it exists, but Drupal.settings.wysiwyg.plugins[params.format].drupal is undefined, the for...in loop won't try to access anything below that.

If for some reason, the profiles for a format does have plugins so that editor instance has its DataProcessor methods wrapped, but they aren't there later, we've got some kind of state tracking error.

Thanks for the extra info, it will be useful when I get a chance to try to reproduce this with field collections.

Drupal.settings.wysiwyg.plugins is meant to always be included in the AJAX response when an editor instance requires plugins, will have to look closer at why that doesn't happen.

Most of the refactoring I've done to wysiwyg.js has been done to work around the very same problem you're describing, from the core outwards. As obvious from this issue, the number of reasons it is needed keeps growing. The next step would indeed be to keep the plugin information currently fetched from Drupal.settings.wysiwyg in a better place. I'm not sure keeping a complete snapshot of Drupal.settings.wysiwyg for each instance is ideal, since most of the information in there won't be relevant for that instance, but maybe it could be kept somewhere where it can be accessed easily and decoupled from the settings received via either the original page load or an AJAX response.

Turns out this general problem isn't specific to the CKEditor integration, that's just where it shows up because of the previously mentioned checks were not added there. We need a more robust way to handle cross-editor plugins which does not lead to confusions about which plugins are enabled on which profiles.

From now on, let's not access Drupal.settings directly from within the editor integration files.
The settings and plugin information will instead be managed by wysiwyg.js and passed in to the integration via the instance.

Speaking of the instance.... It's currently a bit difficult to tell which parts of the Drupal.wysiwyg.instances['field-id'] object are safe to call/alter from the outside. This patch solves that by creating a private copy of the instance object and exclusively relying on that when checking states etc. This also helps dealing with calls to the instances when the editor is inactive (calls get redirected to the 'none' editor).

All data is contained using scoped variables inside wysiwyg.js and the relevant parts for an instance are assembled and pushed in via the private instance as editors are attached. All calls from wysiwyg.js into the editor integration use the private instance object as the context (the this reference), and all calls from the public API are ensured to also have that same context.
All the shuffling and syncing of data (settings and plugin info) as editors are toggled is managed by wysiwyg.js itself and outside code can not accidentally throw the instance into an inconsistent state.
This gets rid of the problem where things passed back from AJAX requests overwrites settings or state for unrelated instances.

The above things are mostly changes in the clientside JavaScript code. A few things changed on the server to simplify things.
Editor integrations currently use a callback set in the key 'proxy plugin settings callback' to "translate" cross-editor plugin metadata into a structure which is supposed to fit better with how each editor likes that kind of information. It turns out all of the editor integrations which support proxy plugin settings all create pretty much the same data structure, duplicating much of the information passed down to the browser.
It's much more convenient to dump this data once per plugin and have the clientside code parse it as needed, and instead use the callback to put out just the information unique to that editor instance. This pretty much boils down to a list of which plugins are enabled, but the "protocol" is still open for anything to be put in there. The amount of de-duplication this patch allows for with will be most obvious for a later task where we implement multi-button support in proxy plugins.
Cross-editor plugins end up getting a bit more information passed into some of their methods, but I've not found any problems with any of the plugins I have tested.

A few changes were just made to shorten or clarify existing code now that the private instance is guaranteed to be the context. Some changes were made because related code was touched and I don't want to leave todos in there.

All changes should be fully backwards compatible with existing [proper] use of the Wysiwyg API.

If possible, please test this with your existing editor profiles to ensure no regressions were introduced.

I have the same problem.

An error occurred while attempting to process /system/ajax: TypeError: e is undefined

I think it could have to do something with the simplify module and boostrap theme: As this error occur under the following circumstances:

• If I tray to add an new field of a field collection
• At least one field in the collection has a wysiwyg profile
• Bootsrap is the used theme
• Simplify is enabled

To get rid of the error I could

• Use an other theme
• Or disable simplify

Maybe that helps to narrow it down.

Did you try the patch?

I get same error when trying to upload image when creating content. I am using the Nexus theme (bootstrap) with Wysiwyg latest dev version + TinyMCE 3. But only site theme is bootstrap, when on admin interface it's default Seven theme.

Update: I fixed it by downgrading to stable version of the module and downgrading to TinyMCE 3.5.4.

Did anyone try the patch?

I can not fix or debug ANYTHING if nobody is testing patches and reporting how it went.

Tried patch from #8, it solved my issue https://www.drupal.org/node/2505391

@sergesemashko, Excellent! Thank you very much for that confirmation!
I'm working nights this week and some overtime so I'm not sure exactly when I'll commit this, but it'll be soon.
Think I'll try to rebase a few other patches on top of it before it goes in, just to make sure it doesn't thwart any of the other things which I'm hoping will make the next release very good.

#8 worked for me too, but it caused an issue with the linkit module. After applying this patch the ckeditor was looking for the linkit ckeditor plugin inside sites/all/libraries/ckeditor/plugins instead of sites/all/modules/linkit/editors/ckeditor. As a quick fix I copied the plugin folder from the linkit module into the sites/all/libraries/ckeditor/plugins folder and renamed it linkit. Everything worked after that. I just wanted to point this out in case it was an issue with the patch above. Thanks!

Thanks for that, I'll look into it ASAP.

UPDATE: Found the problem. The patch rewrites the plugin handling in editor integrations to expect global (as in same across instances of the editor) metadata about native and "Drupal/cross-editor plugins" to be delivered once per editor instead of once per format an editor is assigned to. The editors use this metadata to register "external" (as in not placed below the editor library folder) plugins before an instance needs one of them so it knows where to load the files from. This is why none of the "internal" plugins were affected.

There's no point in duplicating the metadata sent to the client once for each format its been assigned to because a plugin can only be loaded once (the patch correctly removes old code which needed to check if external plugins had already been registered since it should no longer be needed).
However, the Wysiwyg core only delivered the metadata for cross-editor plugins this way and left the native plugin metadata duplicated per format (via plugin settings callback). I think a commit or two got left out from the patch, which added a new callback (plugin meta callback, called only once per plugin instead of per format) to the editor definition and moved much of what's now in the per-format callback there instead.

I probably mixed it up with a patch from another issue which also splits settings up into per-editor, per-format, and per-field sections to decrease the size of Drupal.settings.wysiwyg.

I'll cross-reference with that patch and update this one.

Here's the new patch with plugin settings callback replaced by the new plugin meta callback.
The old callback still works exactly the same way so it should be no different for any other module implementing an editor. None of the bundled editor implementations needed it anymore though.

EDIT: Scratch that, I borked it. New patch to follow.

@TwoD, should we try the above patch #17, or is there some work in progress? Thank you.

Please try the attached patch. I reworked things a bit, and now I think it's stable enough. The new plugin meta callback from #17 is still there. There will maybe be another use for plugin settings callback later so it's still supported.

Basically decided to explicitly track more of the settings and states internally and have all the logic for deciding which information from Drupal.settings is new and relevant in one place. As each editor instance is created it is given a copy of all the relevant settings information upon its creation, and its state is mirrored out to a public replica of the "Wysiwyg instance" representing a field and its attached editor. Any changes made to the public instance's properties are ignored by Wysiwyg to not risk encountering invalid states, but they may still be read by other code so it knows what Wysiwyg is currently doing.

This takes care of most of the problems related to the problems described here, and in #2566175: Drupal.settings.wysiwyg.plugins being overridden on AJAX calls..
I think it may be useful to apply the patch given in that issue as well, but I have not tested it with this patch yet.

If you find any modules/plugins which do not work after applying this patch (your own or from d.o.) I would like to know about it ASAP. I've tried hard to not break anything related to what I consider "good" use of Wysiwyg's API, but I've seen other code attempting to interface with it in very strange ways before...

If only minor issues are found, I will commit this patch and rebase everything else I have planned on top of it as soon as I can and make a push to include button management and TinyMCE 4 support, then we can finally have a proper release!

It helps if the patch actually gets uploaded...
EDIT: A few lines in updateInternalState() are commented out. An idea I had was to actually "purge" Drupal.settings.wysiwyg while reading data from it so there's no chance anything is still trying to use it directly. Doing this was successful in testing, but I don't want to risk breaking other modules in that way yet. I'm leaving the lines in there to make testing easier, and to hint of this possible outcome in later releases.

@TwoD WOW! Great patch and thank you for your hard work.

I applied your patch, in a quite complex article form with a lot of field collections and some additional bundles from the paragraphs module. Works great, all of my problems have been gone with that patch.

However, I have a new bug with the Simplify module only for authenticated users.

When I enable the option to "Hide text format selection", I am getting a lot of undefined errors messages:

Notice: Undefined index: format in wysiwyg_pre_render_text_format() (line 222 of /sites/all/modules/contrib/wysiwyg/wysiwyg.module).
Notice: Undefined index: format in wysiwyg_pre_render_text_format() (line 224 of /sites/all/modules/contrib/wysiwyg/wysiwyg.module).
Notice: Undefined index: format in wysiwyg_pre_render_text_format() (line 224 of /sites/all/modules/contrib/wysiwyg/wysiwyg.module).
Notice: Undefined index: format in wysiwyg_pre_render_text_format() (line 224 of /sites/all/modules/contrib/wysiwyg/wysiwyg.module).
Notice: Undefined index: format in wysiwyg_pre_render_text_format() (line 225 of /sites/all/modules/contrib/wysiwyg/wysiwyg.module).
Notice: Undefined index: #id in wysiwyg_pre_render_text_format() (line 268 of /sites/all/modules/contrib/wysiwyg/wysiwyg.module).
Warning: Invalid argument supplied for foreach() in element_children() (line 6525 of /includes/common.inc).

PS. I get the same error for each text field, so I copied only once.

Unchecking that option from the Simplify module, the errors stop. The strange thing is that with the admin user (UID = 1), I am not experiencing that issue.

So far, my workaround is to have that option enabled and hide it through CSS.

I will continue my debugging to see if it's applied only in my setup.

Thank you! That's exactly the kind of feedback I was looking for!
I've been testing mostly with Better Formats and there it works well when hiding the selector box, but maybe Simplify does it in a different way? At first glance it seems to just set $element['format']['#access'] = FALSE;, which Wysiwyg should know how to react to, but maybe I'm missing something.

The UID 1 part is interesting. That user normally has access to all formats, so maybe this happens only if it tries to hide one which a user has explicit access to, or one which they don't have access to?

Will have to install it in my test sites and see what happens. Should be fairly easy to fix. :)

Finally figured out what was causing the notices!
When a text_format element is expanded to two child elements - 'value' (textarea) and 'format' (selectbox) - by filter_process_form() it copies down all properties from the text_format element to 'value', including the Wysiwyg's #pre_render callback. Wysiwyg operates only on the parent element so it checks that $element has a 'format' property, which the textarea child in 'value' doesn not have.

The '#after_build' callback is also copied from the parent down into 'value' so Simplify's simplify_hide_text_format_element runs twice too. All is fine when it is passed the parent element and sets $element['format']['#access'] = FALSE. When it is given the 'value' element as a parameter there is no 'format' property in it already, so it gets created.

That makes Wysiwyg's safeguard against running twice fail and it generates the notices because it is given the wrong element (the textarea instead of its parent). This makes it appear as if no formats are available for the field so no editor gets attached.

A simple change to Wysiwyg's safeguard makes it more strict when checking that the element does indeed have formats available.
In wysiwyg.module, line 199:

-  if (!isset($element['format']) || !empty($element['value']['#disabled'])) {
+  // Simplify module creates an extra incomplete 'format' on the base field.
+  if (!isset($element['format']['format']) || !empty($element['value']['#disabled'])) {

Re-rolled patch attached. Considering filing a bug report to Simplify so it adds a similar safeguard and won't fool other modules.
EDIT: There already exists an issue: #2483393: Multiple notices and warnings when used together with wysiwyg.module.

TwoD please accept my "Green Light" :-)
Great patch! Really appreciate your hard work.

Finally committed!
Backporting to D6 took a while, but I think it was worth it considering the branches are now much closer to each other, and AJAX/AHAH stuff is finally working in D6 without additional module! (Working with that old code brought back some memories... and nightmares! ;P)

I see I messed up the last merge a bit, but who cares...

Now on to re-rolling all other stuff on top of this...

Thank you all for reviewing, testing and commenting!