Problem/Motivation

In 7.x-2.x, the markup that gets rendered into WYSIWYG is "pre-built" in a JS settings array (Drupal.settings.media.formatFormFormats) and attached when the modal popup is launched. This is a problem if one wants to dynamically change the markup based on options and fields added in the popup.

Use case: There is an alignment field (right, left, center).

Proposed resolution

Rather than pre-computing the markup and attaching, dynamically build the markup using a new hook_menu callback. This menu provides the same markup as currently provided by the existing JS solution. But there is a drupal_alter hook available to enhance the markup that gets rendered.

Remaining tasks

Reviews

User interface changes

n/a

API changes

Change: Markup is generated dynamically after submitting the Media WYSIWYG modal form. Changes to formats using media_wysiwyg_format_form_prepare will no longer effect the rendered markup in the WYSIWYG. It will only effect the markup in the modal popup.

Addition: There is a new alter hook (hook_media_wysiwyg_wysiwyg_markup_alter) that provides a mechanism to enhance and/or replace the default markup that gets rendered in WYSIWYG.

Comments

heddn’s picture

Status: Active » Needs review
StatusFileSize
new7.67 KB

Let's see how things fair with the testbot. There was a large amount of what seemed like crufty/old js in popup.js. Rather than fix it to use a js callback for the ajax calls, I opted to remove it.

heddn’s picture

StatusFileSize
new8.83 KB
new1022 bytes

Added api.php docs.

heddn’s picture

StatusFileSize
new8.79 KB
new545 bytes
new8.96 KB

Following the lead in #2349977: DoS image derivatives in Media WYSIWYG, I've updated the hook_menu to use a more restrictive security access callback.

Additionally, if you happen to run #1792738: Allow custom file view modes for WYSIWYG display, then you'll want to apply the do-not-test patch.

heddn’s picture

heddn’s picture

heddn’s picture

StatusFileSize
new9.7 KB

The do-not-test patch is the same as test. This fixes that.

zuernbernhard’s picture

Great Work !!!

One Thing: i have no table "media_view_mode_wysiwyg" and thats why i get an exception in media_wysiwyg_wysiwyg_markup.

patched patch (just added "if (db_table_exists('{media_view_mode_wysiwyg}')) {" )is attached.

What is still missing to see this in the module ?

Status: Needs review » Needs work
zuernbernhard’s picture

BTW.: We use this together with Media Browser Plus (https://www.drupal.org/project/media_browser_plus) and Entity Emed Backport (https://www.drupal.org/sandbox/brockfanning/2400183) to Insert Images as Entities Using CKEditor Widgets.
Our Editors are happy to have these Widgets and with Filebrowser (https://www.drupal.org/project/filebrowser) they can rearrange the Images in the filesystem without "breaking" the already inserted images instances.

Status: Needs work » Needs review

Status: Needs review » Needs work
devin carlson’s picture

Component: Code » Media WYSIWYG
chiebert’s picture

The do-not-test patch in #6 (which supposed to apply if you're using #1792738: Allow custom file view modes for WYSIWYG display) fails on two hunks (if you've applied the latest from that issue against the latest -dev) as follows:

~: patch -p1 < media-allow_dynamic_wysiwyg_markup-2348439-6-do-not-test.patch
patching file js/media.popups.js
patching file modules/media_wysiwyg/includes/media_wysiwyg.pages.inc
Hunk #2 succeeded at 180 with fuzz 2 (offset 76 lines).
patching file modules/media_wysiwyg/js/media_wysiwyg.format_form.js
patching file modules/media_wysiwyg/media_wysiwyg.api.php
patching file modules/media_wysiwyg/media_wysiwyg.module
Reversed (or previously applied) patch detected!  Assume -R? [n] n
Apply anyway? [n] y
Hunk #1 FAILED at 34.
Hunk #2 FAILED at 48.
2 out of 2 hunks FAILED -- saving rejects to file modules/media_wysiwyg/media_wysiwyg.module.rej

On looking at the two failed hunks, the hook_permission() change had already been applied, but I needed to manually add the following to hook_menu() in media_wysiwyg.module:

  $items['media/wysiwyg/%file'] = array(
    'title' => 'WYSIWYG Markup',
    'description' => 'Generate WYSIWYG Markup',
    'file' => 'includes/media_wysiwyg.pages.inc',
    'page callback' => 'media_wysiwyg_wysiwyg_markup',
    'page arguments' => array(2),
    'access callback' => 'media_wysiwyg_access',
    'access arguments' => array('view', 2),
    'type' => MENU_CALLBACK,
   );

And without this patch, the patch in #1792738: Allow custom file view modes for WYSIWYG display wouldn't save overrides to the embedded media on form submit.

chiebert’s picture

Same with the patch in #5 (didn't try #7 since it was failing tests, but I guess it would be the same for that as well): the permission change is now already in -dev, so these need a re-roll. Never done one of those before...

kevinquillen’s picture

Would seem to cause the issue of it not respecting a media object used more than once in a WYSIWYG field. If you use the same image multiple times with different view modes, it does not distinguish between any of them and simply replaces all tokens matching on very basic patterns. This overwrites the selected view mode on the tokens, causing all instances of the same image inserted to use that view mode. It needs to respect the view mode selected per token.

Also media source map in Drupal.settings doesn't record anything but the file and fid, making it difficult to reuse the same file multiple ways.

joseph.olstad’s picture

Status: Needs work » Closed (duplicate)
Parent issue: » #2842391: better support for float media left and float media right

see parent issue:
support for float media left and float media right

this functionality was added a while back, and works quite well