I'd like to propse a change that I believe will be necessary to properly support embedding file entities in text fields using the filter supplied by media_wysiwyg. The problem is the call to menu_get_object in file_entity_is_page. file_entity_is_page is used by many of the routines related to rendering a file entity, but if the file entity is embedded in a text field, then it will cause an infinite loop. The problem could happen for any entity with a text field, but I will use a node as an example. It goes like this:

- A node starts to load to be viewed
- The text field is loaded
- The media_wysiwyg filter applies to the field to set its contents correctly
- The filter attempts to render the embedded file entity
- drupal_render leads to the theme functions in file_entity
- file_entity calls file_entity_is_page
- file_entity_is_page calls menu_get_object
- menu_get_object attempts to load the node (IE back to the first step)

Note that because the markup is meant to be embedded in the content of the text field the file entity must be rendered to load the node, even without attempting to render the node, which is what menu_get_object attempts to do.

Currently the media_wysiwyg module is avoiding this problem by tiptoeing around the need to render the file object directly, but this leads to other problems mentioned in #2194821: Embedded media objects should honor display suite settings. I believe I have a good solution to that problem, but it will require this change to work correctly. I think that any solution to properly render an embedded file entity will require this change, because without it any attempt to render a file entity will call menu_get_object leading to the infinite loop.

(Will post patch)

CommentFileSizeAuthor
#1 file_entity-media_wysiwyg_rendering-2195491-1.patch437 bytesjmuzz

Comments

jmuzz’s picture

jmuzz commented
Status: Active » Needs review
StatusFileSize
new file_entity-media_wysiwyg_rendering-2195491-1.patch437 bytes

I know that the docs for arg() recommend to "Avoid use of this function where possible, as resulting code is hard to read," but given the purpose of file_entity_is_page I think the result is easier to read. I don't think there will be a better solution to the problems mentioned.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

This problem is not unique to files at all. If you had an input format that allowed you to embed nodes, the code in node_view() would give you the same problem.

jmuzz’s picture

jmuzz commented

Yes, and there could be a use case for embedding other entities. I won't try to argue the pros and cons of embedding nodes, or users, or taxonomies or whatever. Being able to properly embed files and have them rendered correctly would be really beneficial though because a lot of people would like to embed images and videos and such in their documents, and having it integrate with a formatting system like display suite (which it can't do without this patch) makes it a lot more flexible and useful.

Without saying this change should be made in other entity types so they could hypothetically be embedded if something else wanted to add such a feature, I think file entities, media, and wysiwyg are seen together often enough right now to make it worth doing here.

justindodge’s picture

justindodge commented
Status: Needs review » Reviewed & tested by the community

I tested the patch in #1 and it successfully resolved my infinite loop issue as a consequence of #2194821: Embedded media objects should honor display suite settings.

I think the code isn't perfect as it doesn't look for objects in the menu path quite the same way that menu_get_item() does, but I'm not sure if there is actually much real world difference between the two and this approach is a very commonly used method of accomplishing this. I think it's safe to say that the position of these URL arguments won't change any time soon.

At any rate, I think it's great improvement and prevents a pretty serious infinite loop issue from occurring in the latest version of media which I think is likely to affect a potentially decent number of folks, so it gets my vote.

I'm not sure if it's too bold to go straight ahead and mark RTBC, but I'm going to try it and see if anyone yells at me.

briand44’s picture

briand44 commented

The patch in #1 resolved the issue for me. Thanks!

  • aaron committed b610413 on 7.x-2.x 
    Issue #2195491 by jmuzz: fixed Change request to support WYSIWYG
aaron’s picture

aaron commented
Status: Reviewed & tested by the community » Fixed

Committed to http://drupalcode.org/project/file_entity.git/commit/b610413.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Fixed » Needs work

I think we will revert this because this is a smell that something else needs to be fixed instead of this code.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Priority: Normal » Critical
justindodge’s picture

justindodge commented

@Dave Reid - I know there is some back tracking going on with other related media commits, but I think this commit was an improvement. I think any time menu_get_object() is used outside of the context of the menu system loading an entity based on the page request, it seems like you invite the possibility of an infinite recursion of loading (which was encountered specifically in #2194821: Embedded media objects should honor display suite settings).

You're probably right about a funkier smell coming from outside the area of this code, but I'm having trouble seeing the downside of leaving it committed but definitely see a potential upside.

Checking the URL path rather than using menu_get_object() to determine this seems like a safer method of determining the same thing that incurs less performance overhead and should be equally reliable.

Just my thought.

justindodge’s picture

justindodge commented
Related issues: +#2194821: Embedded media objects should honor display suite settings
izmeez’s picture

izmeez commented

Is my understanding, this is still committed, correct?

chiebert’s picture

chiebert commented

@izmeez: yes, this is still committed.

devin carlson’s picture

devin carlson commented
Priority: Critical » Normal

Reverted #6, moving back to normal.

  • Devin Carlson committed 812dc89 on 7.x-2.x 
    Revert "Issue #2195491 by jmuzz: fixed Change request to support WYSIWYG...
torotil’s picture

torotil commented

I think the source problem here is that the Drupal core text module renders text formats on entity load. It does so to provide the $item['safe_value'] early on. Let‘s see if there is already a bug report about that …

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

Use the media module and media_wysiwyg
Test drive with media_dev distro

See the quick install recipe link on the media project page

torotil’s picture

torotil commented
Related issues: +#3129651: Infinite load loops: media_filter should not be cacheable

I’ve created #3129651: Infinite load loops: media_filter should not be cacheable with a simple fix to this. Note that in addition to the patch there you need to load/save all text formats using the media_filter.