Follow-up to #2501639: Remove SafeMarkup::set in drupal_check_module()

Problem/Motivation

rdf_preprocess_comment() calls SafeMarkup::set() which is meant to be for internal use only.

Proposed resolution

Move markup created inside PHP into template.

Remaining tasks

  1. ✔ Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123
  2. ✔ Manual testing and screenshots
  3. ✔ Identify whether there is existing automated test coverage for the sanitization of the string. If there is, list the test in the issue summary. If there isn't, add an automated test for it.

Manual testing steps (for XSS and double escaping)

Do these steps both with HEAD and with the patch applied:

  1. Clean install of Drupal 8.
  2. Enable RDF and comment modules (already enabled in standard install)
  3. Add a comment to a node (article)
  4. Find the RDF markup next to author and submitted
  5. make sure it's in the output from comment.html.twig, twig_debug is helpful for verifying
  6. Compare the output above in HEAD and with the patch applied. Confirm that there is no double-escaping.

Screenshot before patch:

Screenshot after patch:

User interface changes

N/A

API changes

N/A

CommentFileSizeAuthor
#109 remove_safemarkup_set-2501697-109.patch4.61 KBjoelpittet
#109 interdiff.txt1.03 KBjoelpittet
#108 remove_safemarkup_set-2501697-107.patch4.62 KBZenDoodles
#106 interdiff-2501697-106.txt1.14 KBZenDoodles
#106 remove_safemarkup_set-2501697-106.patch4.65 KBZenDoodles
#102 remove_safemarkup_set-2501697-85_94_102.patch3.48 KBYesCT
#100 remove_safemarkup_set-2501697-100.patch3.51 KBZenDoodles
#100 interdiff-2501697-100.txt419 bytesZenDoodles
#95 remove_safemarkup_set-2501697-85_94.patch3.48 KBscor
#90 remove_safemarkup_set-FAILS-2501697-90.patch3.77 KBZenDoodles
#89 interdiff-2501697-89.txt1.89 KBZenDoodles
#89 remove_safemarkup_set-Test_Fail-2501697-89.patch34.03 KBZenDoodles
#86 remove_safemarkup_set-2501697-85.patch3.48 KBZenDoodles
#86 interdiff.txt1.33 KBZenDoodles
#78 Screenshot_8_7_15__3_40_PM.png155 KBjday
#71 remove_safemarkup_set-2501697-71.patch3.48 KBjoelpittet
#55 core-2501697-55-safemarkup_set_rdf_preprocess_comment.patch2.2 KBLes Lim
#55 interdiff-2501697-40-55.txt1011 bytesLes Lim
#51 post-patch-spacing.png59.7 KBhestenet
#51 d8-HEAD-pre-patch.png39.87 KBhestenet
#51 2501697-51.patch2.3 KBhestenet
#40 interdiff.txt1.81 KBpfrenssen
#40 2501697-40.patch2.31 KBpfrenssen
#36 interdiff.txt1.43 KBpfrenssen
#36 2501697-36.patch2.24 KBpfrenssen
#20 remove_safemarkup_set-2501697-19.patch2.36 KBleslieg
#18 remove_safemarkup_set-2501697-17.patch3.33 KBleslieg
#18 interdiff.txt1.23 KBleslieg
#17 rdf-after 15-06-06, 9.16.19 PM.png532.83 KBstar-szr
#17 rdf-before 15-06-06, 9.12.57 PM.png422.24 KBstar-szr
#14 remove_safemarkup_set-2501697-14.patch1.23 KBleslieg
#12 view-source_drupaltwig_dd_8083_node_1_comment-2.png401.41 KBleslieg
#12 view-source_drupaltwig_dd_8083_node_1_comment-1.png444.26 KBleslieg
#7 remove_safemarkup_set-2501697-7.patch2.36 KBleslieg
#5 remove_safemarkup_set-2501697-5.patch2.78 KBleslieg
#3 remove_safemarkup_set-2501697-3.patch2.59 KBleslieg
#60 core-2501697-60-safemarkup_set_rdf_preprocess_comment.patch2.73 KBLes Lim
#60 interdiff-2501697-55-60.txt537 bytesLes Lim
#65 interdiff-2501697-60-65.txt1.24 KBLes Lim
#65 core-2501697-65-safemarkup_set_rdf_preprocess_comment.patch2.28 KBLes Lim
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

leslieg’s picture

working on this as part of NHDevDays2 sprint

leslieg’s picture

Title: Remove SafeMarkup::set in rdf.module » Remove SafeMarkup::set in rdf_preprocess_comment
Issue summary: View changes
leslieg’s picture

leslieg’s picture

Status: Active » Needs review

patch to change safemarkup::set to safemarkup::format attached

leslieg’s picture

fixed formatting issues with patch to change safemarkup::set to safemarkup::fromat

Status: Needs review » Needs work

The last submitted patch, 5: remove_safemarkup_set-2501697-5.patch, failed testing.

leslieg’s picture

leslieg’s picture

Status: Needs work » Needs review
lokapujya’s picture

Status: Needs review » Reviewed & tested by the community

Good to go. Expecting this to be green.

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs manual testing

Thanks @lokapujya and @leslieg!

On a skim this seems to make sense, but can we document some steps for manual testing and have someone run through them before/after?

leslieg’s picture

Issue summary: View changes

Added manaul test steps

leslieg’s picture

star-szr’s picture

Status: Needs review » Needs work
+++ b/core/modules/rdf/rdf.module
@@ -440,11 +440,11 @@ function rdf_preprocess_comment(&$variables) {
+    $variables['author'] = SafeMarkup::format('<span@author_attributes>@author</span>', ['@author_attributes' => $author_attributes,'@author' => $variables['author']]);
+    $variables['submitted'] = SafeMarkup::format('<span@author_attributes>@submitted</span>', ['@author_attributes' => $author_attributes,'@submitted' => $variables['submitted']]);

These two lines both need a space after the final comma in the array per https://www.drupal.org/coding-standards#array.

leslieg’s picture

Made changes suggested by @cottser to comply with coding standards

leslieg’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 14: remove_safemarkup_set-2501697-14.patch, failed testing.

star-szr’s picture

I manually tested #7, @leslieg is uploading a new patch presently that fixes the coding standards but there are no other changes.

This patch actually improves things, I'm updating the issue summary with new screenshots showing this in action and updated the manual testing steps to be more specific.

leslieg’s picture

Status: Needs work » Needs review
FileSize
1.23 KB
3.33 KB

recreated patch - fixed issue with diff not using 8.0.x

Status: Needs review » Needs work

The last submitted patch, 18: remove_safemarkup_set-2501697-17.patch, failed testing.

leslieg’s picture

Status: Needs work » Needs review
FileSize
2.36 KB

Inadvertantly includes another file's changes

star-szr’s picture

Title: Remove SafeMarkup::set in rdf_preprocess_comment » Remove SafeMarkup::set in rdf_preprocess_comment()
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Much better! This is ready to go.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 20: remove_safemarkup_set-2501697-19.patch, failed testing.

Status: Needs work » Needs review
cilefen’s picture

Status: Needs review » Reviewed & tested by the community

I think there was a random test failure. I am putting this back to RTBC based on #21.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 20: remove_safemarkup_set-2501697-19.patch, failed testing.

Status: Needs work » Needs review
akalata’s picture

Status: Needs review » Reviewed & tested by the community

One of the testbots got twitchy this morning. Setting back to RTBC.

xjm’s picture

Assigned: Unassigned » xjm

Assigning to myself for additional feedback, but leaving at RTBC for now.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 20: remove_safemarkup_set-2501697-19.patch, failed testing.

Status: Needs work » Needs review
joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Failed to checkout bogus. back to RTBC

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/rdf/rdf.module
@@ -440,11 +440,11 @@ function rdf_preprocess_comment(&$variables) {
+    $variables['author'] = SafeMarkup::format('<span@author_attributes>@author</span>', ['@author_attributes' => $author_attributes, '@author' => $variables['author']]);
+    $variables['submitted'] = SafeMarkup::format('<span@author_attributes>@submitted</span>', ['@author_attributes' => $author_attributes, '@submitted' => $variables['submitted']]);

Given that we're building render arrays are this point we could just use the #type => 'html_tag'.

So something like

$variables['author'] = [
  '#type' => 'html_tag',
  '#tag' => 'span',
  '#attributes' => [
    'rel' => $author_mapping['properties'],
  ],
  '#value' => $variables['author'],
];
alexpott’s picture

Assigned: xjm » Unassigned
xjm’s picture

Agreed, thanks @alexpott.

pfrenssen’s picture

Assigned: Unassigned » pfrenssen
Issue tags: +D8 Accelerate London

Going to address the remarks as part of the D8 Accelerate sprint.

pfrenssen’s picture

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 36: 2501697-36.patch, failed testing.

pfrenssen’s picture

Assigned: Unassigned » pfrenssen

Thanks bot for reinforcing the notion that manual testing is essential :)

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
FileSize
2.31 KB
1.81 KB

Fixed failures, and as a bonus we now got rid of a deprecated call to drupal_render().

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Tested this manually on the default frontpage and node page and it seems to be working. Patch looks also sane.

star-szr’s picture

There was another issue to remove html_tag (I think it was a @catch issue) but probably much too late by this point. Only thing is I think the #value line is missing a trailing comma.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Nice, this is looking much improved.

+++ b/core/modules/rdf/rdf.module
@@ -440,11 +440,16 @@ function rdf_preprocess_comment(&$variables) {
+    // Wraps the 'author' and 'submitted' variables which are both available in
+    // comment.html.twig.
+    foreach (['author', 'submitted'] as $variable) {
+      $variables[$variable] = [
+        '#type' => 'html_tag',
+        '#tag' => 'span',
+        '#attributes' => ['rel' => $author_mapping['properties']],
+        '#value' => $variables[$variable]

@@ -457,11 +462,15 @@ function rdf_preprocess_comment(&$variables) {
+    foreach (['created', 'submitted'] as $variable) {
+      $variables[$variable] = [
+        // Ensure the original variable is represented as a render array.
+        !is_array($variables[$variable]) ? ['#markup' => $variables[$variable]] : $variables[$variable],
+        $rdf_metadata,
+      ];

I think we should also have an automated test here to assert the variables are sanitized on render for XSS and not double-escaped (since they can contain markup I think). Particularly since this is being output to RDF and not HTML. (Sorry for not mentioning this before.)

If anyone can find a link to the issue @Cottser is mentioning that would also be helpful to review/reference at least here in the issue.

lauriii’s picture

It was at the IS but someone else has marked that we don't need it so didn't spend time on reviewing that.

star-szr’s picture

Hmm maybe it was just an issue comment somewhere. I can't find it. May also have dreamt it.

star-szr’s picture

I may be thinking of this from the issue summary on #1825952: Turn on twig autoescape by default, but I think these are @chx's words above the "Original report by @catch" :)

You are advised to not use #type => html_tag if at all possible or at least not with unsafe user data. This is not something I want to waste an effort on making it work.

chx’s picture

Even if I didn't write that I strongly agree with it :D

pwolanin’s picture

So, this is to add RDF tags into the HTML so the output is HTML.

given the new filtering in #markup, probably we could just convert these to be of type #markup as an easy fix?

pwolanin’s picture

hestenet’s picture

I am going to try @pwolanin's suggestion in comment #48

hestenet’s picture

Tried my rather naive edit based on comment #48. Seems to apply cleanly at least - but someone with much more experience should determine whether this is safe and probably pick it up from here:

Source from HEAD:

d8-Head-pre-patch

Source with patch:

d8-post-patch

star-szr’s picture

To follow up #46 I think this is the comment from @catch I was thinking of:

#2296101-7: Remove SafeMarkup::set() use in \Drupal\Core\Render\Element\HtmlTag::preRenderHtmlTag()

joelpittet’s picture

Status: Needs work » Needs review

@hestenet thanks I think that looks pretty good at first glance. A couple of things.

Also setting status to needs review so testbot can take a crack at it.

  1. +++ b/core/modules/rdf/rdf.module
    @@ -440,11 +440,16 @@ function rdf_preprocess_comment(&$variables) {
    +        '#type' => 'markup',
    

    I think it's 'type' => 'html_tag'. But close.

  2. +++ b/core/modules/rdf/rdf.module
    @@ -440,11 +440,16 @@ function rdf_preprocess_comment(&$variables) {
    +        '#value' => $variables[$variable]
    

    nit: Needs a comma at the end of all elements listed on a new line.

  3. +++ b/core/modules/rdf/rdf.module
    @@ -457,11 +462,15 @@ function rdf_preprocess_comment(&$variables) {
    +      $variables[$variable] = [
    +        // Ensure the original variable is represented as a render array.
    +        !is_array($variables[$variable]) ? ['#markup' => $variables[$variable]] : $variables[$variable],
    +        $rdf_metadata,
    +      ];
    

    This looks like it should work, just needs to be tested to confirm.

Status: Needs review » Needs work

The last submitted patch, 51: 2501697-51.patch, failed testing.

Les Lim’s picture

Status: Needs work » Needs review
FileSize
2.2 KB
1011 bytes

#51 isn't quite the way to use #markup -- attaching a new patch based off of #40.

Re #53:

I think it's 'type' => 'html_tag'. But close.

This patch implements @pwolanin's suggestion in #48 to specifically not use 'html_tag', opting instead for '#markup' which applies Xss::filterAdmin() automatically.

The last submitted patch, 55: core-2501697-55-safemarkup_set_rdf_preprocess_comment.patch, failed testing.

Les Lim’s picture

Status: Needs review » Needs work

Shoot, using #markup as in #55 isn't going to work. Xss:filterAdmin() doesn't like attributes with ":" characters in them, so:

<span rel="schema:author"></span>

is being stripped down to:

<span rel="author"></span>

YesCT’s picture

can we whitelist the rdf keywords?

Les Lim’s picture

@YesCT: maybe? It would probably be easier and more future-proof to whitelist the "rel" attribute, to exempt it from protocol stripping. Patch coming.

Les Lim’s picture

joelpittet’s picture

Status: Needs work » Needs review

Go go gadget tests!

Status: Needs review » Needs work

The last submitted patch, 60: core-2501697-60-safemarkup_set_rdf_preprocess_comment.patch, failed testing.

dsnopek’s picture

Do we know for sure that $variables['author'] and $variables['submitted'] are safe? Maybe it would be better to use an inline template so that those will get filtered if they aren't already safe?

Les Lim’s picture

The failures in #60 are because we also need to whitelist the "typeof" attribute. or maybe not. Hm.

Working on a new patch to use inline template per #63.

Les Lim’s picture

I'm not really comfortable with the patch in #60 touching Xss::filter() internals, especially since it would have to whitelist several attributes to cover the full set used for RDF. I've opened #2544110: XSS attribute filtering is inconsistent and strips valid attributes to talk about the XSS filter separately.

This patch follows dsnopek's suggestion in #63 to use inline templates (instead of #markup with its implicit XSS filtering).

The inline template presumes that new Attribute($author_attributes) is already sanitized. The Attribute class is supposed to take care of that, but we've seen that it does not strip potentially dangerous protocols like javascript:, which is why it allows for valid RDF values like "schema:author". But I also can't think of a way to exploit that in this case.

YesCT’s picture

Status: Needs review » Needs work

@Les Lim thanks for implementing the inline templates.

sounds like we should have some code comments explaining
what you said:
The inline template presumes that new Attribute($author_attributes) is already sanitized.

and any implications of that? Like how there is no way to exploit it...

Do we still need tests here? per #43
" to assert the variables are sanitized on render for XSS and not double-escaped (since they can contain markup I think). Particularly since this is being output to RDF and not HTML."
probably yes, asserting we are santitizing (where we are) and not (where we are not... and explain why it is ok to let things through)

dsnopek’s picture

The inline template presumes that new Attribute($author_attributes) is already sanitized. The Attribute class is supposed to take care of that, but we've seen that it does not strip potentially dangerous protocols like javascript:, which is why it allows for valid RDF values like "schema:author". But I also can't think of a way to exploit that in this case.

I think this is because it's concatenating the attributes directly into the template, rather than using Twig to expand them!

So, rather than:

+++ b/core/modules/rdf/rdf.module
@@ -441,10 +441,17 @@ function rdf_preprocess_comment(&$variables) {
+        '#template' => '<span' . new Attribute($author_attributes) . '>{{ var }}</span>',
+        '#context' => array(
+          'var' => $variables[$variable],
+        ),

... this should do something like:

  '#template' => '<span {{ attrs }}>{{ var }}</span>',
  '#context' => array(
    'var' => $variables[$variable],
    'attrs' => new Attribute($author_attributes),
  ),

However, this might not have the nice property of not filtering out those RDF properties. :-/ But it's the right thing to do from a security and Twig perspective! Concatenating data directly into the inline template is definitely wrong - it's as if that data were part of the code in the template written by the themer in a non-inline template.

xjm’s picture

The purpose of Attribute() is to do sanitization of attributes, so it always makes safe attributes when we use it HTML tags for attributes. So that's fine either way. We have used this in previous issues, for example, in #2501705: Remove SafeMarkup::set() in LinkGenerator and document SafeMarkup::set() in LinkGeneratorTest. (Using inline templates in this issue is advantageous because it allows us to do late rendering instead of the early rendering needed for links.)

What @dsnopek suggests is probably fine too if we use an inline template here. However, the code flow is very hard to follow now with the foreach. There are three variables: 'author', 'created', and 'submitted'. 'author' should only be processed when $author_mapping is set. 'created' should only be processed when $created_mapping is set. 'submitted' should be processed when either one or the other is set. I think we could make the code flow easier to understand by handling each case separately.

I will say that I'm a bit sad this issue got reworked to avoid html_tag based on a third-hand comment (like is there even an issue for that? and it certainly can't happen in D8 AFAIK... that'd be an enormous BC break that's hard to justify now and I think not at all feasible after RC). On the other hand, the inline template is more relevant for this issue than other issues where there has been pushback about its use, because the goal of this code is specifically to assemble markup. (It is a lot of boilerplate though still.)

xjm’s picture

On the other other hand, @alexpott and I also discussed that it might just be good to make this an actual template. We have simpler templates than this in core too.

dsnopek’s picture

@xjm: Ah, ok, thanks! I still don't think we shouldn't concatenate directly into an inline template, if this stays being an inline template, though.

There's at least one issue about removing html_tag: #2544318: Remove #type => html_tag usages

However, it's newer than the comment in #42

joelpittet’s picture

Status: Needs work » Needs review
FileSize
3.48 KB

I hope I didn't misconstrued anything in #68 or #69 but here's how I think that would be implemented.

No interdiff because it's quite different approach.

dsnopek’s picture

Not that this issue needs any more uncertainty, suggestions for approach or off-hand comments, but... :-)

I think what @xjm said in #68 about using html_tag should be considered. Given the simplicity of the template or inline template it would seem like a good match? And while there is at least one issue for removing html_tag, like @xjm said:

... it certainly can't happen in D8 AFAIK... that'd be an enormous BC break that's hard to justify now and I think not at all feasible after RC

However, I don't want to derail current work even further than I have already, so feel free to ignore this. :-)

But I think it might be good to have a sub-system maintainer or committer review the issue and at least dictate the approach (is it #type => markup, inline template, real template, or html_tag?) so that work can go forward and prevent more changes of direction.

joelpittet’s picture

@dsnopek I'm for anything that get this past the core gates:) I have no problem with using html_tag to solve the problem.

And I am one of the sub-system maintainers for the theme system;) and @xjm is a committer, but maybe we should get another set of eyes?

We are trying to go to templates first, second inline-templates/renderable arrays, and #markup should be avoided if something is more appropriate. So to dictate a bit: #type => html_tag would be preferable over #markup.

lauriii’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

I think between using #type => html_tag and templates I would prefer using real template whenever it makes any sense. Moving all the markup from PHP to templates is quite ideal because then we don't have to deal with autoescaping. Also in this case I wonder why this markup even was in the PHP because it might be something that people want to override easily and after moving it to template its trivial.

Not much to do with this patch but should we open a follow-up to start using the template created here in other places creating RDF spans?

I did also test the latest patch manually and I saw the RDF spans showing up normally in comments :)

Patch is RTBC for me but the tests needs still some figuring out.

lauriii’s picture

And because this template doesn't have any classes inside it, it doesn't have to be copied inside the module. Also the attributes variable and span is needed for functional reasons so they shouldn't be removed from the module.

joelpittet’s picture

Assigned: Unassigned » joelpittet

Assigning to myself for writing tests. (discussed on twig call)

acouch’s picture

Assigned: joelpittet » acouch

Working with jday at Madison hackathon and cat story sharathon

jday’s picture

Deleting this comment, error I was reporting was in fact due to a poorly applied patch, i.e. user error

xjm’s picture

Re: #78, I wasn't able to reproduce this, but @jday and I figured out it was an issue with the way the patch was applied, so @jday will retest using git.

acouch’s picture

Per chat I will work on this this weekend and 1) work on trying to break the current test 2) add assertLink and check the output for the author and submitted

akalata’s picture

Chiming in with some backstory on the approach to avoid #type => html_tag. dawehner had suggested this in #2296101-4: Remove SafeMarkup::set() use in \Drupal\Core\Render\Element\HtmlTag::preRenderHtmlTag(), with Catch agreeing.

  • The SafeMarkup work on HtmlTag::preRenderHtmlTag() has lead to long discussions about what input is sanitized where; using a template makes that much more clear.
  • #type => html_tag is used primarily for html <head> elements, or in tests. The other uses in core could easily be converted into templates. So even if it is not removed completely, we can scale back its usage, at least in core. Discussion at #2544318: Remove #type => html_tag usages welcome!
andypost’s picture

Why we add extra span for rdf attributes? suppose parent element (field template) should be re-used

joelpittet’s picture

Issue tags: +RDF

@andypost I believe the answer to that is related to not knowing if the parent element is being used in the template but that would be a good question for @scor to answer because he'd know better than I.

Wim Leers’s picture

This patch looks beautiful.


Only nits:

  1. +++ b/core/modules/rdf/rdf.module
    @@ -457,11 +468,13 @@ function rdf_preprocess_comment(&$variables) {
    +    // Append RDF metadata to elements which are both available
    +    // in comment.html.twig.
    +    $variables['created'] = [$created, $rdf_metadata];
    +    $variables['submitted'] = [$submitted, $rdf_metadata];
    

    This is not really appending data to "elements" (I am interpreting "elements" as "render elements"), it's just creating a pair of [$render_array, $rdf_metadata]. That's not appending.

    Not sure what the best comment here is, but this is IMO fairly confusing.

  2. +++ b/core/modules/rdf/templates/rdf-wrapper.html.twig
    @@ -0,0 +1,13 @@
    + * - attributes: HTML attributes for the containing element.
    

    s/element./, including RDF attributes./

ZenDoodles’s picture

Assigned: acouch » ZenDoodles

Mine!

ZenDoodles’s picture

Updated based on feedback in #84.

Wim Leers’s picture

Looks great! But this still needs tests AFAICT. I think somebody who was more involved in this issue would be better able to RTBC this.

ZenDoodles’s picture

Assigned: Unassigned » ZenDoodles
Status: Needs review » Needs work

I'll write tests.

ZenDoodles’s picture

Actually... SimpleTests already exist.

Attached patch is how I broke it to demonstrate. I wrapped everything in check_plain() to improperly escape. Tests will fail.

ZenDoodles’s picture

Hah... always read the patch with your eyeballs first. This one is the real one.

The last submitted patch, 89: remove_safemarkup_set-Test_Fail-2501697-89.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 90: remove_safemarkup_set-FAILS-2501697-90.patch, failed testing.

ZenDoodles’s picture

Assigned: ZenDoodles » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests

Since tests fail as expected when the elements are improperly escaped, the patch above demonstrates there are already tests.

The patch in #86 is ready for review again.

ZenDoodles’s picture

@andypost, Re: #82... I believe the your concern is out-of-scope. This patch does not add a wrapper. It just moves it to a template where it belongs.

scor’s picture

Why we add extra span for rdf attributes?

@joelpittet is right of course. at the time when this code was written, there was no way to add attribute to the parent element from within the preprocess function. Now this has changed, and this markup will improve with #2214241: Field default markup - removing the divitis.

Re-uploading latest working patch to avoid having to scroll up (no credit please).

joelpittet’s picture

YesCT’s picture

I'm going to review this.

YesCT’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/rdf/rdf.module
    @@ -440,11 +443,19 @@ function rdf_preprocess_comment(&$variables) {
    +    // Wraps the 'author' and 'submitted' variables which are both available in
    +    // comment.html.twig.
    

    Make render arrays that wrap the 'author' and 'submitted' RDF metadata. They will be available variables in comment.html.twig.

  2. +++ b/core/modules/rdf/rdf.module
    @@ -457,11 +468,12 @@ function rdf_preprocess_comment(&$variables) {
    +    // Make render array and RDF metadata available in comment.html.twig.
    

    I think might be more accurate.

    Make the render arrays for RDF metadata available in comment.html.twig.

other than those clarifications, I think this looks ready.

It has been a while since we have done manual testing. From reading the patch, the markup *should* be exactly the same. I'll just confirm that next.

YesCT’s picture

Issue summary: View changes

1. Discussed this with @ZenDoodles in person, and after learning a bit more about this, I think those comments are fine. No need to change them.

2. manual testing

before

<article role="article" data-quickedit-entity-id="comment/1" data-comment-user-id="1" about="/drupal/comment/1" typeof="schema:Comment" class="comment js-comment by-node-author clearfix">
    <span class="hidden" data-comment-timestamp="1439487389"></span>

  <footer class="comment__meta">
    <article data-quickedit-entity-id="user/1" typeof="schema:Person" about="/drupal/user/1" class="profile">
  </article>

    <p class="comment__author"><span  rel="schema:author"><a title="View user profile." href="/drupal/user/1" lang="" about="/drupal/user/1" typeof="schema:Person" property="schema:name" datatype="" class="username">admin</a></span></p>
    <p class="comment__time">Thu, 08/13/2015 - 19:36  <span property="schema:dateCreated" content="2015-08-13T19:36:29+02:00" class="rdf-meta hidden"></span>
</p>
    <p class="comment__permalink"><a href="/drupal/node/1#comment-1" hreflang="en">Permalink</a></p>
          </footer>

  <div class="comment__content">
          
      <h3 property="schema:name" datatype=""><a href="/drupal/comment/1" class="permalink" rel="bookmark" hreflang="en">my comment</a></h3>
      
        <div data-quickedit-field-id="comment/1/comment_body/en/full" class="clearfix text-formatted field field-comment--comment-body field-name-comment-body field-type-text-long field-label-hidden">
    <div class="field-items">
          <div property="schema:text" class="field-item"><p>the comment</p></div>
      </div>
</div>

          <nav><ul class="links inline"><li class="comment-delete"><a href="/drupal/comment/1/delete" hreflang="en">Delete</a></li><li class="comment-edit"><a href="/drupal/comment/1/edit" hreflang="en">Edit</a></li><li class="comment-reply"><a href="/drupal/comment/reply/node/1/comment/1">Reply</a></li></ul></nav>
      </div>
</article>

after

<article role="article" data-quickedit-entity-id="comment/1" data-comment-user-id="1" about="/drupal/comment/1" typeof="schema:Comment" class="comment js-comment by-node-author clearfix">
    <span class="hidden" data-comment-timestamp="1439487397"></span>

  <footer class="comment__meta">
    <article data-quickedit-entity-id="user/1" typeof="schema:Person" about="/drupal/user/1" class="profile">
  </article>

    <p class="comment__author"><span rel="schema:author"><a title="View user profile." href="/drupal/user/1" lang="" about="/drupal/user/1" typeof="schema:Person" property="schema:name" datatype="" class="username">admin</a></span>
</p>
    <p class="comment__time">Thu, 08/13/2015 - 19:36  <span property="schema:dateCreated" content="2015-08-13T19:36:37+02:00" class="rdf-meta hidden"></span>
</p>
    <p class="comment__permalink"><a href="/drupal/node/1#comment-1" hreflang="en">Permalink</a></p>
          </footer>

  <div class="comment__content">

      <h3 property="schema:name" datatype=""><a href="/drupal/comment/1" class="permalink" rel="bookmark" hreflang="en">my comment</a></h3>

        <div data-quickedit-field-id="comment/1/comment_body/en/full" class="clearfix text-formatted field field-comment--comment-body field-name-comment-body field-type-text-long field-label-hidden">
    <div class="field-items">
          <div property="schema:text" class="field-item"><p>the comment</p></div>
      </div>
</div>

          <nav><ul class="links inline"><li class="comment-delete"><a href="/drupal/comment/1/delete" hreflang="en">Delete</a></li><li class="comment-edit"><a href="/drupal/comment/1/edit" hreflang="en">Edit</a></li><li class="comment-reply"><a href="/drupal/comment/reply/node/1/comment/1">Reply</a></li></ul></nav>
      </div>
</article>

there are two differences,
a. the double space in the span for rel in head is fixed in the patch to just be one space.
b. there is a newline after the patch before the comment__author p close tag
--

also confirmed the new template core/modules/rdf/templates/rdf-wrapper.html.twig is being used, for example:

    <p class="comment__author">

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'rdf_wrapper' -->
<!-- BEGIN OUTPUT from 'core/modules/rdf/templates/rdf-wrapper.html.twig' -->
<span rel="schema:author">

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'username' -->
<!-- BEGIN OUTPUT from 'core/themes/classy/templates/user/username.html.twig' -->
<a title="View user profile." href="/drupal2/user/1" lang="" about="/drupal2/user/1" typeof="schema:Person" property="schema:name" datatype="" class="username">admin</a>
<!-- END OUTPUT from 'core/themes/classy/templates/user/username.html.twig' -->

</span>

(and that comment.html.twig is still being used)

<!-- END OUTPUT from 'core/modules/rdf/templates/rdf-wrapper.html.twig' -->

3. updating the remaining tasks in the summary (to note they are done)

====

aside from the extra newline, I think this is rtbc.

ZenDoodles’s picture

Status: Needs work » Needs review
FileSize
419 bytes
3.51 KB

You're killing me Smalls!

joelpittet’s picture

We follow git settings. Normally we don't care about whitespaces, but we do care for inline elements (which this using). BUT this inline element is for robots, not people, it doesn't display to anybody.

It's not just coding standards, it's git too. It will show as "No newline at end of file" in every single diff.

Here's where we are discussing this.
#2245901: Trim trailing EOF whitespace from templates

So don't do this removal because of robots:)

YesCT’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
3.48 KB

ok. that makes sense. so that makes 85_94 the one to commit. reuploading it so it is the most recent patch on the issue (again).

The last submitted patch, 100: remove_safemarkup_set-2501697-100.patch, failed testing.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests
+++ b/core/modules/rdf/rdf.module
@@ -457,11 +468,12 @@ function rdf_preprocess_comment(&$variables) {
+    // Ensure the original variable is represented as a render array.
+    $created = !is_array($variables['created']) ? ['#markup' => $variables['created']] : $variables['created'];
+    $submitted = !is_array($variables['submitted']) ? ['#markup' => $variables['submitted']] : $variables['submitted'];

Reading this, I questioned what happened if $variables['created'] or $variables['submitted'] weren't set at all, but I confirmed they are both always set in template_preprocess_comment().

Also, as far as I can tell, in HEAD, $variables['created'] would never be a render array; it is only ever set to a string. So the !is_array() check is not actually necessary. However, it makes sense to support it being a render array going forward for consistency, so I think it makes sense to do this.

I also read the great saga of the final template newline and agree with retaining it here since it doesn't actually negatively affect the presentation in this case. (However, YesCT++ for the careful testing to identify the potential issue in the first place.)

So all the above things are fine. However, after carefully inspecting @ZenDoodles' test results in #100, I realized it isn't actually failing any assertions for the correct markup contents of the span tag. So I think we need to add one short additional test method to CommentsAttributesTest that saves a comment using the existing save method in the test and then asserts that the author name is a link for the authenticated user. Should only be a couple lines long.

Thanks everyone for your careful review of this issue!

ZenDoodles’s picture

Assigned: Unassigned » ZenDoodles

Ah. Okay... I got it.

ZenDoodles’s picture

Assigned: ZenDoodles » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
4.65 KB
1.14 KB

Added a test for @xjm

YesCT’s picture

+++ b/core/modules/rdf/src/Tests/CommentAttributesTest.php
@@ -145,6 +145,25 @@ public function testNumberOfCommentsRdfaMarkup() {
+    user_role_grant_permissions(
+      RoleInterface::AUTHENTICATED_ID,
+      ['access user profiles']
+    );

I'm not sure we do function calls like that.

the place that says is probably https://www.drupal.org/coding-standards#functcall

ZenDoodles’s picture

Oops... Put the newline back in this one.

joelpittet’s picture

Clarified the comment above the user_role_grant_permissions and fixed #107

kgoel’s picture

Status: Needs review » Reviewed & tested by the community

Spoke with @joelpittet - comment is improved in CommentAttributesTest.php for testCommentRdfAuthorMarkup() and so it's clear which link show up when registered user posts comment and also #107 has been addressed. RTBC pending testbot.

  • xjm committed d600269 on 8.0.x
    Issue #2501697 by ZenDoodles, leslieg, Les Lim, pfrenssen, joelpittet,...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

Great work on this everyone! This patch is a great example for other issues as well.

Committed and pushed to 8.0.x. I added the following inline comment on commit:

diff --git a/core/modules/rdf/src/Tests/CommentAttributesTest.php b/core/modules/rdf/src/Tests/CommentAttributesTest.php
index bce3552..8d3beb7 100644
--- a/core/modules/rdf/src/Tests/CommentAttributesTest.php
+++ b/core/modules/rdf/src/Tests/CommentAttributesTest.php
@@ -155,6 +155,8 @@ public function testCommentRdfAuthorMarkup() {
     user_role_grant_permissions(RoleInterface::AUTHENTICATED_ID, ['access user profiles']);
     $this->drupalLogin($this->webUser);
 
+    // Ensure that the author link still works properly after the author output
+    // is modified by the RDF module.
     $this->drupalGet('node/' . $this->node->id());
     $this->assertLink($this->webUser->getUsername());
     $this->assertLinkByHref('user/' . $this->webUser->id());
dsnopek’s picture

Woohoo! Congrats everyone, this issue was epic :-)

Status: Fixed » Closed (fixed)

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