Support from Acquia helps fund testing for Drupal Acquia logo

Comments

scor’s picture

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.

c4rl’s picture

webchick’s picture

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.

Anonymous’s picture

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.

scor’s picture

Status: Active » Needs review
FileSize
1.53 KB

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.

scor’s picture

Status: Needs work » Needs review
FileSize
1.13 KB
1.59 KB

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.

scor’s picture

Status: Needs work » Needs review
FileSize
9.34 KB

rdf_process() is now gone...

jwilson3’s picture

Assigned: Unassigned » jwilson3

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

jwilson3’s picture

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.

scor’s picture

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.

star-szr’s picture

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.

scor’s picture

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

scor’s picture

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

remove tag

scor’s picture

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

jenlampton’s picture

Status: Needs review » Reviewed & tested by the community

Looks good! :)

jenlampton’s picture

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.

scor’s picture

Status: Needs work » Needs review

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

jenlampton’s picture

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.

scor’s picture

Status: Needs work » Needs review
FileSize
9.56 KB

rerolling

jenlampton’s picture

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

sorry, missed the window there.

jenlampton’s picture

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

sorry, cross post. re-testing! :)

jenlampton’s picture

Status: Needs review » Reviewed & tested by the community

that's what I'm talking about :)

jenlampton’s picture

Issue summary: View changes

Updated issue summary.

alexpott’s picture

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?

scor’s picture

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

alexpott’s picture

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

scor’s picture

Status: Needs work » Needs review
FileSize
843 bytes
10.22 KB

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

scor’s picture

FileSize
10.25 KB

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.

jenlampton’s picture

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

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

jenlampton’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
41.04 KB
38.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?)

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Eric_A’s picture

Status: Fixed » Needs review
FileSize
2.06 KB

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

Anonymous’s picture

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.

jenlampton’s picture

@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

Anonymous’s picture

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.

scor’s picture

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.

Anonymous’s picture

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

scor’s picture

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.

scor’s picture

Issue summary: View changes

not related

jenlampton’s picture

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

Anonymous’s picture

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

YesCT’s picture

Issue tags: +RTBC July 1

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

Dries’s picture

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.

Anonymous’s picture

Issue summary: View changes

related