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

Files: 
CommentFileSizeAuthor
#42 1941286_42_rdf_process.patch2.94 KBscor
PASSED: [[SimpleTest]]: [MySQL] 56,411 pass(es).
[ View ]
#42 interdiff.txt1.41 KBscor
#36 1941286_36_rdf_process.patch2.06 KBEric_A
PASSED: [[SimpleTest]]: [MySQL] 56,379 pass(es).
[ View ]
#34 rdf-before-process-removal.png38.14 KBjenlampton
#34 rdf-after-process-removal.png41.04 KBjenlampton
#31 1941286_31_rdf_process.patch10.25 KBscor
PASSED: [[SimpleTest]]: [MySQL] 56,675 pass(es).
[ View ]
#30 1941286_30_rdf_process.patch10.22 KBscor
PASSED: [[SimpleTest]]: [MySQL] 58,275 pass(es).
[ View ]
#30 interdiff.txt843 bytesscor
#22 1941286_22_rdf_process.patch9.56 KBscor
PASSED: [[SimpleTest]]: [MySQL] 58,437 pass(es).
[ View ]
#14 1941286_14_rdf_process.patch9.34 KBscor
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1941286_14_rdf_process.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#9 1941286_9_rdf_process.patch9.34 KBscor
PASSED: [[SimpleTest]]: [MySQL] 55,492 pass(es).
[ View ]
#7 1941286_7_rdf_process.patch1.59 KBscor
FAILED: [[SimpleTest]]: [MySQL] 55,458 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#7 interdiff.txt1.13 KBscor
#5 1941286_5_rdf_process.patch1.53 KBscor
FAILED: [[SimpleTest]]: [MySQL] 55,441 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

Comments

I 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.

Regardless of the outcome of #1977028: Remove RDF module from core, this still needs to be possible to do from contrib for modules like RDF.

From 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.

Status:Active» Needs review
StatusFileSize
new1.53 KB
FAILED: [[SimpleTest]]: [MySQL] 55,441 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

I'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.

Status:Needs review» Needs work

The last submitted patch, 1941286_5_rdf_process.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.13 KB
new1.59 KB
FAILED: [[SimpleTest]]: [MySQL] 55,458 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

This patch fixes the warnings during the comment preview.

Status:Needs review» Needs work

The last submitted patch, 1941286_7_rdf_process.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new9.34 KB
PASSED: [[SimpleTest]]: [MySQL] 55,492 pass(es).
[ View ]

rdf_process() is now gone...

Assigned:Unassigned» jwilson3

I'll take a look at this for a manual and code review.

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

The dc:date RDF metadata wrapper span is now separated out into a separate empty SPAN.

Before patch:

<span property="dc:date dc:created" content="2013-05-23T20:03:33+02:00" datatype="xsd:dateTime" rel="sioc:has_creator">Submitted by <span class="username" lang="" typeof="sioc:UserAccount" property="foaf:name" datatype="">Anonymous (not verified)</span> on Thu, 05/23/2013 - 20:03</span>

After patch:

<span rel="sioc:has_creator">Submitted by <span class="username" lang="" typeof="sioc:UserAccount" property="foaf:name" datatype="">Anonymous (not verified)</span> on Thu, 05/23/2013 - 20:03</span>
<span property="dc:date dc:created" content="2013-05-23T20:03:33+02:00" datatype="xsd:dateTime" class="rdf-meta"></span>

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.

Status:Needs work» Needs review

Not entirely sure this is desired. Ideally, we could wrap that span around the date text value itself.

yes, 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.

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

This 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.

StatusFileSize
new9.34 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1941286_14_rdf_process.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Good 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 :)

Status:Needs work» Needs review
Issue tags:-Needs reroll

remove tag

Another issue which will also be reduced once this patch is committed: #2009664: Replace theme() with drupal_render() in rdf module.

Status:Needs review» Reviewed & tested by the community

Looks good! :)

Status:Reviewed & tested by the community» Needs work

On closer inspection, maybe not.... we're loosing some attributes on the span tag around submitted by info.

Sample before patch:

<span rel="sioc:has_creator" datatype="xsd:dateTime" content="2013-06-17T15:32:14-07:00" property="dc:date dc:created">
  Submitted by <a class="username" lang="" datatype="" property="foaf:name" typeof="sioc:UserAccount" about="/user/1" title="View user profile." href="/user/1">root</a> on Mon, 06/17/2013 - 15:32
</span>

Sample after patch

<span rel="sioc:has_creator">
  Submitted by <a class="username" lang="" datatype="" property="foaf:name" typeof="sioc:UserAccount" about="/user/1" title="View user profile." href="/user/1">root</a> on Mon, 06/17/2013 - 15:32
</span>

I don't think this was intended.

Status:Needs work» Needs review

@jenlampton this is intended, see #11 and my response in #12. I'll let Jen set back to RTBC.

Issue tags:-Twig, -theme system cleanup

#14: 1941286_14_rdf_process.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Twig, +theme system cleanup

The last submitted patch, 1941286_14_rdf_process.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new9.56 KB
PASSED: [[SimpleTest]]: [MySQL] 58,437 pass(es).
[ View ]

rerolling

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

sorry, missed the window there.

Status:Needs work» Needs review
Issue tags:-Needs reroll

sorry, cross post. re-testing! :)

Status:Needs review» Reviewed & tested by the community

that's what I'm talking about :)

Issue summary:View changes

Updated issue summary.

I'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?

@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.

Status:Reviewed & tested by the community» Fixed

Committed 899ec0b and pushed to 8.x. Thanks!

Status:Fixed» Needs work

Actually 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...

Status:Needs work» Needs review
StatusFileSize
new843 bytes
new10.22 KB
PASSED: [[SimpleTest]]: [MySQL] 58,275 pass(es).
[ View ]

updated 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).

StatusFileSize
new10.25 KB
PASSED: [[SimpleTest]]: [MySQL] 56,675 pass(es).
[ View ]

The main RDF refactoring patch was committed (yay!) so here is a reroll.

Status:Needs review» Needs work
Issue tags:-Twig, -theme system cleanup

The last submitted patch, 1941286_31_rdf_process.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Twig, +theme system cleanup

#31: 1941286_31_rdf_process.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new41.04 KB
new38.14 KB

Yay for passing tests! :) Retested manually too, and things still look good here.

Before:
rdf-before-process-removal.png

After:
rdf-after-process-removal.png

(hopefully you can see all the RDF data in these?)

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

Status:Fixed» Needs review
StatusFileSize
new2.06 KB
PASSED: [[SimpleTest]]: [MySQL] 56,379 pass(es).
[ View ]

Looks like this accidentally reverted part of #2009664: Replace theme() with drupal_render() in rdf module.

This 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 in template_preprocess_comment.

@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

Yes, 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.

This 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.

It was passing in Bartik before this patch. For example, see the test run in #31 of that issue.

StatusFileSize
new1.41 KB
new2.94 KB
PASSED: [[SimpleTest]]: [MySQL] 56,411 pass(es).
[ View ]

Got 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.

Issue summary:View changes

not related

Status:Needs review» Reviewed & tested by the community

Thanks 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

Just confirming, this does solve the issue that I found with my tests... so RTBC from me as well.

Issue tags:+RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

Status:Reviewed & tested by the community» Fixed

Committed the remainder. All is good now. Thanks. :)

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

Issue summary:View changes

related