Scenario

With the following scenario, something goes wrong:

  1. Add a (embedded) paragraph item which contains a field with a WYSIWYG editor (in my case tinymce)
  2. Add another paragraph item (the same one with the WYSIWYG editor)
  3. Remove the second item immediately

Now the editor for the first item will be broken. It will not load anymore. Switching the formatter select also does not trigger the editor.

Problem

What is happening?

Clicking the "add item"-button does an AJAX callback. Wysiwyg does add JS settings when rendering the text element in wysiwyg_pre_render_text_format() in wysiwyg.module at line 296.

$element['#attached']['js'][] = array(
    'data' => array(
      'wysiwyg' => array(
        'triggers' => array(
          $element['value']['#id'] => $settings,
        ),
      ),
    ),
    'type' => 'setting',
  );

The first time a paragraph item is being added, the AJAX response contains settings for that item. When the second one is added, the settings are for the first and second item, but now the first item has a different id.
What actually happens in the ajax.js is that those settings are being merged. This is because in ajax_render() in includes/ajax.inc on line 299

array_unshift($commands, ajax_command_settings(drupal_array_merge_deep_array($settings['data']), TRUE));

instructs ajax_command_settings() to merge the settings (by passing TRUE). As it should.

So... the JS settings now have three items. Two for the first item, once with a normal id and once with --2 appended.
This is still no problem (due to the processing order, which will become clear later on), but when you delete the second item, the settings returned by wysiwyg_pre_render_text_format() now again contain settings for the first item, but with the original ID (no --X appended).

Ok, bear with me, the actual problem is coming...
In wysiwyg.js in updateInternalState() at line 480 all triggers (part of those JS settings) are being looped, and used to determine which formatter select belongs to which textarea. Since the base field id is being used, i.e. the --X part is being cut off, the two field settings for the first item conflict.
OK, there is some code checking if the _fieldInfoStorage already contains info about the base field id, but later on at line 534

fieldInfo.select = trigger.select;

will override the fieldInfo with the info for the second set of settings (the --2 settings). The --2 field does no longer exist, and neither does the matching select. Nevertheless the fieldInfo now has that unexisting select linked.

The same goes for every other setting of course, like the editor (which should be tinymce , but is now none).

In case it's not obvious: because the settings are wrong, the textarea is not converted into an editor, because the JS is trying to fetch the format from an undefined select (amongst other problems due to the misconfiguration).

Solution

The solution is really easy, but that raises my antenna's :-)
It seems too easy... However, it does seem to work.

Just, check if the actual field still exist before considering storing the according settings.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sneakyvv created an issue. See original summary.

Sneakyvv’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
751 bytes
TwoD’s picture

Thanks for the detailed report and the patch!
I think your patch on its own looks good and I will compare it with some other notes I have related to these kind of id conflicts but I'm too tired to do it tonight.

We're using Paragraphs quite in some complex setups at work so I'll see if I can use some time there to do some additional testing.

TwoD’s picture

Been thinking about this for a while now and testing various things.

Your approach works well, and in addition to this I found that we should remove the instance information from Drupal.wysiwyg.instances on the detach (unload) event so that we're not leaving those around. It confused some other modules which looked for any Wysiwyg instance without also checking if the field existed.

Wysiwyg still internally keeps track of the last enabled/disabled status per field so if you disable an editor, remove a field, and eventually add it back in (without reloading the page) the editor will not be automatically be re-attached (even if set to attach on load).

I took the opportunity to clean up some of the internal function calls. Several parameters related to a field's current state were passed into the attachToField(), detachFromField() functions while those functions really should just be told the desired state. This way I think it's easier to read the code and understand what the intent is, which helped debugging this.

Also added a sanity check to the format switching code so it doesn't detach and reattach editors when the format hasn't really changed. It's not directly related to this change but it's something that came up and was quite annoying while I was working on this.

Thanks again for the patch! It grew quite a bit with the above mentioned changes, but I'm giving you credit as it's all derived from your initial fix. ;)

  • TwoD committed 3d10c58 on 6.x-2.x authored by Sneakyvv
    - #2862459 by Sneakyvv, TwoD: Fixed issue with removed fields.
    
  • TwoD committed 772f569 on 7.x-2.x authored by Sneakyvv
    - #2862459 by Sneakyvv, TwoD: Fixed issue with removed fields.
    
TwoD’s picture

Status: Needs review » Fixed

I decided to add this immediately, it's now part of 7.x-2.5.

TwoD’s picture

Status: Fixed » Closed (fixed)

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