rdf.module - Convert theme_ functions to Twig

Attached patch converts rdf_metadata and rdf_template_variable_wrapper theme calls to twig. There was no rdf_template_variable_wrapper twig file in the sandbox, so I created one.

The docblocks for the preprocess functions will need to be updated at some point but it's too soon to say exactly how, so they can probably be left for now. See #1913208: [policy] Standardize template preprocess function documentation.

+++ b/core/modules/rdf/templates/rdf-template-variable-wrapper.html.twigundefined
@@ -0,0 +1,58 @@
+ * @see template_preprocess
+ * @see template_preprocess_rdf_template_variable_wrapper

These should end in parens i.e. template_preprocess() per http://drupal.org/node/1354#see.

Otherwise looks pretty good to me, thanks @duellj!

Thanks for the review @Cottser! I've updated the @see references. Watching #1913208: [policy] Standardize template preprocess function documentation to see what happens.

#5: 1898444-5-twig-rdf.patch queued for re-testing.

A few other changes:
- renamed rdf_attributes to attributes
- added a missing space before a closing }}
- updated docs to reference .html.twig files instead of .tpl.php (maybe premature?)
- removed all dollar signs ($) from docs and instead surrounded variable names with single quotes
- removed references to variable types in docblock
- removed any mention of theme functions in docblock
- trimmed down the docs in templates, and restored docs on preprocess functions.
- updated preprocess docs to match new standards :)

Looking good. Test bot?

Removed whitespace for final review.

We're going to be removing the entire process layer eventually in favor of having Twig drpal_render() all the pieces instead. I don't think that will be a huge problem for RDF since we're mostly talking about Attributes() here anyway, but we'll definitely need to triple check the output when that's done. If you're refactoring as well for better markup we'll just need to quadruple check!

Almost… :)

+++ b/core/modules/rdf/rdf.moduleundefined
@@ -863,42 +858,32 @@ function rdf_preprocess_image(&$variables) {
- * give machines extra information about the content and its structure.
+ * give machines extra information about the content and its structure. ¶

One more bit of whitespace here.

Testbot is having a rough day. I'll do a quick reroll for the whitespace tonight.

#14: 1898444-14.patch queued for re-testing.

Great work here, and good to see progress on Twig.

+++ b/core/modules/rdf/templates/rdf-metadata.html.twig
@@ -0,0 +1,19 @@
+{% for attributes in metadata %}
+<span class="{{ attributes.class }}"{{ attributes }}></span>
+{% endfor %}

why do we need a special case for class here? Isn't Twig going to treat it like any regular attribute and output it if it's present? in other word, why can't we use:

<span {{ attributes }}></span>

@Scor,

great feedback. The short answer is: we can!

In general, we want to separate out the class from the rest of the attributes in places where we think front-end devs will want to add a class to something. If you don't think this is a place anyone will theme override, or they won't be doing it to add a class (which - now that you bring it up, I think is the exact case) then we should just print all the attributes at once.

The reason for separating them out at all is that 1) front end devs will instantly recognize class="" in the markup, and 2) won't need to consult Drupal documentation to do something as seemingly straightforward as adding a class.

That said, attached patch does not treat classes special here. (and slightly clarifies docblock)

The class attribute wasn't explicitly pulled before Twig when we were using drupal_attributes(), so I don't see a need to pull it out now, so you patch looks good. template_preprocess_rdf_metadata() actually sets a default class 'rdf-meta' automatically, so that class is there for themes if they need to target this element for some reason. There would still be a way for themers to alter the class attribute I imagine through some kind of preprocessing function, or via overriding rdf-metadata.html.twig. I'm assuming both these operations are/will still be possible with Twig?

whitespaces are back! (3 lines)

Rerolled to remove whitespace and added a commit message to the issue summary based on git blame and sandbox issues. If I missed anyone, please edit the issue summary.

thanks, #20 looks great.

#20: 1898444-20.patch queued for re-testing.

#20: 1898444-20.patch queued for re-testing.

Add commit message

Found some minor documentation and code style things that can be fixed up, then back to RTBC. Making these changes would be a good novice task, tagging.

+++ b/core/modules/rdf/templates/rdf-metadata.html.twigundefined
@@ -0,0 +1,19 @@
+ * Returns HTML for a series of empty spans for exporting RDF metadata in RDFa.

+++ b/core/modules/rdf/templates/rdf-template-variable-wrapper.html.twigundefined
@@ -0,0 +1,36 @@
+ * Returns HTML for a template variable wrapped in an HTML element with the
+ * RDF attributes.

These should start with 'Default theme implementation…' per http://drupal.org/node/1823416#docblock and the second one should be under 80 characters per http://drupal.org/node/1354#file. If need be, more description can be added in a separate paragraph after the summary line.

+++ b/core/modules/rdf/templates/rdf-metadata.html.twigundefined
@@ -0,0 +1,19 @@
+<span {{ attributes }}></span>

+++ b/core/modules/rdf/templates/rdf-template-variable-wrapper.html.twigundefined
@@ -0,0 +1,36 @@
+    <span {{ attributes }}>{{ content }}</span>
...
+    <div {{ attributes }}>{{ content }}</div>

The {{ attributes }} should not have a space before it, see http://drupal.org/node/1823416#attributes.

+++ b/core/modules/rdf/templates/rdf-metadata.html.twigundefined
@@ -0,0 +1,19 @@
+ * @ingroup rdf

+++ b/core/modules/rdf/templates/rdf-template-variable-wrapper.html.twigundefined
@@ -0,0 +1,36 @@
+ * @ingroup rdf

Let's remove the @ingroup rdf lines from the templates as well, I don't see why RDF is a special case here, all other templates have just @ingroup themeable.

Working on this currently, will post more later

Ok I incorporated the feedback from comments 24/25, just a little cleanup.

Missed some documentation, will revise later

+++ b/core/modules/rdf/templates/rdf-template-variable-wrapper.html.twig
@@ -0,0 +1,38 @@
+ * @file
+ * Default theme implementation that returns an HTML element with the RDF ¶
+ * attributes.
+ *
+ * Returns HTML for a template variable wrapped in an HTML element with the RDF ¶
+ * attributes.

Nice work cleaning that up! You got some ending whitespace in there. Depending on your ide/editor there may be an option to trim trailing whitespace on save. Keep it up!

Ok, I cleaned up the whitespace and trimmed the documentation even further to keep with the coding standards (http://drupal.org/node/1354#file). Also fixed some indentation as per http://drupal.org/node/1823416#expressions.

Looks like a random test failure. I worked on this patch with @lbainbridge, but looking at the docs again…

+++ b/core/modules/rdf/templates/rdf-template-variable-wrapper.html.twigundefined
@@ -0,0 +1,37 @@
+ * If the 'attributes' variable is present, wrap the content in a div or span
+ * tag with the additional RDF attributes added.

I think this should say 'the content is wrapped' instead of 'wrap the content'. We could also consider adding something like "If not, the content is left as is."

Something like this?

Attached an interdiff from #20 as well (previous RTBC).

Just gave this one a quick re-test, still works, new docs look great!

Add conversion summary table

Update commit message

#33: 1898444-33.patch queued for re-testing.

will take on profiling this one.

=== 8.x..8.x compared (519fc956834f6..519fc98bf3bad):

ct  : 34,154|34,154|0|0.0%
wt  : 173,054|173,173|119|0.1%
cpu : 164,011|164,010|-1|-0.0%
mu  : 13,850,608|13,850,608|0|0.0%
pmu : 13,976,200|13,976,200|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fc956834f6&...

=== 8.x..rdf-1898444-33 compared (519fc956834f6..519fc9a1407b3):

ct  : 34,154|34,328|174|0.5%
wt  : 173,054|173,463|409|0.2%
cpu : 164,011|160,011|-4,000|-2.4%
mu  : 13,850,608|14,002,920|152,312|1.1%
pmu : 13,976,200|14,129,744|153,544|1.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fc956834f6&...

The previous profile did not use theme_rdf_metadata. Here's results for a page that executes both theme functions.

=== 8.x..8.x compared (519ffa0e6cdec..519ffa45eeaa4):

ct  : 34,117|34,117|0|0.0%
wt  : 165,355|165,906|551|0.3%
cpu : 164,010|160,010|-4,000|-2.4%
mu  : 12,013,832|12,013,832|0|0.0%
pmu : 12,124,808|12,124,808|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519ffa0e6cdec&...

=== 8.x..rdf-1898444-33 compared (519ffa0e6cdec..519ffa6332b29):

ct  : 34,117|34,510|393|1.2%
wt  : 165,355|167,440|2,085|1.3%
cpu : 164,010|156,010|-8,000|-4.9%
mu  : 12,013,832|12,080,992|67,160|0.6%
pmu : 12,124,808|12,193,104|68,296|0.6%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519ffa0e6cdec&...

Remove sandbox link

accidental post

profiling done

Created a node with a comment and made it the front pagein for both rdf_template_variable_wrapper and rdf_metadata Twig templates to be rendered.

=== 8.x..8.x compared (519ff693b4ca2..519ff79b9e81a):

ct  : 44,408|44,408|0|0.0%
wt  : 285,608|285,043|-565|-0.2%
cpu : 280,748|280,116|-632|-0.2%
mu  : 12,038,808|12,038,808|0|0.0%
pmu : 12,140,200|12,140,200|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519ff693b4ca2&...

=== 8.x..1898444-rdf-twig compared (519ff693b4ca2..519ff7de62889):

ct  : 44,408|44,801|393|0.9%
wt  : 285,608|286,741|1,133|0.4%
cpu : 280,748|281,923|1,175|0.4%
mu  : 12,038,808|12,107,688|68,880|0.6%
pmu : 12,140,200|12,209,968|69,768|0.6%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519ff693b4ca2&...

+++ b/core/modules/rdf/rdf.module
@@ -863,42 +858,32 @@ function rdf_preprocess_image(&$variables) {
+function template_preprocess_rdf_template_variable_wrapper(&$variables) {
+  if (!empty($variables['content']) && !empty($variables['attributes'])) {
+    $variables['attributes'] = new Attribute($variables['attributes']);
   }

I am almost positive we don't need to do this due to https://drupal.org/node/1982024
If so this should be slightly faster and we can remove the preprocess, don't forget to remove the @see in the twig template too.

@joelpittet you're right, this approach seems to work. perf++.

Updated issue summary.

postponing, see #1941286-13: Remove the process layer (rdf module).

unpostponing as the process layer is out of there. Needs a re-roll.

#45: 1898444-45.patch queued for re-testing.

rerolled.

Thanks for the re-roll @jenlampton!

@@ -0,0 +1,18 @@
+ * @see template_preprocess()

Removed this as per #2013094: [policy adopted, patch needed] Stop saying '@see template_preprocess()' in every twig file

This is ready to go and has been profiled in #39 #40 and #43

Anything else or RTBC?

I would actually like to see this re-profiled first since things have changed. Other than that looks great!

#51: 1898444-51-twig-rdf-theme.patch queued for re-testing.

Capitalization in commit message

51: 1898444-51-twig-rdf-theme.patch queued for re-testing.

Same Scenario as #43 except 5 comments.

=== 8.x..8.x compared (52907577b0b08..529075c009a60):

ct  : 80,606|80,606|0|0.0%
wt  : 366,627|366,594|-33|-0.0%
cpu : 340,371|340,405|34|0.0%
mu  : 17,460,064|17,460,064|0|0.0%
pmu : 17,530,168|17,530,168|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52907577b0b08&...

=== 8.x..1898444-twig-rdf-theme compared (52907577b0b08..5290760896b28):

ct  : 80,606|81,189|583|0.7%
wt  : 366,627|368,874|2,247|0.6%
cpu : 340,371|342,375|2,004|0.6%
mu  : 17,460,064|17,504,432|44,368|0.3%
pmu : 17,530,168|17,575,008|44,840|0.3%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52907577b0b08&...

I think this one is ready to go!

I manually tested the patch once more and this still checks out. Profiling results look reasonable to me and I think we made it faster because #43 tested 1 comment and #55 tested 5 comments.

51: 1898444-51-twig-rdf-theme.patch queued for re-testing.

51: 1898444-51-twig-rdf-theme.patch queued for re-testing.

Committed 360c0c0 and pushed to 8.x. Thanks!