Instead of:

$description_build = [];
// ...
 
if ($display_mode != 'title') {
  // We render node contents.
  $description_build = $build;
}

Try:

// We render node contents.
$description_build = ($display_mode != 'title') ? $build : [];
Files: 
CommentFileSizeAuthor
#7 remove_safemarkup_set-2501949-7.patch919 bytescdulude
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,164 pass(es). View
#2 remove_safemarkup_set-2501949-2.patch540 bytescdulude
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,707 pass(es). View

Comments

cdulude’s picture

Issue summary: View changes
cdulude’s picture

FileSize
540 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,707 pass(es). View

Removed SafeMarkup::set(). Not sure if this needs a t() or not?

$item->description = SafeMarkup::set($item_text);
$item->description = $item_text;
cdulude’s picture

Status: Active » Needs review
Cottser’s picture

@cdulude thanks! As far as I know no need for t() here, anything that needed to be translated would have happened much earlier than this.

joelpittet’s picture

This seems like it should work since it's concatenating only an empty string on to the drupal_render_root() output althought it may be worth to bring that content more inline by getting the $item_text initialize down to something like this:

    $item_text = ($display_mode != 'title') ? drupal_render_root($build) : '';

What do you think @cdulude?

My thought is that way someone won't try to stick some more concatination in the middle and break that safe markup.

joelpittet’s picture

Title: Remove or document SafeMarkup::set in Rss render() » Remove SafeMarkup::set in Rss render()
cdulude’s picture

FileSize
919 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,164 pass(es). View

@joelpittet Ah! That would be more efficient. I just did a fresh git clone, and it looks like the code for that section has changed slightly since yesterday. I just rolled a new patch that will hopefully do the trick!

joelpittet’s picture

Priority: Major » Normal
Issue tags: -SafeMarkup, -Twig, -D8 Accelerate +Needs issue summary update

@cdulude someone must have resolved what this issue was meant to do. Maybe you want to re-purpose it for this cleanup?

Not sure it will get in but we can still try if you're up for it?

cdulude’s picture

Sure! Is this just a matter of changing the title and summary of this issue to say something like "Condense code into ternary operator for readability," rather than the SafeMarkup::set() title and description? And just leave the patch as is? Or, is there something additional I should do? (i.e. start a brand new issue?)

joelpittet’s picture

I think that would be it, just need to gut the issue summary, change the title. Probably doesn't need a new issue. May need to demote to minor, not sure.

cilefen’s picture

Title: Remove SafeMarkup::set in Rss render() » Use the ternary operator for $description_build in Drupal\node\Plugin\views\row\RSS::render()
Priority: Normal » Minor
Issue summary: View changes
Issue tags: -Needs issue summary update +Novice
Parent issue: #2280965: [meta] Remove every SafeMarkup::set() call »
jcloys’s picture

I am going to review

jcloys’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

I am new to this and it looks like a simple code clean up. I understand that this is supposed to output the node description when looking in the RSS file and the code looks good. We looked around for code coverage, particularly: RowPluginTest and NodeRSSContentTest and realized there might not be any test coverage for this. I am going to mark this as "needs work" and add the "needs test" tag so someone can verify if there is an existing test or we can add an assert in NodeRSSContentTest.

davidhernandez’s picture

Issue tags: +D8 Accelerate
joelpittet’s picture

@jcloys Would you like to write a test for that?

tomhollevoet’s picture

Assigned: cdulude » tomhollevoet
tomhollevoet’s picture

tomhollevoet’s picture

Issue tags: +DUGBE0609
mgifford’s picture

Assigned: tomhollevoet » Unassigned

@tomhollevoet - feel free to re-assign it to yourself when you've got time to work on it.

snehi’s picture

Status: Needs work » Reviewed & tested by the community

Worked for me.
RTBC Now

joelpittet’s picture

Issue tags: -Needs tests, -D8 Accelerate

Don't think this needs tests, it's RTBC. It's really just a clean-up and would be tempted to just have it committed for D8 during RC as "documentation" as it's really just makes the code clearer to read by moving the variable close to where it's used.

joelpittet’s picture

Status: Reviewed & tested by the community » Postponed

We are going to postpone this till after the release but it can go in to something like 8.0.1

joelpittet’s picture

Status: Postponed » Reviewed & tested by the community
Issue tags: +API clean-up, +theme system cleanup

alexpott’s picture

Status: Reviewed & tested by the community » Closed (won't fix)

This has already been fixed in a slightly different way by #2500931: Views feed doesn't encode embedded HTML anymore