We have a lot of content created by Views/CCK. After the 7.x-1.26 update (from 7.x-1.25) all nodes are having issues with the body content being rendered as plain text, with all HTML and images etc stripped. Reverting back to 7.x-1.25 fixes the issue. (Running D7.69 w/ all latest modules)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jmouse888 created an issue. See original summary.

DamienMcKenna’s picture

Version: 7.x-1.26 » 7.x-1.x-dev
Issue tags: -views cck +views, +Regression

That's unfortunate, I'm sorry to hear it.

slovv’s picture

A similar problem. In my case, formatting of the text except edited by the administrator disappears and only the database rollback helped.

mattlt’s picture

Hello,

Just confirming the report, and that reverting to 1.25 brings back html for formatting.

I'm available to test solutions if needed.

Thanks,

•• matt

ron_s’s picture

I can confirm this is an issue for us too. 1.26 strips all HTML from the summary and body fields. Reverting back to 1.25 fixes the problem.

We're not using Views to render the fields, but we are using Panels/Panelizer.

ron_s’s picture

Issue tags: +panels, +panelizer

Adding panels and panelizer to the tags.

ron_s’s picture

Component: Views integration » Code
Related issues: +#3063056: Metatag has media browser token in meta description

I see the problem, although I think some code is going to need to be reworked to achieve the original goal.

The issue is with this block of code that was added as part of this patch:
https://www.drupal.org/node/3063056
https://git.drupalcode.org/project/metatag/commit/2c52df6

Specifically, it is this line of code:
https://git.drupalcode.org/project/metatag/blob/7.x-1.x/metatag.inc#L512

When the if(isset($options['token data']['node'])) { block of code was added, there is now a call to tidyValue. The problem is tidyValue runs the strip_tags function: https://git.drupalcode.org/project/metatag/blob/7.x-1.x/metatag.inc#L229

The body is reformatted to strip tags, and then saved back into the token data. Any other rendering after this point is done with the version that has no tags.

I understand what is attempting to be done in Issue #3063056, but the approach being used is not correct. I'm wondering why hook_metatag_pattern_alter can't be used to reach the result @tristiangraves is seeking?

ron_s’s picture

Giving this some thought... maybe you'll disagree, but I think the patch should be rolled back, and I don't believe Metatag should be trying to modify token data.

Instead, @tristiangraves should be using an approach like we do for our sites -- use the Smart Trim module (https://www.drupal.org/project/smart_trim) to selectively choose which tags can be passed into [node:summary].

This does two things: ensures the node summary never contains content that should not be present during display (such as embedded media browser content), and cleans up the summary for use with modules like Metatag.

joekings’s picture

I ran into the same issue. We are using silex to add some code to our templates before final rendering and feed it the page html from the node data. Since I updated metatag at the same time as drupal for a while I thought it was the drupal update that had the bug. It wasn't until I updated a test instance that was I able to find the cause of the issue, the updated metatag plugin.

DamienMcKenna’s picture

steveoriol’s picture

Same problem here, I had to go back to version 1.26

DamienMcKenna’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
1.7 KB

Could someone please explain how to replicate the problem?

I don't agree with needing to require another module (Smart Trim) to fix what is a core problem.

How about we change getValue() to put back the original version of the body string after Metatag is done processing the tokens?

DamienMcKenna’s picture

Doh, sorry about that; this one should work.

steveoriol’s picture

The patch #13 fix the issue :-)
But I had to apply it by hand ...

DamienMcKenna’s picture

Thank you steveoriol.

Could someone else please try the fix, just to confirm it resolves the problem? Thanks.

ron_s’s picture

I will look into it shortly and report my findings.

ron_s’s picture

Status: Needs review » Reviewed & tested by the community

Have tested and patch #13 takes care of the issue. Marking as RTBC.

  • DamienMcKenna committed bc51f50 on 7.x-1.x
    Issue #3102817 by DamienMcKenna, ron_s, steveoriol, jmouse888, mattlt,...
DamienMcKenna’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs tests

Great!

Let's do the test coverage separately, this should be good for now. Thank you.

Committed.

ron_s’s picture

Status: Fixed » Needs work

I'm sorry, just did a load test with this patch and it needs work. If you'd prefer to have this as a new issue task, let me know.

The problem is this block of code:

       // Make sure there is a node to work with.
       if (isset($options['token data']['node'])) {
         // Get language to use for selecting body field value.
         $lang = field_language('node', $options['token data']['node'], 'body');
         if (!empty($options['token data']['node']->body[$lang][0]['value'])) {
           $old_value = $options['token data']['node']->body[$lang][0]['value'];
           // Pre-tidy the node body for token_replace if it's not empty.
           $options['token data']['node']->body[$lang][0]['value'] = $this->tidyValue($old_value);
         }
       }

On a site with a number of panel panes (using Panelizer), it runs this code each time. Having this patch in place increases the page load time from ~2 seconds to around 28 seconds! Rolling it back fixes the issue.

ron_s’s picture

One additional point, I think the cause is the call to media_wysiwyg_filter in tidyValue. Running a trace shows calls to media_wysiwyg_token_to_markup are consuming over 20 seconds of time.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
1014 bytes

Does this help?

ron_s’s picture

General concept makes sense to me, I'll test it out and get back to you.

DamienMcKenna’s picture

Has anyone tested the patch in #22 yet? Does it help?

ron_s’s picture

@Damien, we're going to test it out, just needs to occur during off-peak hours so we can ensure it doesn't cause any issues in the live environments too. If you can be patient I should be able to let you know by tomorrow.

DamienMcKenna’s picture

@ron_s: That'd be great, thank you!

ron_s’s picture

@Damien, this patch is certainly better, but it's still a fairly heavy performance hit until pages are cached. This is for a site with tens of thousands of nodes.

The media_wysiwyg_filter function is being run for every node loaded to a page, even if it's just a view mode which references a node. This function runs media_wysiwyg_token_to_markup, which is a fairly intensive operation.

I can understand why metatag would want it to work this way (need to clean up reference nodes that might be used as related metatags), but it requires a lot of processing. What's most unfortunate is 90% of the nodes on a given page have nothing to do with metatags, yet it's still being forced to run the media_wysiwyg_filter function via tidyValue.

Have been watching as new pages are hit before being cached, and we're easily seeing a 15-20% increase in load time for the first rendering.

DamienMcKenna’s picture

Status: Needs review » Fixed

Let's split this off into a separate issue so we don't overload this one with performance discussions that affect a subset of sites: #3107151: Performance problems from entity pre-rendering for summary token

ron_s’s picture

Sounds good, thanks.

Anybody’s picture

Hi and thank you all very much, is it possible to create a new stable release which fixes the critical issue in the current stable?

Status: Fixed » Closed (fixed)

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