Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#34 | interdiff-2130673-32-34.txt | 759 bytes | lokapujya |
#34 | 2130673-34.patch | 4.24 KB | lokapujya |
#32 | interdiff-2130673-32.txt | 2.64 KB | lokapujya |
#32 | 2130673-32.patch | 4.26 KB | lokapujya |
#28 | 2130673-28.patch | 4.6 KB | lokapujya |
Comments
Comment #1
chrisfromredfinThis patch addresses this issue in the prescribed way.
Comment #2
scor CreditAttribution: scor commentedlooks like this permission check should be preserved.
Comment #3
scor CreditAttribution: scor commentedWe 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):
Comment #4
chrisfromredfinThe 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.
Comment #5
chrisfromredfinI tested the attached patch from 4, and I also have provided an interdiff.
Flagging needs review for the testbot.
Comment #6
scor CreditAttribution: scor commentedthis needs to go
Comment #7
chrisfromredfinOops, removed that piece from the patch.
Comment #8
scor CreditAttribution: scor commentedThanks 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.
Comment #9
scor CreditAttribution: scor commenteddidn't mean to assign this issue.
Comment #11
scor CreditAttribution: scor commentednot sure why PIFR believes the last patch is #4. The last patch is #7 and is green.
Comment #12
chrisfromredfin7: rdf-comment_metadata-2130673-7.patch queued for re-testing.
Comment #13
webchickHuh! Well this certainly looks like nice clean-up. :)
Committed and pushed to 8.x. Thanks! Back to 7.x.
Comment #14
scor CreditAttribution: scor commentedalright, let's get this in D7 now. This is similar to this backport: #1323830: Place title RDFa metadata inside entity HTML element.
Comment #15
chrisfromredfinHere is a backport to 7.x, with corrected tests, and screenshot of markup for my sample node 1 with three comments.
Comment #16
jesse.d CreditAttribution: jesse.d commentedThis 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.
Comment #17
scor CreditAttribution: scor commentedThis patch looks good so far except that it should be removing the code following
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.
Comment #18
lokapujyaHaven't tested this yet.
Comment #20
scor CreditAttribution: scor commentedLooks like we have some changes from the comment module which shouldn't be in there...
Comment #21
lokapujyaHow did that sneak in there? Removed.
Comment #23
lokapujyaRemoved a condition. Changed the tests: removed test for empty rel attribute. Interdiff is ignoring whitespace.
Comment #25
lokapujyaActual patch cannot ignore whitespace.
Comment #27
scor CreditAttribution: scor commentedDid you mean to add those weird comment.module hunks back in?
Comment #28
lokapujyaNo.
Comment #30
scor CreditAttribution: scor commentedThe 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.
Comment #31
scor CreditAttribution: scor commentedThis 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.Comment #32
lokapujyaCreated an updated patch with Scor during the RDF Code Sprint. Implemented above suggestions and updated the tests.
Comment #33
scor CreditAttribution: scor commentedWe could even remove the $element variable altogether.
Comment #34
lokapujyaFurther simplified the variables assignment.
Comment #35
David_Rothstein CreditAttribution: David_Rothstein commentedDidn'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.
Comment #36
scor CreditAttribution: scor commentedThis 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.
Comment #38
dcam CreditAttribution: dcam commentedComment #41
dcam CreditAttribution: dcam commentedComment #42
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks!