Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
We'll need to think carefully about how we can move all the process RDF does in the process layer up to preprocess, either by use of Attributes, or other methods.
Related
#1843650: Remove the process layer (hook_process and hook_process_HOOK)
#1706612: remove 'submitted' variable in templates for ease of theme development
Comment | File | Size | Author |
---|---|---|---|
#42 | 1941286_42_rdf_process.patch | 2.94 KB | scor |
#42 | interdiff.txt | 1.41 KB | scor |
#36 | 1941286_36_rdf_process.patch | 2.06 KB | Eric_A |
#34 | rdf-before-process-removal.png | 38.14 KB | jenlampton |
#34 | rdf-after-process-removal.png | 41.04 KB | jenlampton |
Comments
Comment #1
scor CreditAttribution: scor commentedI can't think of a reason why we could not move what's currently in rdf_process() to be in preprocess instead, they don't need to wait on anything done in preprocess.
This issue should probably wait on #1898444: rdf.module - Convert theme_ functions to Twig to be committed.
Comment #2
c4rl CreditAttribution: c4rl commentedRelated #1977028: Remove RDF module from core
Comment #3
webchickRegardless of the outcome of #1977028: Remove RDF module from core, this still needs to be possible to do from contrib for modules like RDF.
Comment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedFrom my own impression and from what scor said, I don't think that we really need to remove RDFa to make this happen, and we don't need to keep process functionality for RDF even if it lives in contrib. We should be able to move everything into preprocess.
Comment #5
scor CreditAttribution: scor commentedI've started moving the RDF process logic into preprocess. This patch takes care of one of the two components: rdf_metadata_attributes. Setting to needs review to bot can test, will switch back to needs work afterwards.
Comment #7
scor CreditAttribution: scor commentedThis patch fixes the warnings during the comment preview.
Comment #9
scor CreditAttribution: scor commentedrdf_process() is now gone...
Comment #10
jwilson3I'll take a look at this for a manual and code review.
Comment #11
jwilson3The dc:date RDF metadata wrapper span is now separated out into a separate empty SPAN.
Before patch:
After patch:
Not entirely sure this is desired. Ideally, we could wrap that span around the date text value itself.
Edit: Updated this comment to reflect the fact that the dc:date is still there, which i missed at first.
Comment #12
scor CreditAttribution: scor commentedyes, this is intentional, and a temporary solution while we work on Twig to allow that markup to wrap the date itself, which is the ultimate goal. back to NR.
Comment #13
star-szrThis needs a reroll.
If we're removing theme_rdf_template_variable_wrapper() here, should we remove its Twig conversion from the patch at #1898444: rdf.module - Convert theme_ functions to Twig? I am assuming it is no longer needed if it's being removed here.
Comment #14
scor CreditAttribution: scor commentedGood point @Cottser, I've postponed #1898444: rdf.module - Convert theme_ functions to Twig until we get this one in.
rerolled this patch, let's get that in asap so we can move on :)
Comment #15
scor CreditAttribution: scor commentedremove tag
Comment #16
scor CreditAttribution: scor commentedAnother issue which will also be reduced once this patch is committed: #2009664: Replace theme() with drupal_render() in rdf module.
Comment #17
jenlamptonLooks good! :)
Comment #18
jenlamptonOn closer inspection, maybe not.... we're loosing some attributes on the span tag around submitted by info.
Sample before patch:
Sample after patch
I don't think this was intended.
Comment #19
scor CreditAttribution: scor commented@jenlampton this is intended, see #11 and my response in #12. I'll let Jen set back to RTBC.
Comment #20
jenlampton#14: 1941286_14_rdf_process.patch queued for re-testing.
Comment #22
scor CreditAttribution: scor commentedrerolling
Comment #23
jenlamptonsorry, missed the window there.
Comment #24
jenlamptonsorry, cross post. re-testing! :)
Comment #25
jenlamptonthat's what I'm talking about :)
Comment #25.0
jenlamptonUpdated issue summary.
Comment #26
alexpottI'm a little confused the after html in #11 and #18 are different... basically we no longer seem to have rdf info for the created date... was this intentional?
Comment #27
scor CreditAttribution: scor commented@alexpott the created date RDFa markup was moved to a separate span right outside and after the original span element. It is present in #11, but I think Jen might simply have forgotten to include that span when she paste the markup in #18. Note that we have a few tests in place which would fail if this markup was missing from the page.
Comment #28
alexpottCommitted 899ec0b and pushed to 8.x. Thanks!
Comment #29
alexpottActually didn't push this :)
And just realised that this removes the rdf_template_variable_wrapper theme... so rdf_theme() needs updating and system.api.php too...
Comment #30
scor CreditAttribution: scor commentedupdated rdf_theme() and the reference @see theme_rdf_template_variable_wrapper(). I think it makes more sense to leave the hook_process() documentation in system/theme.api.php removal to the main issue about hook_process(), I left a comment to that effect at #1843650-6: Remove the process layer (hook_process and hook_process_HOOK).
Comment #31
scor CreditAttribution: scor commentedThe main RDF refactoring patch was committed (yay!) so here is a reroll.
Comment #33
jenlampton#31: 1941286_31_rdf_process.patch queued for re-testing.
Comment #34
jenlamptonYay for passing tests! :) Retested manually too, and things still look good here.
Before:
After:
(hopefully you can see all the RDF data in these?)
Comment #35
catchCommitted/pushed to 8.x, thanks!
Comment #36
Eric_A CreditAttribution: Eric_A commentedLooks like this accidentally reverted part of #2009664: Replace theme() with drupal_render() in rdf module.
Comment #37
Anonymous (not verified) CreditAttribution: Anonymous commentedThis patch also broke the output for some of the comment RDFa. For example, it adds the attributes for the submitted line in
rdf_preprocess_node
, but these are then overwritten intemplate_preprocess_comment
.Comment #38
jenlampton@linclark do we have a test for that, if not, should we write one?
This patch looks good for removing theme(), but doesn't add back missing attribute data from comments... will dig deeper
Comment #39
Anonymous (not verified) CreditAttribution: Anonymous commentedYes, I'm sorry, I should have mentioned that. I only discovered this because it makes the test I wrote in #1784234: Change notice: Use schema.org types and properties in RDF mappings fail.
Comment #40
scor CreditAttribution: scor commentedThis markup is definitely tested in Drupal\rdf\Tests\CommentAttributesTest->_testBasicCommentRdfaMarkup() lines 262, 269 and 287 in the default theme for testing which is Stark. In fact this test fails if the code from rdf_preprocess_comment() is removed. I've checked in my browser and they are present in the HTML. The problem comes from Bartik that has its own comment.html.twig and outputs its own author and date as opposed to using the submitted variable. Bartik is used in the Standard profile which explains the failing test on #1784234: Change notice: Use schema.org types and properties in RDF mappings. Switching to stark should solve that issue.
Let's commit #36 and follow up in #2031545: Fix Bartik's comment.html.twig to markup author and creation date in RDFa.
Comment #41
Anonymous (not verified) CreditAttribution: Anonymous commentedIt was passing in Bartik before this patch. For example, see the test run in #31 of that issue.
Comment #42
scor CreditAttribution: scor commentedGot it. The comment template has extra variables which are not present in node.html.twig nor used in stark: author and created. Bartik uses those instead of submitted. rdf.module was supporting all those variables but my patch #31 dropped support for author and created, that's where the regression came from. My apologies. I will write some code to test both Stark and Bartik, but in the meantime, here is the new patch that fixes the regression.
Comment #42.0
scor CreditAttribution: scor commentednot related
Comment #43
jenlamptonThanks guys! This patch looks great.
I think long term we're going to get rid of $submitted anyway, so it would be good not to loose support for author and date. See related: #1706612: remove 'submitted' variable in templates for ease of theme development
However, I have some concerns about the current approach RDF is using to wrap elements in spans, so I also created a follow-up here with an alternative: #2031797: Provide a RDF wrapper twig template for renderable variables + attributes
Comment #44
Anonymous (not verified) CreditAttribution: Anonymous commentedJust confirming, this does solve the issue that I found with my tests... so RTBC from me as well.
Comment #45
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #46
Dries CreditAttribution: Dries commentedCommitted the remainder. All is good now. Thanks. :)
Comment #47.0
(not verified) CreditAttribution: commentedrelated