link.module - Convert theme_ functions to Twig

I've been trying to learn a bit about D8 and Twig this evening, so I checked out the Twig sandbox and tried following the conversion instructions for the simplest module needing to be ported I could find, this one. My patch is attached. We'll see if this is at all on the right track.

Yay, I may not have totally completed the job, but at least I didn't blow anything up.

BTW I punted on dealing with l() since it looks like there are some big discussions happening around it.

#1812562: Create best practices about use of the l() function and the url() function in templates
#1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig

Hi Floydm, and thanks for jumping right in! :)

A few notes..
- In the Twig sandbox all the templates lived in the stark theme, but as part of the move to core we're putting them "back where they belong" so in this case there needs to be templates directory added into link module, and your new template goes there.
- We also need to remove the theme function, since we are replacing it with the preprocess + template file. (We did not remove the theme function in the sandbox, for testing purposes)
- We're going to remove the process layer, so we need to remove any reference to template_process * @see template_process()
- We changed our minds about checking variables a few times, but in the end we decided not to use 'is defined' anymore in {% if title is defined %}
- When using a filter, we don't want any space around the pipe character {{ title | striptags }}
- But, rather than use a striptags filter, let's preprocess that baby so we can just print title.
- We need to add a 'template' in our hook_theme implementation.

Here's a re-roll of this patch, with the above changed. Let's see if it works!

Thanks @jenlampton, for the explanation.

I updated my twig sandbox and applied this patch in the hope of helping take it farther, but I think I'm in over my head. It looks like this is failing in the unit tests, so I figured out how to run them on my box today, which is *something*, but ... I'm not quite sure how/where to take it from here. More reading up to do, I guess. :)

@Floydm,

The next step would be to take this patch and apply it to a local install of Drupal 8 HEAD (not the sandbox), and then run only the test(s) that failed. I'd start with LinkFieldTest. Find out what that code is testing, and see if those conditions actually exist after this patch.

From the test result it looks like the first test is looking for the exact string <div class="link-item"><div class="link-url"><a href="http://www.example.com/content/articles/archive?author=John&amp;year=2012#com">http://www.example.com/content/articles/archive?author=John&amp;year=2012#com</a></div></div>.

If we have whitespace in there, or are missing a class or something, those are reasons we could be causing this test to fail.

That might be a simple fix, do you want to give it a shot?

Hi @jenlampton. Thanks for the follow up.

I don't quite get what is going on, but I get errors like "Fatal error: Call to a member function entityType() on a non-object in /www/d8/core/modules/link/lib/Drupal/link/Tests/LinkFieldTest.php on line 475" and "FATAL Drupal\link\Tests\LinkFieldTest: test runner returned a non-zero error code (255)." when I try to run the test on my box with "php ./run-tests.sh --module --verbose link". Then I get all sorts of failures in the tests, like userLogin fails. Is that not the correct way to run the test?

So I haven't tested this locally, but I *think* it is just a whitespace issue in the tests. The new template introduces whitespace where previously there was none, so the attached patch attempts to copy the whitespace changes from the template into the tests.

@Floydm - Thanks for your work on this! I think it will probably be more instructive to run the relevant tests through the UI after enabling the Testing module.

Thank you, @ jenlampton and @Cottser, for your help.

This now passes all tests on my box. There was one legitimate error in the template (two closing a tags) and the rest of the issues were new white space and carriage returns. I fixed the closing tag and added the white space to the tests.

+++ b/core/modules/link/templates/link-formatter-link-separate.html.twigundefined
@@ -0,0 +1,19 @@
+  {% if title %}<div class="link-title">{{ title }}</div>{% endif %}

Why write all this logic in one line. I would place the actual printing of the div between the if statement

+++ b/core/modules/link/templates/link-formatter-link-separate.html.twigundefined
@@ -0,0 +1,19 @@
\ No newline at end of file

You need to end a file with a newline

hang on. fixing newline too.

Added newline to template.

This patch passes all tests on my machine.

Looking at sandbox issues and commits, I couldn't find anyone else to credit. So no commit message added to the summary for this issue.

This is looking good, but I'm not sure about the way new lines are added in the tests. I mean, it passed, so that's obviously good :) But can we check in the other core tests and see how the \n is usually added? Maybe we can just stick it on the end of the previous line, like:

-             $expected = '<div class="link-item">';
+            $expected = "<div class=\"link-item\">\n    ";

That would look a little better, anyway. But I'm curious to see how it's done elsewhere.

The block rendering tests use implode() to add the newlines, not sure if that's the rule or the exception though. See the interdiff in #1898034-16: block.module - Convert PHPTemplate templates to Twig.

Agreed, @jenlampton, I'm not fond of the newlines either.

Thank you, @Cottser, for pointing out the block example. That seems like a better approach to me but we'll see what others think.

Rerolled the patch with the approach to newlines found in the blocks interdiff referenced in #23.

@Floydm - You're probably missing some blank lines now. I found it helpful to use debug() while working on the test to compare the expected output vs. the actual output.

No I just made a dumb mistake. This time after fixing it I ran it to verify it passes all the tests, then built the patch and applied it to the 8.x branch to verify it applied cleanly, then ran the tests against the 8.x branch to verify it passed there too. So, really, this time it should work.

Thank you both, @Cottser and @jenlampton, for your patience and coaching.

Here's a thorough review from my end.

On the test:

+++ b/core/modules/link/lib/Drupal/link/Tests/LinkFieldTest.php
@@ -424,19 +425,23 @@ function testLinkSeparateFormatter() {
             $url_title = isset($new_value) ? truncate_utf8($url, $new_value, FALSE, TRUE) : $url;
-            $expected = '<div class="link-item">';
-            $expected .= '<div class="link-url"><a href="' . check_plain($url) . '">' . check_plain($url_title) . '</a></div>';
-            $expected .= '</div>';
-            $this->assertRaw($expected);
+            $expected  = array();
+            $expected[] = '<div class="link-item">';
+            $expected[] = '    <div class="link-url"><a href="' . check_plain($url) . '">' . check_plain($url_title) . '</a></div>';
+            $expected[] = '</div>';
+            $expected_output = implode("\n", $expected);
+            $this->assertRaw($expected_output);
 
             $url = $url2;
             $url_title = isset($new_value) ? truncate_utf8($url, $new_value, FALSE, TRUE) : $url;
             $title = isset($new_value) ? truncate_utf8($title2, $new_value, FALSE, TRUE) : $title2;
-            $expected = '<div class="link-item">';
-            $expected .= '<div class="link-title">' . check_plain($title) . '</div>';
-            $expected .= '<div class="link-url"><a href="' . check_plain($url) . '">' . check_plain($url_title) . '</a></div>';
-            $expected .= '</div>';
-            $this->assertRaw($expected);
+            $expected  = array();
+            $expected[] = '<div class="link-item">';
+            $expected[] = '      <div class="link-title">' . check_plain($title) . '</div>';
+            $expected[] = '    <div class="link-url"><a href="' . check_plain($url) . '">' . check_plain($url_title) . '</a></div>';
+            $expected[] = '</div>';
+            $expected_output = implode("\n", $expected);

I think we should /not/ change the test and instead use the {% spaceless %} tag around the entire codeblock of the template, as previously the markup has had no whitespace.

For the preprocess docblock:

+++ b/core/modules/link/link.module
@@ -408,20 +408,20 @@ function link_theme() {
 /**
- * Formats a link as separate title and URL elements.
+ * Preprocesses variables for link-formatter-link-separate.html.twig.
+ *
+ * @see link-formatter-link-separate.html.twig
  */

As per #1913208: [policy] Standardize template preprocess function documentation:

I think we should edit to this (may or may not want to add the @param $variables in):

/**
 * Prepares variables for separated link field templates.
 *
 * Core template: link-formatter-link-separate.html.twig.
 */

For the preprocess function:

+++ b/core/modules/link/link.module
@@ -408,20 +408,20 @@ function link_theme() {
-  $output .= '<div class="link-url">' . l($vars['url_title'], $vars['href'], $vars['options']) . '</div>';
-  $output .= '</div>';
-  return $output;
+  // @todo revisit this use of l() after http://drupal.org/node/1922454 is resolved.
+  $variables['link'] = l($variables['url_title'], $variables['href'], $variables['options']);

l()/theme('link') should be done soon.

As per Best practices - preprocess functions and templates:

Let's make this l() into a render array, like this:

$variables['link'] = array(
  '#type' => 'link',
  '#title' => $variables['url_title'],
  '#path' => $variables['href'],
  '#options' => $variables['options']
);

And for the template:

--- /dev/null
+++ b/core/modules/link/templates/link-formatter-link-separate.html.twig
@@ -0,0 +1,21 @@
+{#
+/**
+ * @file
+ * Formats a link as separate title and URL elements.
+ *
+ * Available variables:
+ * - link: The link that has already been formatted by l().
+ * - title: The title of the link.
+ *
+ * @see template_preprocess()
+ * @see template_preprocess_link_formatter_link_separate()
+ *
+ * @ingroup themeable

Since, once l()/theme('link') works as expected, we'll have all these variables, lets document them such:

/**
 * @file
 * Default theme implementation of a separated (title and URL) link formatter.
 *
 * Available variables:
 *  - link: A renderable link including the following properties:
 *    - title: The actual content of the link.
 *    - path: The base path of the link.
 *    - options: An array of options, including:
 *      - query: An array of query parameters for the link.
 *      - fragment: A string to be placed after a '#' in the link.
 *  - title: A descriptive or alternate title for the link, which may be different than the actual link text.
 *
 * @see template_preprocess()
 * @see template_preprocess_link_formatter_link_separate()
 *
 * @ingroup themeable
 */

+++ b/core/modules/link/templates/link-formatter-link-separate.html.twig
@@ -0,0 +1,21 @@
+<div class="link-item">
+  {% if title %}
+    <div class="link-title">{{ title }}</div>
+  {% endif %}
+  <div class="link-url">{{ link }}</div>
+</div>

Then wrap {% spaceless %} ... {% endspaceless %} around all 6 lines.

NOTE: Until '#type' 'link' returns an active class for links, this won't pass. So we can either:

a. Keep it failing and dependent upon #1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence and/or #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig, or
b. Insert a hack with @todo remove once above issues are fixed.

Any takers for rolling a new patch based on the feedback in #28? If you have questions you can always stop by #drupal-twig on IRC :)

I will try.

add how to test

Tagging.

Rolling a new patch based on @steveoliver's feedback.

Leaving the l() in for now. It's not perfect but at this point I'm not sure we have the luxury of waiting for #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig.

The automated testing for this output is quite thorough as we've discovered, so I'm removing the manual testing tag I added last night.

Oops, two interdiffs.

Fixed indent level within {% spaceless %} tag.

Seems to work nicely!

Followup docs tweaks as per twig docs

Thanks @chrisjlee! I agree this is good to go.

#39: 1898426-39.patch queued for re-testing.

Add conversion summary table

+++ b/core/modules/link/link.moduleundefined
@@ -417,20 +417,31 @@ function link_theme() {
+  // @todo Revisit this use of l() after http://drupal.org/node/1922454 is
+  //   resolved.
+  $variables['link'] = l($variables['url_title'], $variables['href'], $variables['options']);

#1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig is resolved :)

What do we want to do with this now? #type link?

#39: 1898426-39.patch queued for re-testing.

Profiling

Results, tested as described in summary

=== 8.x..8.x compared (519fbb3f5c2a0..519fbca569199):

ct  : 50,343|50,343|0|0.0%
wt  : 221,119|220,482|-637|-0.3%
cpu : 209,215|208,928|-287|-0.1%
mu  : 14,598,272|14,598,272|0|0.0%
pmu : 14,762,624|14,762,624|0|0.0%

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

Run 519fbb3f5c2a0 uploaded successfully for drupal-perf-drupalcon.
Run 519fbcb7249d1 uploaded successfully for drupal-perf-drupalcon.
=== 8.x..1898426-39-field-link compared (519fbb3f5c2a0..519fbcb7249d1):

ct  : 50,343|50,430|87|0.2%
wt  : 221,119|221,500|381|0.2%
cpu : 209,215|209,535|320|0.2%
mu  : 14,598,272|14,642,552|44,280|0.3%
pmu : 14,762,624|14,805,968|43,344|0.3%

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

Manually testing this now.

HTML output identical.

See attached diff.

Back to RTBC as testbot wasn't actually supposed to test that. See patch in #39

Remove sandbox link

thanks @jerdavis

#39 doesn't apply, needs a reroll.

Unassigning, anyone can work on the reroll.

regarding #39:

+++ b/core/modules/link/link.module
@@ -417,20 +417,31 @@ function link_theme() {
+  // @todo Revisit this use of l() after http://drupal.org/node/1922454 is
+  //   resolved.

This issue was fixed: #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig.

Rerolled the patch.
Removed the following comment.

+  // @todo Revisit this use of l() after http://drupal.org/node/1922454 is
+  //   resolved.

As far I have understood l(), the call is ok by now.

Made an error with the patchfile.
Removed comment is now really removed.

@derheap seems like something went a bit sideways with the re-roll. There is an extra bit at the end with "NodeTestStorageController.php" and "Prepares variables for separated link field templates." rewrite is missing.

Would you mind trying again from #39?
http://drupal.org/patch/reroll

I have a feeling maybe your repo wasn't rebased to origin/8.x, just a guess though...

retagging

Attaching a reroll of patch from #39 and removed the comment as per #60.

@joelpittet: I had some problems with merge conflict. I am just a novice …
@hussainweb: thanks for fixing my reroll.

Okay so this has been profiled, would like one more round of manual testing please.

Thanks everyone for moving this forward :)

@derheap no worries, thanks for the patch:) Merge conflicts can sometimes suck

Updated issue summary.

Update commit message

I just ran a quick test.

1. I applied the patch from from #64.
2. I enabled the Link module.
3. I added a link field to Article content type and created test content. I changed the display formatter to 'Separate title and link' and verified it.

After above steps, I changed the twig file with arbitrary text.

Finally, cleared the cache and refreshed the content page and verified the arbitrary text:

Test result in comment #68.

Sorry, you can't RTBC your own patch @hussainweb. Thanks for testing! We should also ensure the markup is the same before and after applying the patch :)

Got it. I thought it would be fine here because I just rerolled the patch. I will try and compare the markup shortly.

This is the default markup:

<div class="link-item"><div class="link-title">Google</div><div class="link-url"><a href="http://www.google.com/">http://www.google.com/</a></div></div>

This is the markup after applying the patch and clearing the cache. I also added random text and removed it from the twig file to ensure it was being used.

<div class="link-item"><div class="link-title">Google</div><div class="link-url"><a href="http://www.google.com/">http://www.google.com/</a></div></div>

They match exactly as far as I can see.

The patch was implemented and tested and ran perfectly. Everything seems to be working.

Committed 6ff8caa and pushed to 8.x. Thanks!

whoohoo! Thanks @alexpott - and thanks everyone for writing, testing, re-rolling, re-testing!

+ Snacks for the testbots :)