Problem/Motivation

The number of comments RDFa markup is currently placed in two different location based on the node view mode.

Proposed resolution

Let's simplify this in the same way we simplified the node title (#1323830: Place title RDFa metadata inside entity HTML element) and always place it in the node template as metadata.

Remaining tasks

write patch

User interface changes

none

API changes

none

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chrisfromredfin’s picture

Status: Active » Needs review
FileSize
2.29 KB

This patch addresses this issue in the prescribed way.

scor’s picture

+++ b/core/modules/rdf/rdf.module
@@ -317,32 +317,12 @@ function rdf_preprocess_node(&$variables) {
-      if ($variables['page'] && \Drupal::currentUser()->hasPermission('access comments')) {

looks like this permission check should be preserved.

scor’s picture

Status: Needs review » Needs work

We can also trim down the tests, we no longer need this extra check because now the logic is always the same (no longer dealing with the a element):

    // Makes sure we don't generate the wrong statement for number of comments.
    $node_comments_uri = url('node/' . $this->node->id(), array('fragment' => 'comments', 'absolute' => TRUE));
    $this->assertFalse($graph->hasProperty($node_comments_uri, 'http://rdfs.org/sioc/ns#num_replies', $expected_value), 'Number of comments found in RDF output of teaser view mode (sioc:num_replies).');
chrisfromredfin’s picture

The attached patch preserves the permissions check and also includes a small performance refactor -- if the comment module is not enabled at all, we don't need the function call overhead of getPreparedFieldMapping either. It also removes the extra check from the test.

chrisfromredfin’s picture

Status: Needs work » Needs review
FileSize
1.78 KB

I tested the attached patch from 4, and I also have provided an interdiff.

Flagging needs review for the testbot.

scor’s picture

Status: Needs review » Needs work
+++ b/core/modules/rdf/rdf.module
@@ -317,32 +317,12 @@ function rdf_preprocess_node(&$variables) {
diff --git a/sites/default/default.settings.php b/sites/default/default.settings.php
diff --git a/sites/default/default.settings.php b/sites/default/default.settings.php
old mode 100644
new mode 100755

this needs to go

chrisfromredfin’s picture

Status: Needs work » Needs review
FileSize
4.13 KB

Oops, removed that piece from the patch.

scor’s picture

Assigned: Unassigned » c4rl
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to 7.x

Thanks for your work cwells. Great to see the logic being streamlined and special case handling code being removed in favor or one single code flow.

scor’s picture

Assigned: c4rl » Unassigned

didn't mean to assign this issue.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: rdf-comment_metadata-2130673-4.patch, failed testing.

scor’s picture

Status: Needs work » Reviewed & tested by the community

not sure why PIFR believes the last patch is #4. The last patch is #7 and is green.

chrisfromredfin’s picture

webchick’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

Huh! Well this certainly looks like nice clean-up. :)

Committed and pushed to 8.x. Thanks! Back to 7.x.

scor’s picture

Version: 8.x-dev » 7.x-dev

alright, let's get this in D7 now. This is similar to this backport: #1323830: Place title RDFa metadata inside entity HTML element.

chrisfromredfin’s picture

Status: Patch (to be ported) » Needs review
FileSize
2.45 KB
125.03 KB

Here is a backport to 7.x, with corrected tests, and screenshot of markup for my sample node 1 with three comments.
screenshot of markup with 3 comments

jesse.d’s picture

This looks good to me. All tests pass and the markup displays the metadata as expected. I've attached before and after looks at the markup.

scor’s picture

Status: Needs review » Needs work
Issue tags: - +RDF code sprint

This patch looks good so far except that it should be removing the code following

// Annotates the 'x comments' link in teaser view.

as well, similar to #7. (don't forget to also generalize the condition that adds that markup to cover both full and teaser view modes). Some tests will also most like need to be updated.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
6.89 KB
2.87 KB

Haven't tested this yet.

Status: Needs review » Needs work

The last submitted patch, 18: 2130673-18.patch, failed testing.

scor’s picture

Looks like we have some changes from the comment module which shouldn't be in there...

lokapujya’s picture

Status: Needs work » Needs review
FileSize
3.67 KB

How did that sneak in there? Removed.

Status: Needs review » Needs work

The last submitted patch, 21: 2130673-21.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
7.54 KB
2.41 KB

Removed a condition. Changed the tests: removed test for empty rel attribute. Interdiff is ignoring whitespace.

Status: Needs review » Needs work

The last submitted patch, 23: interdiff-2130673-23.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
7.84 KB
2.41 KB

Actual patch cannot ignore whitespace.

Status: Needs review » Needs work

The last submitted patch, 25: 2130673-23.patch, failed testing.

scor’s picture

Did you mean to add those weird comment.module hunks back in?

lokapujya’s picture

Status: Needs work » Needs review
FileSize
4.6 KB

No.

Status: Needs review » Needs work

The last submitted patch, 28: 2130673-28.patch, failed testing.

scor’s picture

+++ b/modules/rdf/rdf.module
@@ -511,35 +511,21 @@ function rdf_preprocess_node(&$variables) {
+      'about' => $variables['node_url'],

The about is not needed in this case. That's the same reason as #1323830-21: Place title RDFa metadata inside entity HTML element.

Also, feel free to jam it all up into a nest statement like the last patch of #1323830: Place title RDFa metadata inside entity HTML element, for consistency.

scor’s picture

+++ b/modules/rdf/rdf.test
@@ -461,15 +461,13 @@ class RdfCommentAttributesTestCase extends CommentHelperCase {
-    $this->drupalGet('node/' . $this->node1->nid);
-    $node_url = url('node/' . $this->node1->nid);
-    $comment_count_teaser = $this->xpath('/html/head/meta[@about=:node-url and @property="sioc:num_replies" and @content="2" and @datatype="xsd:integer"]', array(':node-url' => $node_url));
+    $this->drupalGet($node_url);

This line is wrong and is causing the tests to fail. drupalGet() will run its own url() on the value (e.g. node/1), so we shouldn't pass in a value such as $node_url which has already been passed through url(). Just use $this->drupalGet('node/' . $this->node1->nid); like it was before.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
4.26 KB
2.64 KB

Created an updated patch with Scor during the RDF Code Sprint. Implemented above suggestions and updated the tests.

scor’s picture

Status: Needs review » Needs work
+++ b/modules/rdf/rdf.module
@@ -511,35 +511,21 @@ function rdf_preprocess_node(&$variables) {
+  if (isset($variables['node']->comment_count) &&
+      !empty($variables['node']->rdf_mapping['comment_count']['predicates']) &&
+      user_access('access comments')) {
+    // Adds RDFa markup for the comment count near the node title as metadata.
+    $element = array(
+      '#theme' => 'rdf_metadata',
+      '#metadata' => array(
+        array(
           'property' => $variables['node']->rdf_mapping['comment_count']['predicates'],
           'content' => $variables['node']->comment_count,
           'datatype' => $variables['node']->rdf_mapping['comment_count']['datatype'],
         ),
-      );
-      drupal_add_html_head($element, 'rdf_node_comment_count');
-    }
+      ),
+    );
+    $variables['title_suffix']['rdf_meta_comment_count'] = $element;

We could even remove the $element variable altogether.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
4.24 KB
759 bytes

Further simplified the variables assignment.

David_Rothstein’s picture

Didn't review this super-carefully, but overall it looks good to me. However, I have the same comment/question as #1323830-57: Place title RDFa metadata inside entity HTML element here too.

scor’s picture

Status: Needs review » Reviewed & tested by the community

This is good to go and get committed right after #2301955: Ensure RDFa metadata tags are hidden. Since those patches are touching different parts of rdf.module, no reroll is necessary.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 34: 2130673-34.patch, failed testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 34: 2130673-34.patch, failed testing.

dcam queued 34: 2130673-34.patch for re-testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community
David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.33 release notes

Committed to 7.x - thanks!

  • David_Rothstein committed 3a4f085 on 7.x
    Issue #2130673 by lokapujya, cwells | scor: Place number of comments...

Status: Fixed » Closed (fixed)

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