Problem/Motivation

During media token input filtering, the function mediay_wysiwyg_token_to_markup() adds all found media tokens to javascript using drupal_add_js() and the file ID (fid) as key. The problem with drupal_add_js() is that arrays with numeric keys, the keys are ignored as the structure is converted to an array with auto-incremented integers as keys in javascript.

So if we in PHP have this desired mediaDataMap entry:

$mediaDataMap = array(
  12 => array(
    "fid" => 12,
    "link_text" => NULL,
    "type" => "media",
    "view_mode" => "teaser",
  ),
);

we end up with this structure in javascript:

// Note that this is now an array, not an object.
Drupal.settings.mediaDataMap = [
  {
    fid: 12,
    link_text: null,
    type: "media",
    view_mode: "teaser",
  }
];

Where the key (file ID) in PHP was 12, it is now 0 in javascript. It will not be found again.

So what is this used for? Why is this added? As soon as the wysiwyg editor kicks in, proper entries in Drupal.settings.mediaDataMap with correct file ID as keys are added. I assume these are generated because the actual file entries are missing. And for small installations, the files with actual file ID = 1 or 2 will collide with the second or third entry of Drupal.settings.mediaDataMap[] as sent over from PHP..

CommentFileSizeAuthor
#4 media-2946380-wrong-fid-key-in-mediadatamap-4.patch1.3 KBkaare
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kaare created an issue. See original summary.

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

hope this helps, maybe something getting in the way of the cache re-load or hash key for retrieval of items.

did you need to add something to the whitelist?

there is a trick here, security fix that introduced a cache option to pass params to javascript from the browser.

see media.module

There is a whitelist of known safe options. Pay attention to 

     // Filter out everything except a whitelist of known safe options.
     $safe_options = array(
       'enabledPlugins',
       'fid',
       'id',
       'multiselect',
       'field',
       'options',
       'plugins',
       'render',
       'types',
       'render_multi_edit_form',
     );

commit 4479a380dc9fd9ca46c55835e62dc1fc0b41b821
Author: joseph.olstad <joseph.olstad@1321830.no-reply.drupal.org>
Date:   Thu Apr 6 13:13:13 2017 -0400

    by Dave Reid, joseph.olstad, fafabedoya, Chi, mlhess, ParisLiakos, greggles, Heine, pfrenssen, Andy Tawse, larowlan, deviantintegral, mglaman, senzee - media/browser URL prevent tampering of (allowed file extensions, access, etc.)

and

commit 86cefbbe7691aac2a6773503c8e613598040f3ec
Author: richard.thomas <richard.thomas@1924680.no-reply.drupal.org>
Date:   Tue May 9 20:19:32 2017 -0400

    by richard.thomas, joseph.olstad: change url filtering from blacklist to whitelist and add simpletest for special use case.
kaare’s picture

kaare
him/his
Primary language Norwegian Bokmål
Location Tromsø
CreditAttribution: kaare at Norway Opening Universities commented

The problem lies in drupal_array_merge_deep_array() during drupal_get_js() which renumbers the keys in array structures upon merging of settings prior to drupal_json_encode(). This exact code:

      // Renumber integer keys as array_merge_recursive() does. Note that PHP
     // automatically converts array keys that are integer strings (e.g., '1')
     // to integers.
     if (is_integer($key)) {
       $result[] = $value;
     }

What we have to do is find another way to index this datamap, based on a combination string and file id to avoid this glitch. It might also be worth combining this with #2945625: Remove duplicate field information in media token which needs a different way to access this datamap based on file id and delta. And, anyway, if the same file is inserted several times in the same page, the generation of mediaDataMap in PHP would override all previous entries of it, if the php→js settings conversion worked as intended.

One possible solution is to have a two-dimensional array with fid first and delta next:

['mediaDataMap'] => [
  'fid:' . $fid => [
    'delta:' . $delta => 
    'type' => 'media',
    'fid'  => $file->fid,
    'view_mode' => $tag_info['view_mode'],
    'link_text' => $tag_info['link_text'],
  ],
];

Then accessing these items in js could be done by a wrapper function:

var getFileInfo(fid, delta) {
  var fidKey = 'fid:' + fid;
  var deltaKey = 'delta:' + (delta == undefined ? '0' : delta);
  delta = delta || 0;
  return Drupal.settings.mediaDataMap[fidKey][deltaKey] == undefined ? {} : Drupal.settings.mediaDataMap[fidKey][deltaKey];
}
kaare’s picture

kaare
him/his
Primary language Norwegian Bokmål
Location Tromsø
CreditAttribution: kaare at Norway Opening Universities commented
Status: Active » Needs review
FileSize
media-2946380-wrong-fid-key-in-mediadatamap-4.patch1.3 KB

Aaand, having looked some more at the code I don't think there is any need for building this server-side anyway. This structure is built client side when the wysiwyg editor is enabled and accessed upon detachment. The rudimentary values built in php has no value. wysiwyg works like usual without this.

kaare’s picture

kaare
him/his
Primary language Norwegian Bokmål
Location Tromsø
CreditAttribution: kaare at Norway Opening Universities commented
Version: 7.x-4.x-dev » 7.x-3.x-dev
joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

great work, I never noticed this before,
Without having tested all the possible versions and combinations of setup, it may be possible that the server side code was needed by media_ckeditor (in the media recipe) , in the case of media_dev distribution, it is using wysiwyg , not media_ckeditor. There's many ways to use and configure the media module. for the 4.x branch, it makes sense to support less variations, so I agree, it makes sense, go with it for 4.x. For 3.x and especially 2.x (200000 installs), it would be maybe more of a risky change.

the two most common ways to use the media tokens is one: through wysiwyg with either ckeditor library or tinymce
two: using the media_ckeditor in place of the wysiwyg module in combination with the ckeditor module

there may be other ways, other media related modules.

for 4.x, I like the idea of supporting wysiwyg only as it's more commonly used, keep it simple like you are doing, excellent work.

kaare’s picture

kaare
him/his
Primary language Norwegian Bokmål
Location Tromsø
CreditAttribution: kaare at Norway Opening Universities commented

I'll commit this to the 4.x branch without further discussion. And unless the use of Drupal.settings.mediaDataMap is traversed one item at the time, there is no other way to actually retrieve the correct media element. I think it's pretty safe to push it to 3.x as well, but it can wait until the 4.x branch actually differ from the 3.x branch and is tested some more.

And I'm glad to hear hints about stripping down complexity, features and support for all possible kinds of usage of media.module in 4.x. I'm not so sure about removing support for ckeditor.module and only go for wysiwyg, but e.g. the rendering mode of file fields in text fields (var media_wysiwyg_default_render) is ready for removal. Legacy rendering (using field attach has been here for ages. It's about time it's removed.

  • kaare committed 8ea9fa3 on 7.x-4.x 
    Issue #2946380 by kaare: Wrong FID key in generated Drupal.settings....
kaare’s picture

kaare
him/his
Primary language Norwegian Bokmål
Location Tromsø
CreditAttribution: kaare at Norway Opening Universities commented
Version: 7.x-3.x-dev » 7.x-4.x-dev
kaare’s picture

kaare
him/his
Primary language Norwegian Bokmål
Location Tromsø
CreditAttribution: kaare at Norway Opening Universities commented
Version: 7.x-4.x-dev » 7.x-3.x-dev
Status: Needs review » Postponed

Actually, I'm postponing this and setting it back to 3.x while waiting for further 4.x testing

kaare’s picture

kaare
him/his
Primary language Norwegian Bokmål
Location Tromsø
CreditAttribution: kaare at Norway Opening Universities commented
Status: Postponed » Needs review

I think it's safe to commit this to 3.x. If not, let's just close this as fixed for 4.x

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

ok, if you want, try committing to 3.x , or you can look for the git hash and do a cherry pick like this:
git checkout 7.x-3.x
git cherry-pick 8ea9fa3
if no errors then
git push

  • kaare committed 6ac4417 on 7.x-3.x 
    Issue #2946380 by kaare: Wrong FID key in generated Drupal.settings....
kaare’s picture

kaare
him/his
Primary language Norwegian Bokmål
Location Tromsø
CreditAttribution: kaare at Norway Opening Universities commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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