There is a regex in media_wysiwyg.filter.js that looks for any img or span tags that have a class of media-element. If found, it then attempts to look up the fid and change this element to a media token. If this process fails, the img or span is then replaced with false, instead of leaving the element untouched. Better error detection in this code would gracefully allow the original image to remain untouched if errors were encountered.

Steps to replicate

  1. Use the WYSIWYG to add a media image to body copy of a node
  2. Save the node
  3. When viewing the node, copy the image by select-dragging over the content of the node, including the image.
  4. Paste the copied content into a new node form with WYSIWYG
  5. Toggle the Disable Rich Text button to view the markup you pasted
  6. Toggle the Enable Rich Text button

At this point, the image will be replaced with false.

CommentFileSizeAuthor
#3 media-2631934-media-element-wysiwyg-false-3.patch1.35 KBq0rban

Comments

q0rban created an issue. See original summary.

q0rban’s picture

q0rban commented
Issue summary: View changes
q0rban’s picture

q0rban commented
Status: Active » Needs review
StatusFileSize
new media-2631934-media-element-wysiwyg-false-3.patch1.35 KB
q0rban’s picture

q0rban commented
Version: 7.x-2.0-beta1 » 7.x-2.x-dev
jantoine’s picture

jantoine commented
Priority: Normal » Critical

This is a regression due to a rewrite of the replacePlaceholderWithToken() function. It was originally discovered and fixed in #2320535: Media 7.x-2.0-alpha2 to 7.x-2.x-dev break existing WYSIWYG content. The function was then rewritten in #2307993: WYSIWYG filter removes script tags causing the fix to be lost. I'm also marking this as critical as it causes data loss when simply saving existing content where match is found but a macro fails to be created.

@q0rban,

I have tested your patch in #3 and it fixes the issue, but I have a question and a suggestion before marking RTBC:

  1. What is the purpose of the change you made to the classRegex variable as it seems to work for me without this change?
  2. Update the comment to provide more contextual information, something like "If we were able to successfully create a macro from the match, store the macro and perform the replacement."
marcoka’s picture

marcoka commented

tested it. fixes my problem.

q0rban’s picture

q0rban commented

Hi jantoine!

The fix to the classRegex variable ensures a proper match. Consider the following code:

<img class="foo'bar baz" />

With the regex as it was before, the only class returned would be foo, because it will match either a double or single closing quote with no verification that it matches the opening quote. The new regex would return foo'bar baz, as it ensures the closing quote matches the opening quote. It's tangentially related to the issue, but you can revert it if you'd rather keep it separate.

Your documentation change on the comment sounds fine to me as well! :)

Thanks for reviewing.

murz’s picture

murz
Primary language English
Location Yerevan, Armenia
commented
Status: Needs review » Reviewed & tested by the community

@q0rban, thanks for the patch! It solves the problem with Media images converts to false after dragging in CKEditor. Please commit it to Media core, because this is critical issue! One image move will break all images in document without any way to restore!!!

marcoka’s picture

marcoka commented

major issue, true. killed a lot of images this way.

othermachines’s picture

othermachines commented

Agree this is critical. We've had to rebuild whole pages from scratch. Patch in #3 seems to have solved the problem for us - thanks!

volker23’s picture

volker23 commented

Patch works like a charm! Thanks for contributing!

  • Dave Reid committed 6785848 on 7.x-2.x authored by q0rban 
    Issue #2631934 by q0rban, jantoine, Murz: Fixed images or spans with the...
dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Priority: Critical » Major
Status: Reviewed & tested by the community » Fixed

Tested and committed #3 to 7.x-2.x. Thanks!

Status: Fixed » Closed (fixed)

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

xaviemirmon’s picture

xaviemirmon
Primary language English
Location Brighton
commented

@q0rban you are a superstar. #3 needs to be released asap