Problem/Motivation

Due a recent commit (from a patch I wrote) on #2720997: Overridden fields not encoded in token this module is now overriding an entire javascript function from the media_wysiwyg module: Drupal.media.formatForm.getOptions(). In hindsight, there is a big problem with this: This module won't get any features/fixes that happen in the media_wysiwyg module. This is bad, because the media_wysiwyg version has important things, like support for multi-value fields.

Proposed resolution

No longer override that function in this module.

There is a catch though: the override here WAS doing something useful, namely capturing input entered into CKEditor text areas in the "format form" where editors configure embedded media. So we need to make sure that this only gets committed after a yet-to-be-written patch in the Media module that reproduces this feature: #2833190: Support CKEditor (module) for long text fields in "format form"

Remaining tasks

Write this patch.
Review this patch
Write a patch for Media to reproduce what we are losing here.
Review patch in Media that reproduces what we are losing here: #2833190: Support CKEditor (module) for long text fields in "format form"

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#7 override-getEditorContent-instead-of-getOptions_2833107-7.patch1.96 KBLittleRedHen
#2 remove_override_of-2833107-2.patch2.2 KBbrockfanning

Comments

brockfanning created an issue. See original summary.

brockfanning’s picture

brockfanning commented
Issue summary: View changes
StatusFileSize
new remove_override_of-2833107-2.patch2.2 KB
brockfanning’s picture

brockfanning commented
Issue summary: View changes
brockfanning’s picture

brockfanning commented
Related issues: +#2833190: Support CKEditor (module) for long text fields in "format form"
brockfanning’s picture

brockfanning commented
Issue summary: View changes
LittleRedHen’s picture

LittleRedHen commented

I have tested the patch from #2 together with the matching media patch, and can confirm that the combination works fine on my bleeding-edge media + media_wysiwyg + media_ckeditor setup (which currently involves a lot of patches, but none that ought to change how this works).

LittleRedHen’s picture

LittleRedHen commented
StatusFileSize
new override-getEditorContent-instead-of-getOptions_2833107-7.patch1.96 KB

I also submitted an alternate solution on the media module thread at https://www.drupal.org/node/2833190#comment-11812554 which split the 'fetch content from my custom editor' bit out into a function that is designed to be overridden, which would mean the media module doesn't need to know the details of all the possible editors.

Here's the media_ckeditor patch which works with that variation: it overrides the new function, instead of the whole getOptions function, and so doesn't need to deal with encoding or any of the other media-token-specific details.

You should only apply this patch if you're also applying the matching patch on the media module (or if that patch has been committed).

brockfanning’s picture

brockfanning commented

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Status: Active » Fixed

in dev of media_ckeditor
requires media 7.x-2.x dev , or beta14 which hasn't yet been released /nor tagged

Status: Fixed » Closed (fixed)

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

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

note: beta14 was tagged since I last commented.

However, I recommend the latest currently, which is 7.x-2.0-rc1 (released today)
also, both media and media_ckeditor have a 7.x-2.0-rc1 release , same date.