Embedded media objects should honor display suite settings

The patch in #4 fixes the issue.

+++ b/modules/media_wysiwyg/includes/media_wysiwyg.filter.inc
@@ -183,6 +186,7 @@ function media_wysiwyg_token_to_markup($match, $wysiwyg = FALSE) {
+    $element['content'] = file_view($file, $tag_info['view_mode']);

@@ -194,9 +198,6 @@ function media_wysiwyg_token_to_markup($match, $wysiwyg = FALSE) {
-    field_attach_prepare_view('file', array($file->fid => $file), $tag_info['view_mode']);
-    entity_prepare_view('file', array($file->fid => $file));
-    $element['content'] += field_attach_view('file', $file, $tag_info['view_mode']);

It also doesn't seem to have any side effects, since file_view already calls the same stuff internally.

This makes a lot more sense than the current implementation. Thanks for the patch!

Committed to http://drupalcode.org/project/media.git/commit/cd63328.

Ah, great! Thank you @jmuzz.

Hey guys - I was having an issue with the latest version of media module that I have traced back to the commit referenced in #9.

Basically, I had pages that were previously working fine with media content embedded into WYSIWYG as the 'Full Content' aka 'full' media view mode (this view mode is the key point - other view modes aren't a problem). After the patch, I get 'Connection Reset' in Firefox when viewing the same page, which is occurring because of an infinite loop in PHP.

What happens:
media_wysiwyg_token_to_markup() has a newly introduced call to
file_view() which checks:
if (!empty($file->fid) && !($view_mode == 'full' && file_entity_is_page($file))) { (file_entity/file_entity.file_api.inc)
The call to file_entity_is_page() then does:
$page_file = menu_get_object('file', 1);.

It's this call to menu_get_object() which ultimately starts the process of loading my node entity again (which is ALREADY being loaded via menu_get_object() by virtue of visiting it as a page), which again takes us back to media_wysiwyg_token_to_markup() - and the loop continues indefinitely.

--
Thoughts for solutions:

1. So, one might argue that you shouldn't use the "Full Content" view mode when embedding media into WYSIWYG. There's logic to that I suppose, but I would try to disagree - mostly because it's always been available to use, and in our use case this is our primary one-size-fits-all mode for the most part. At the least it would be very unfortunate for me and other users that already have all embedded media configured to use this mode. Taking this stance I think means at a minimum you must then prevent the view mode from being used via the media WYSIWYG UI for the future.

This would leave current users using 'full 'SOL with infinite loop pages, potentially lots. Doesn't really seem acceptable to me, but I could imagine someone taking this position.

2. To me file_entity_is_page() (file_entity module) seems to be pretty heavy handed by calling menu_get_object() - clearly this will attempt to load just about anything and this example proves bad things can happen when it's done at the wrong time. Without thinking much about it, it just seems there are better ways to perform this check. We could lobby them for a fix.

I will mention that file_entity module's template_preprocess_file_entity() runs the exact same check, so this would need to be changed in at least two places (maybe more?).

3. I think a hacky but effective solution may be to tamper with menu_get_item's $router_items temporarily during the file viewing process to prevent the infinite recursion, then restore it directly after the view process is done. God might kill kittens, but he's killed cuter baby animals for less in the past.

function menu_get_item($path = NULL, $router_item = NULL) {
  $router_items = &drupal_static(__FUNCTION__);
... 

I'm going to experiment with this, as it seems to be the most direct way to keep things from breaking and maintain the patch. It's probably a bad idea, but maybe it can serve as a not-committed stop gap.

4. Something I didn't consider?

Depending on the direction taken, it may be worth considering reverting the patch - at least until infinite recursion issues are no longer a factor. What do we think?

Let me know if you guys would rather spin up a new issue, just thought I'd start here first where the trail was fresh.

Hey Justin,

Your use case is perfectly valid and I encountered the same problem you have when I implemented this patch. The patch in #1 for the related issue in File entity solves the problem by rewriting file_entity_is_page: #2195491: Change request to support WYSIWYG

@idebr - Well shoot, I thought of looking through File Entity issues before posting this. Thanks for the point in the right direction.

The patch in that issues resolves the problem for me as well. Thanks!

If this new rendering disrupted anyone else's WYSIWYG content (like ours) - because it's now pulling in full template file, here's a gist to revert via hooking into token_to_markup_alter() - https://gist.github.com/mglaman/f9460f9c1bd6bf230dd5

I'm coming from https://www.drupal.org/node/2401811 after having the contextual links rendered in the output.

Thanks mglaman, just tried your code. Works like a charm!

But nevertheless, this should just be a workaround, as this issue completely changed and destroyed the output of the media filter. Therefore I re-open this issue now.

@agoradesign The original issue has been fixed. If you disagree with the solution or you encounter a bug with the implementation, please open a new issue to prevent confusion about the status of this issue.

ok, here's the new one: https://www.drupal.org/node/2408629

The solution as committed is not currently acceptable from my standpoint. It's clear that this change has caused several major regressions that would need to be fixed:
#2401811: With Media WYSIWYG enabled - "Contextual links" are shown for anonymous users
#2408629: Media filter always renders full template output
#2367725: Max nesting level reached due media_wysiwyg_token_to_markup() & view mode "full"
#2195491: Change request to support WYSIWYG

We either need to revert the change, or make it configurable which output method is used, and default to the old method.

+1 that we revert this.

We need to also revert #2367725: Max nesting level reached due media_wysiwyg_token_to_markup() & view mode "full" as well if this becomes reverted.

Is this the cause of several regressions as in comment #21 or is this more in keeping with D8 as suggested in #2408629-3: Media filter always renders full template output ?

I arrived here because I'm having issue with contextual links showing to everyone.

We either need to revert the change, or make it configurable which output method is used, and default to the old method.

By make it configurable are you suggesting two options in the configuration page such as: Embedded entity and Field attach.

If so, do you mean for each available view mode or a general settings?

Is it not acceptable to check for view modes which are ticked for embedding files like this:

if (!array_key_exists($tag_info['view_mode'], media_wysiwyg_get_wysiwyg_allowed_view_modes($file))) {
  $element['content'] = file_view($file, $tag_info['view_mode']);
}

As detailed above here's a patch related to the remaining tasks:

Make output method configurable, per maintainer feedback in #21

It should solve these two issues:

Contextual links shown to anonymous users and the markup/linking in wysiwyg issue.

You need to flush the cache after applying the patch.

This patch is working for me! No contextual links for anonymous users with this patch.

So I'm trying to figure this patch out. Disclaimer: only read the patch, didn't test.

+++ b/modules/media_wysiwyg/includes/media_wysiwyg.filter.inc
@@ -195,7 +195,9 @@ function media_wysiwyg_token_to_markup($match, $wysiwyg = FALSE) {
+    if (!array_key_exists($tag_info['view_mode'], media_wysiwyg_get_wysiwyg_allowed_view_modes($file))) {

This line confuses me. So if the array key does NOT exist, we use the Entity view, so Display Suite can work its magic. However, if they key exists, render as if field attach.

If I am understanding this right.. if you have not customized the file display view modes, it will use Display Suite? Or does this patch actually undo the original motive of the issue?

I wanted to mark this as "needs work" because I don't think it's actually supporting both options. But I don't use Display Suite and maybe I'm missing something.

@mglaman, I don't expect it to undo the changes in the original issue, but exclude the rendering for view modes that are selected for embedding embedding files inside of the the WYSIWYG editor, which can be configured at: /admin/config/media/wysiwyg-view-mode.

@DanzDance thanks for checking this. Like you I'm concerned with the contextual links and adding links to images in wysiwyg, although I had quickly tested fields with DS am hoping someone can also test this with fields.

@drupalove great, that's what I thought. Sounds like the proper way to do it to me, then. Great fix!

@drupalove It's fixing the issue for me too.

Forgive me if I'm being premature in reverting this to Needs review, but:

I've just been trying to deal with the contextual links issue that seems to be appearing in a number of related issues. Seeing some of the comments here - notably at #28, I assumed that perhaps this one patch would be the Holy Grail and deal with the underlying contextual links problem, but leave the other behaviours alone.

Alas. I'm not using Display Suite to define any file entity view modes - just a custom view mode via hook_entity_info_alter(), and I've attached a caption field to the image file entity. Before this patch, I get the unwanted contextual links for anonymous users - but I also get the expected media-element-container wrapper <div>. After this patch, I don't get the contextual links, but I also don't get a full rendered image file entity as per the view mode I've specified. I'm just getting a naked <img> element, without a wrapper <div> or the caption field I've configured for the view mode.

I'm using the latest -dev versions (as of this date) of File Entity, Media, and Ckeditor. Media_wysiwyg and media_wysiwyg_view_mode modules are enabled, and I've confirmed that, even with this patch applied, I'm getting the machine name of the view mode appearing as a class on the rendered <img> tag: in my case, file-em-img-caption.

Is anyone getting the best of both worlds with this patch + any other?

Oops - meant to set this to 'needs work', actually...

So, yeah - I've spent some more time in the debugger and the logic of this escapes me:

if (!array_key_exists($tag_info['view_mode'], media_wysiwyg_get_wysiwyg_allowed_view_modes($file))) {
  $element['content'] = file_view($file, $tag_info['view_mode']);
}

I don't use DS, but I do have a custom view mode and I have added fields and have configured my view mode to display one of those fields. So, in my case (since I've embedded an image via Ckeditor, and used media_wysiwyg_view_mode to choose my view mode for the display of my image), if (!array_key_exists(...)) evaluates overall as FALSE, so file_view() never gets called for my image file entity, and so my custom field never gets attached, and therefore, later on down the function, when we get to

if (count(element_children($element['content'])) > 1) {...}

$element['content'] only has 1 child, ['file'], so it evaluates as FALSE, and I never get my wrapper divs...

Does this really work for folks using DS?

@chiebert thanks for looking into this.

Adding your custom view mode is fine since it will be excluded from the unwanted rendering with Wysiwyg editor.

If everyone is happy with the approach of skipping view mode rendering in the Wysiwyg, it is easy to extend it to output the image caption & other fields you have added and a wrapper - in the same way like the image attributes and the image link (link_text), classes, etc. are output.

@drupalove: I think perhaps you may have misunderstood me: I created my image file view mode and selected it for display using media_wysiwyg_view_mode precisely because I wanted - and expected - view mode rendering in the final node view.

I think that's the expected behaviour here, since out of the box, when one enables media_wysiwyg_view_mode, one is presented with a choice in the media browser popup: 'display as', with a choice of view modes (configured via media_wysiwyg_view_mode). Why have the configuration to choose a view mode, and then present the user with a view mode choice, if by the very act of choosing that view mode, this patch excludes it from view mode rendering? We shouldn't then require the user to write a hook_media_wysiwyg_token_to_markup_alter, just to attach the fields that she has already painstakingly configured using the UI.

Am I missing something here?

Okay, I'm nervous that, because this change potentially breaks so many old sites, it'll be reverted, which I think is a step backwards - I mean, leveraging the already-existing file_view() makes total sense here.

I'm willing to plug away at a way to configure this, as per @Dave Reid's concern in #21. Would this work as a concept:

• Declare a new variable in the media_wysiwyg group: media_wysiwyg_default_render, configurable at the existing admin/config/media/browser page where the other media_wysiwyg variables are set.
• Variable asks user to 'Choose a default method of rendering file entities when embedded in a text field'. Options: 'Full file entity rendering' or 'Legacy field attach rendering' (better names?). Probably should also include some more helpful explanations of why one over the other.
  • Optionally, in media_wysiwyg_view_mode module, declare another 'multiple'-type variable, media_wysiwyg_render_VIEW_MODE, so that when someone chooses to restrict available view modes at admin/config/media/wysiwyg-view-mode, she can set a rendering type per enabled view modes. (But the UI for that might be ugly, especially since the logic of that particular admin page is currently a bit opaque - but see #2158177: Confusing UI for WYSIWYG view modes)
• Then, in media_wysiwyg_token_to_markup() we check these variables:
  • if there's a rendering method set for the file's view mode, in media_wysiwyg_render_VIEW_MODE, use that;
  • else if there's a default method set in media_wysiwyg_default_render, use that;
  • otherwise, default to the legacy method

Hey @chiebert,

I’m glad we’re in agreement the output should be configurable as suggested by Dave.

Could you throw some light on why we need a variable on the browser page at admin/config/media/browser.

Also, why can’t we check for allowed wysiwyg view modes in the array media_wysiwyg_wysiwyg_allowed_types() like I did in the patch rather than declaring another multiple type variable? I understand if the elements are not in the array then the user has intentionally not restricted them at admin/config/media/wysiwyg-view-mode and wants to use them for embedding files via wysiwyg. In other words, we have a handy switch already available to us rather than adding a variable at admin/config/media/wysiwyg-view-mode.

After you had the issue with the markup and caption, I thought we could look into using the same conditional execution in the media_wysiwyg_token_to_markup() - i.e. checking again for allowed wysiwyg view modes and adjusting the output to include the caption and other fields for the allowed view modes.

If you think it will not work could you briefly shed some light as your earlier comment was not clear to me.

If we agree my way is not suitable I’m in support of declaring new variable(s).

@drupalove: I'll see if I can be more clear...

Could you throw some light on why we need a variable on the browser page at admin/config/media/browser.

Precisely because we need to configure this rendering method, and I can't see how your proposed patch in #28 is testing the cases correctly (see next response).

Also, why can’t we check for allowed wysiwyg view modes in the array media_wysiwyg_wysiwyg_allowed_types() like I did in the patch [...]?

(By which you mean media_wysiwyg_get_wysiwyg_allowed_view_modes(), of course.) The main reason is that your test is backwards, I think - don't get me wrong: I keep saying that I'm open to being corrected, but nobody has done so thus far.

I see two problems with your patch:

1. media_wysiwyg_get_wysiwyg_allowed_view_modes() returns an array of view modes that a user has selected for customisation at admin/structure/file-types/manage/FILETYPE/display, using the 'Custom display settings' vertical tab at the bottom. Right? You're testing that the requested view mode is NOT in that list, before calling file_view() on that file entity. But your requested view mode will always be in that list (otherwise, you wouldn't have been able to select it in the popup. So you're never going to have file_view() called, and therefore you'll never get any of your custom fields rendered.

   And by the way, if the user has also installed the Media WYISWYG View Mode submodule, and chosen to restrict the view modes presented in the media browser popup, then that array will have even fewer items. In my case, it only had the one, which is the one that I chose.

2. The second problem is, even if I'm wrong and your test works for some reason, this doesn't count as a configuration solution, because you haven't put back in the old way of rendering fields, which were removed up in #4. At least before that patch was committed, some of us we still getting our fields on our embedded file entities.

Not being a user of Display Suite, I assume that the change up in #4 was made so that, however you chose to define your entity's display, you could be sure that was what was rendered when it was embedded in a text field. That being the case, we need to find a solid/easy way to let users manage the display of their file entities, and configure which rendering method is used. I suggested the Media browser config page as a place for a default rendering setting, since that's the currently-existing media_wysiwyg config page, and so it makes sense to put it there, from a UX perspective.

If we also want users to be able to more granularly set a rendering method per view mode, then I retract my earlier suggestion of using the media_wysiwyg_view_mode admin page, and rather suggest adding a vertical tab to the admin/structure/file-types/manage/FILETYPE/display admin page for each of the instances of the multi-variable...

@chiebert, thanks for clarifying this and for mentioning the change in #4 which will make the next step easier.

I think you missed something, see number #2 below.

media_wysiwyg_get_wysiwyg_allowed_view_modes() returns an array of view modes that a user:

1. has selected for customisation at admin/structure/file-types/manage/FILETYPE/display
2. has not restricted them for the wysiwyg at admin/config/media/wysiwyg-view-mode. See the screenshot: ‘allowed_view_modes.png’ which shows 5 view modes not restricted (not ticked).

See the other screenshot showing the dpm of $view_modes returned by media_wysiwyg_get_wysiwyg_allowed_view_modes() in the node. It is an array of the exact 5 view modes (keyed by machine names) which I have not restricted at admin/config/media/wysiwyg-view-mode.

Unless I’m missing something I still believe we are able to distinguish the view modes of embedded wysiwyg files without additional variable at admin/config/media/wysiwyg-view-mode

What do you think we should do next?

I'm glad you're trying to push this forward which eventually will fix at least 3 issues I know of beside this beta blocker issue.

I didn't miss that bit about restricted view modes - see the last sentence of my first point in #43.

Unless I’m missing something I still believe we are able to distinguish the view modes of embedded wysiwyg files without additional variable at admin/config/media/wysiwyg-view-mode

Let me try this again, from the beginning (meaning, before this whole issue started):

1. People have come to expect that when they use Media WYSIWYG to insert a media object in a text area, that they will get the file itself, plus whatever fields they've configured for that entity over at admin/structure/file-types/manage/FILETYPE/display.
2. Before this whole issue came along, those fields were prepared because media_wysiwyg_token_to_markup() sort-of-manually built up the fields using calls to field_attach_prepare_view(), entity_prepare_view() and then field_attach_view() (see the lines that were removed in the patch that was committed way up in #4).
3. This apparently worked fine for people who were just using the default D7 field UI + File Entity (it was working for me), but it apparently didn't bring in a fully rendered entity for those who were using Display Suite, which does more than rearrange the fields. Hence, this issue was born.
4. The patch in #4 was conceived and tested and committed. Now, the field_attach_view() method was completely removed, in favour of adding field information from media_wysiwyg_filter_field_parser($tag_info)to the $file object, and then simply calling file_view() from File Entity, which is all about using the right way of rendering entities - so presumably now Display Suite-managed view modes are handled properly (again, I don't use DS, so I don't know what's different about the method).
5. The problem is that some folks now had slightly different markup from before, which was a major regression. Plus, since the full entity was being built, contextual links were being added in if you had Contextual Links enabled, and these were being cached and shown to all users (see the other issues out there).
6. So, we need a way for a site builder to say: "hey, I want to use the full entity-build method, because I use DS and otherwise I'm screwed" or either to explicitly opt for the old way (which would mean putting back in the calls to field_attach_view() and its precursers), or else simply default to using the old method.
7. Just because someone has customised a view mode and allowed it in the WYSIWYG does not mean that they've made a decision one way or another about how they want to have it rendered. This is the gist of your attempt in #28 (which I still think it backwards - lose the !, and it might make a little more sense to me).
8. Also, if you use an if (whatever test you want) {use the file_view method} then you would also need to include an else {use the old method} to cover those who want the old method. Which you've left out.

So, the way forward, as I see it:

We need something to test, to see if someone wants/needs the full entity rendering of file_view(), or the old method of calling field_attach_view().

We might possibly test to see whether the selected view mode is controlled by DS - but who knows if there will arise other module out there that does something similar to DS and we'd be back in the same boat.

If we can't find something existing to test (and I hope I've convinced you that media_wysiwyg_get_wysiwyg_allowed_view_modes() does not cut it), then we need a variable.

#45 is a very good summary. As chiebert says, for this to be configurable, we need some way to know whether the site should be using the old field attach–based rendering method or the new file_view() method. Proposals on the table are

a) opt in to the new rendering sitewide, via a checkbox at admin/config/media/browser
b) opt in to the new rendering one view mode at a time, via a vertical tab at admin/structure/file-types/manage/FILETYPE/display
c) use old-style rendering for people who have restricted their list of wysiwyg-allowed view modes, and new-style rendering for everyone else (at least this is how I think the patch in #28 ends up working)

The original point of this issue was to get rid of the old field attach–style rendering entirely. Does anyone disagree with that as a long-term goal? If not, then I'd rather see us go with a) or b). That way, we can default to the old rendering method, and people can switch over to the new rendering when they're ready to. Nobody's markup will break because of a minor version update. On the next major update, we could even get rid of the old method, and point out the updated markup in the release notes as something people need to be ready for. If we go with c), we'll be stuck with the old field attach–based rendering, even into the next major version update.

I personally think a) is the best and fastest way forward. b) seems like overkill (especially if we want to someday phase out the old rendering style). Dave, any opinions? Technically I'm a media module maintainer but I haven't committed anything in donkey's years so I don't really count. :)

I agree with ksenzee that #45does a great job of summarizing the thread/issue. I also concur that option a is a future-friendly and backward compatible approach.

Even if 2.x reaches a stable release, the old render mode could be dropped, but we need to help people see what has changed. I love the idea of defaulting to the current display method and then allowing the new method to be turned on when a site owner is ready and had made the appropriate changes.

Hrm, I thought I had already stated I'm 100% in favor of option A in #46. I would prefer that new installs get the new render method, but existing installs get the old render method (set manually via an update hook).

Here's a patch that makes the file rendering method configurable (option A). New sites get the new method, old sites get the old method, and you can change methods on the settings page.

Nice to see consensus arriving here.
I have a remaining niggle in this space - specifically that *although* this patch/fix now mostly uses my displaysuite layout (yay)
- I still get an extra unneccessary 'media-element-container' div, even when my rendered content is just one figure element (that I'd rather style directly).
I understand that it's safer and much more Drupally to always add another div if there is any chance of styling in the future ... but this screws up my widths and floats.

The problem I see right now is that display suite (specifically at least) intercepts the rendering quite late in the process to add its own wrapper. At this point in the code

     if (count(element_children($element['content'])) > 1) {

we don't actually know what the $output will really look like or whether we need to force that wrapper.

FOR NOW (my need today) I'm thinking to introduce a little media_wysiwyg_token_to_markup_alter() to repair this niggle.

But I'm wondering whether we could also retire that count(element_children) logic to the "old way" and trust that the "new way" will always have at least one of its own wrappers around a view_mode.

Committed #49 to 7.x-2.x. Thanks so much @ksenzee and everyone else for helping push this forward.

@dman Could you please file a follow-up issue for your comment so we can address it in a new issue?