Task

Use Twig instead of PHPTemplate

Remaining

  • Replace all theme functions and templates with .html.twig equivalent templates
  • Add new preprocess functions for the .html.twig equivalent templates
  • Update all hook_theme definitions

Related

#1885560: [meta] Convert everything in theme.inc to Twig
#311011: Replace links.html.twig with item-list--links.html.twig

Files: 
CommentFileSizeAuthor
#112 interdiff.txt3.74 KBjoelpittet
#112 1939064-twig-links-112.patch16.62 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 63,357 pass(es). View
#107 1939064-twig-links-106-reroll.patch13.17 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 63,765 pass(es). View
#102 interdiff.txt1.31 KBCottser
#102 1939064-102.patch13.73 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 63,630 pass(es). View
#100 twig-links-1939064-100.patch13.74 KBInternetDevels
PASSED: [[SimpleTest]]: [MySQL] 63,276 pass(es). View
#98 twig-links-1939064-98.patch13.64 KBmdrummond
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/theme.inc. View
#96 interdiff.txt1.68 KBjoelpittet
#96 1939064-96-twig-links.patch15.12 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 59,341 pass(es). View
#92 1939064-92-twig-links.patch13.93 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 59,208 pass(es), 4 fail(s), and 0 exception(s). View
#86 1939064-85-convert-theme-links.patch19.67 KBmark.labrecque
FAILED: [[SimpleTest]]: [MySQL] 57,933 pass(es), 70 fail(s), and 23 exception(s). View
#83 1939064-79-covert-theme-links.patch18.74 KBmark.labrecque
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/theme.inc. View
#79 interdiff.txt4.02 KBjoelpittet
#79 1939064-79-twig-links.patch14.53 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1939064-79-twig-links.patch. Unable to apply patch. See the log in the details link for more information. View
#76 1939064-76-twig-links.patch15.98 KBdrupalninja99
FAILED: [[SimpleTest]]: [MySQL] 59,364 pass(es), 3 fail(s), and 0 exception(s). View
#75 interdiff-62-69.txt7.66 KBBarisW
#75 interdiff-69-71.txt4.51 KBBarisW
#71 1939064-71-twig-links.patch16.54 KBfarrington
FAILED: [[SimpleTest]]: [MySQL] 58,672 pass(es), 3 fail(s), and 0 exception(s). View
#69 1939064-68-twig-links.patch16.56 KBfarrington
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1939064-68-twig-links.patch. Unable to apply patch. See the log in the details link for more information. View
#67 1939064-67-twig-links.patch14.17 KBfarrington
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site. View
#62 1939064-62-twig-links.patch15.34 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 59,276 pass(es). View
#62 interdiff.txt8.35 KBjoelpittet
#56 interdiff.txt942 bytesjoelpittet
#56 1939064-56-twig-links.patch8.45 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 58,625 pass(es). View
#54 1939064-54-twig-links-reroll.patch8.44 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 58,514 pass(es). View
#54 1939064-54-twig-links.patch8.44 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 58,531 pass(es), 3 fail(s), and 0 exception(s). View
#54 interdiff.txt1.99 KBjoelpittet
#44 1939064-44.patch8.42 KBpwieck
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1939064-44.patch. Unable to apply patch. See the log in the details link for more information. View
#44 1939064-41-44-interdiff.txt1.48 KBpwieck
#41 1939064-41.patch8.94 KBpwieck
PASSED: [[SimpleTest]]: [MySQL] 57,942 pass(es). View
#37 1939064-37.patch8.99 KBpwieck
PASSED: [[SimpleTest]]: [MySQL] 57,875 pass(es). View
#37 1939064-27-37-interdiff.txt1.06 KBpwieck
#34 1939064-34.patch9.03 KBpwieck
FAILED: [[SimpleTest]]: [MySQL] 57,618 pass(es), 4 fail(s), and 23,035 exception(s). View
#34 1939064-27-34-interdiff.txt1.06 KBpwieck
#27 1939064-27.patch8.98 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 56,005 pass(es). View
#27 interdiff.txt499 bytesCottser
#23 interdiff.txt979 bytesjoelpittet
#23 1939064-23-twig-links.patch9.05 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 54,438 pass(es). View
#21 interdiff-19-20.txt3.01 KBjoelpittet
#20 1939064-20-twig-links.patch8.25 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 54,598 pass(es), 4 fail(s), and 0 exception(s). View
#20 interdiff.txt5.62 KBjoelpittet
#19 twig-convert_links-1939064-19.patch8.74 KBjenlampton
FAILED: [[SimpleTest]]: [MySQL] 54,590 pass(es), 4 fail(s), and 0 exception(s). View
#19 interdiff.txt4.88 KBjenlampton
#16 interdiff.txt1.5 KBjoelpittet
#16 1939064-16-twig-links.patch9.01 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 54,471 pass(es), 4 fail(s), and 0 exception(s). View
#14 1939064-14-twig-links.patch8.87 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 54,420 pass(es), 7 fail(s), and 0 exception(s). View
#14 interdiff.txt3.53 KBjoelpittet
#12 interdiff.txt1.99 KBjoelpittet
#12 1939064-12-twig-links.patch8.83 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 54,227 pass(es), 9 fail(s), and 1 exception(s). View
#3 twig-theme-links-1939064-3.patch8.33 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 53,931 pass(es), 8 fail(s), and 1 exception(s). View
#2 twig-theme-links-1939064-2.patch5.35 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site. View

Comments

psynaptic’s picture

Assigned: Unassigned » psynaptic
joelpittet’s picture

Assigned: psynaptic » joelpittet
Status: Active » Needs work
FileSize
5.35 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site. View

Left the debug messages where I am having trouble with the active link tests.

Tests that are failing are at
/Users/joel/Sites/d8/html/core/modules/system/lib/Drupal/system/Tests/Theme/FunctionsTest.php

one is because of template_process line #2642

  // Initialize html class attribute for the current hook.
  $variables['attributes']['class'][] = drupal_html_class($hook);

The other is because they expect being an active link in the tests.

Have a crack:)

joelpittet’s picture

FileSize
8.33 KB
FAILED: [[SimpleTest]]: [MySQL] 53,931 pass(es), 8 fail(s), and 1 exception(s). View

Whoops add template

tlattimore’s picture

Status: Needs work » Needs review

Not sure if this is supposed to be tagged for review, but I don't see anything noting that it shouldn't be.

Status: Needs review » Needs work

The last submitted patch, twig-theme-links-1939064-3.patch, failed testing.

joelpittet’s picture

Status: Needs work » Postponed

Needs this patch for part of this
#1938430: Don't add a default theme hook class in template_preprocess() where the extra class is added.

-
-  // Initialize html class attribute for the current hook.
-  $variables['attributes']['class'][] = drupal_html_class($hook);

And may need this patch as well (which could be tested) for the active class on the links...
#1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig

@steveoliver was having a look at those two items as well. Correct me if I am wrong but you came to the same conclusion?

Cottser’s picture

I would think #1938430: Don't add a default theme hook class in template_preprocess() can be worked around/overridden in preprocess just by instantiating a new Attribute object, but I can definitely understand postponing on #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig.

joelpittet’s picture

@Cottser, would overriding the template_preprocess() need to be removed at a later date?

Cottser’s picture

Not necessarily, but I see your point.

Cottser’s picture

Issue tags: +Needs manual testing

This could use manual testing once we pick it back up.

Cottser’s picture

Status: Postponed » Needs work
joelpittet’s picture

Status: Needs work » Needs review
FileSize
8.83 KB
FAILED: [[SimpleTest]]: [MySQL] 54,227 pass(es), 9 fail(s), and 1 exception(s). View
1.99 KB

tweaked up the preprocess bit to remove the links... but having issues on the active class on Theme functions in Theme tests.

Status: Needs review » Needs work

The last submitted patch, 1939064-12-twig-links.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
3.53 KB
8.87 KB
FAILED: [[SimpleTest]]: [MySQL] 54,420 pass(es), 7 fail(s), and 0 exception(s). View

Ok so still don't know what to do about the active links test but this should solve the exception.

Status: Needs review » Needs work

The last submitted patch, 1939064-14-twig-links.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
9.01 KB
FAILED: [[SimpleTest]]: [MySQL] 54,471 pass(es), 4 fail(s), and 0 exception(s). View
1.5 KB

@Cottser, thanks for the suggestion to look at the span attributes, it was pulling the wrong bits and I spotted another missing bit. item.text was missing {{}} on the last part.

So with this patch the only bit left to fix is the 'active' class I hope.

Status: Needs review » Needs work

The last submitted patch, 1939064-16-twig-links.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

added item_list issue

jenlampton’s picture

Assigned: joelpittet » jenlampton

Looks like a few tests need to be updated. on it.

jenlampton’s picture

Assigned: jenlampton » Unassigned
FileSize
4.88 KB
8.74 KB
FAILED: [[SimpleTest]]: [MySQL] 54,590 pass(es), 4 fail(s), and 0 exception(s). View

Okay, well it wasn't the tests that were the problem. For some reason the {% spaceless %} tag doesn't seem to do anything within the for loop. I added some whitepsace controlers in there to take care of the line breaks and spaces.

The tests are also testing for the ability to add an ID for the heading. We didn't have any code in our preprocess for IDs, only classes, so I added that.

It also looks like for some reason our front page link is getting an active class (both the A tag and the LI tag) when tests are running. Not sure why that is, but it's definitely an issue. in our preprocess we have $is_current_path = ($link['href'] == current_path() || ($link['href'] == '<front>' && drupal_is_front_page())); Not sure why this wold return true during testing, but it does.

I'm not sure the tests are actually running our preprocess function either, even after making the changes noted above, the tests still fail with missing IDs on the heading tags.

still needs work.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
5.62 KB
8.25 KB
FAILED: [[SimpleTest]]: [MySQL] 54,598 pass(es), 4 fail(s), and 0 exception(s). View

Changes from #19 incorporated. Thank you @jenlampton docs and whitepsace. Then reverted my edits for the header[id] bug you found.

joelpittet’s picture

FileSize
3.01 KB

@jenlampton sorry my interdiff was messed up. This it the diff between #19-#20.

Status: Needs review » Needs work

The last submitted patch, 1939064-20-twig-links.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
9.05 KB
PASSED: [[SimpleTest]]: [MySQL] 54,438 pass(es). View
979 bytes

Ok so drupal_is_front_page() call wasn't releasing the static variable so I added a bounce sheet to the tests so that it would pickup the new "front page". i.e. drupal_static_reset('drupal_is_front_page');

This should pass test as they passed locally. *crosses fingers*

Cottser’s picture

Green! Fantastic work @joelpittet and @jenlampton!

jenlampton’s picture

Assigned: Unassigned » jenlampton

reviewing :)

jenlampton’s picture

Assigned: jenlampton » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

Manual testing:
The only difference in the markup is the order of the classes on the UL tag, great job guys!

before:

<ul class="links inline"><li class="node-readmore odd first"><a href="/node/1" rel="tag" title="I am an article">Read more<span class="element-invisible"> about I am an article</span></a></li><li class="comment-comments even"><a href="/node/1#comments" title="Jump to the first comment of this posting." property="sioc:num_replies" content="1" datatype="xsd:integer" rel="">1 comment</a></li><li class="comment-add odd last"><a href="/node/1#comment-form" title="Add a new comment to this page.">Add new comment</a></li></ul>

after:

<ul class="inline links"><li class="node-readmore odd first"><a href="/node/1" rel="tag" title="I am an article">Read more<span class="element-invisible"> about I am an article</span></a></li><li class="comment-comments even"><a href="/node/1#comments" title="Jump to the first comment of this posting." property="sioc:num_replies" content="1" datatype="xsd:integer" rel="">1 comment</a></li><li class="comment-add odd last"><a href="/node/1#comment-form" title="Add a new comment to this page.">Add new comment</a></li></ul>

Docs look pretty good, verbose, but more is better than less. :)

Cottser’s picture

FileSize
499 bytes
8.98 KB
PASSED: [[SimpleTest]]: [MySQL] 56,005 pass(es). View

Just fixing indent level and removing a consolidation-type @todo, see #1757550-38: [Meta] Convert core theme functions to Twig templates for reasoning on the @todo removal.

Status: Reviewed & tested by the community » Needs work
Issue tags: -Twig

The last submitted patch, 1939064-27.patch, failed testing.

Cottser’s picture

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

#27: 1939064-27.patch queued for re-testing.

Cottser’s picture

Status: Needs review » Reviewed & tested by the community

And back to RTBC per #26.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs profiling
joelpittet’s picture

Issue tags: +Needs reroll

patch does not apply, needs a re-roll on #27

pwieck’s picture

Assigned: Unassigned » pwieck

Re-rolling #27 now

pwieck’s picture

Status: Needs work » Needs review
FileSize
1.06 KB
9.03 KB
FAILED: [[SimpleTest]]: [MySQL] 57,618 pass(es), 4 fail(s), and 23,035 exception(s). View

re-rolled

Status: Needs review » Needs work

The last submitted patch, 1939064-34.patch, failed testing.

pwieck’s picture

I will fix this re-roll my mistake.

pwieck’s picture

Assigned: pwieck » Unassigned
Status: Needs work » Needs review
FileSize
1.06 KB
8.99 KB
PASSED: [[SimpleTest]]: [MySQL] 57,875 pass(es). View

By mistake, in re-roll #34, I removed $language_url = language(Language::TYPE_URL);

pwieck’s picture

Issue tags: -Needs reroll

#37 passed and ready for review

tlattimore’s picture

Status: Needs review » Needs work

Here's some cleanup that needs to be done.

+++ b/core/includes/theme.incundefined
@@ -1687,13 +1689,21 @@ function theme_link($variables) {
+  // @todo Remove after http://drupal.org/node/1938430 is resolved.

Does this @todo still apply? Looks like #1938430: Don't add a default theme hook class in template_preprocess() is fixed.

+++ b/core/includes/theme.incundefined
@@ -1744,45 +1749,37 @@ function theme_links($variables) {
+          $item += array(
+++ b/core/includes/theme.incundefined
@@ -1744,45 +1749,37 @@ function theme_links($variables) {
+          $item += array(

I have never seen += used for concatenation PHP, nor do I believe it will work in. To add additinal keys to an array shouldn't we be doing something like $items[] = array();?

+++ b/core/modules/system/templates/links.html.twigundefined
@@ -0,0 +1,64 @@
+ *   the 'element-invisible' CSS class. Do not use 'display:none', which

This needs to updated per #1363112: Simplify names of "element-x" helper classes to mention 'visually-hidden' and not 'element-invisible'.

pwieck’s picture

Assigned: Unassigned » pwieck

Fixing patch

pwieck’s picture

Status: Needs work » Needs review
FileSize
8.94 KB
PASSED: [[SimpleTest]]: [MySQL] 57,942 pass(es). View

Made corrections stated in #39.

Did not change: '$item += array(' because this is proper per Cottser.

pwieck’s picture

Assigned: pwieck » Unassigned

#41 Passed still needs profiling by someone.

joelpittet’s picture

Status: Needs review » Needs work
+++ b/core/includes/theme.inc
@@ -1677,13 +1679,20 @@ function theme_status_messages($variables) {
+
+  // Remove 'links' from the attributes array. This is added through
+  // template_preprocess() but we do not need it.
+  if (isset($variables['attributes']['class'][0]) && $variables['attributes']['class'][0] == 'links') {
+    unset($variables['attributes']['class'][0]);
+    if (empty($variables['attributes']['class'])) {
+      unset($variables['attributes']['class']);
+    }
+  }
+  $variables['attributes'] = new Attribute($variables['attributes']);

This whole chunk can be removed.

+++ b/core/modules/system/templates/links.html.twig
@@ -0,0 +1,64 @@
+ *   If the heading is an array, it can have the following elements:

Shouldn't say the type in a twig doc, array.
Just remove this line.

+++ b/core/modules/system/templates/links.html.twig
@@ -0,0 +1,64 @@
+ *   - attributes: (optional) A keyed array of attributes for the heading.

This should not say 'array' this should read a list.

pwieck’s picture

Status: Needs work » Needs review
FileSize
1.48 KB
8.42 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1939064-44.patch. Unable to apply patch. See the log in the details link for more information. View

Made changes from #43

pwieck’s picture

#44 passed still needs review and profiling.

siccababes’s picture

Status: Needs review » Reviewed & tested by the community

I added three articles to my site, and I found that they all had read more links. These links seem to be rendered through theme_links(). This patch seems to be working fine.

Cottser’s picture

Status: Reviewed & tested by the community » Needs review

Thanks for reviewing @siccababes :) this patch still needs to be profiled to see how it performs in comparison to the theme function.

jerdavis’s picture

Assigned: Unassigned » jerdavis

Profiling

jerdavis’s picture

Scenario
- Devel generate
- Generate 50 menu links in the Main Navigation menu. Allow up to 50 links on the menu with a depth of 1

=== 8.x..8.x compared (51ec5ca916d7d..51ec5cfb002ea):

ct  : 133,342|133,342|0|0.0%
wt  : 532,495|534,057|1,562|0.3%
cpu : 518,900|518,998|98|0.0%
mu  : 18,330,840|18,330,840|0|0.0%
pmu : 18,391,368|18,391,368|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51ec5ca916d7d&...

=== 8.x..1939064-theme_links compared (51ec5ca916d7d..51ec5d631cb28):

ct  : 133,342|135,809|2,467|1.9%
wt  : 532,495|540,491|7,996|1.5%
cpu : 518,900|527,997|9,097|1.8%
mu  : 18,330,840|18,395,136|64,296|0.4%
pmu : 18,391,368|18,457,408|66,040|0.4%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51ec5ca916d7d&...

jerdavis’s picture

Assigned: jerdavis » Unassigned
Issue tags: -Needs profiling

Removing the needs profiling tag

jibran’s picture

Issue tags: -Twig

#44: 1939064-44.patch queued for re-testing.

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

The last submitted patch, 1939064-44.patch, failed testing.

joelpittet’s picture

Issue tags: +Novice, +Needs reroll
joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Novice, -Needs reroll
FileSize
1.99 KB
8.44 KB
FAILED: [[SimpleTest]]: [MySQL] 58,531 pass(es), 3 fail(s), and 0 exception(s). View
8.44 KB
PASSED: [[SimpleTest]]: [MySQL] 58,514 pass(es). View

Re-rolled and removed extra whitespace modifiers and intented the contents of the spaceless control. Also removed the @see template_preprocess() in the docblock.

Status: Needs review » Needs work

The last submitted patch, 1939064-54-twig-links.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
8.45 KB
PASSED: [[SimpleTest]]: [MySQL] 58,625 pass(es). View
942 bytes

Hmm forgot that the modifiers are need inside the loop. Spaceless doesn't penetrate the for loop.

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

The last submitted patch, 1939064-56-twig-links.patch, failed testing.

joelpittet’s picture

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

#56: 1939064-56-twig-links.patch queued for re-testing.

joelpittet’s picture

I re-ran the profiling similar but I turned on all permissions for anonymous and did 250 links on all menus using devel. So this should also grab a bit of menus that are generated in admin as well.
Without the spaceless things looks better on the # of function calls.

=== 8.x..8.x compared (52353d82849e7..52353dd1139c1):

ct  : 70,607|70,607|0|0.0%
wt  : 671,741|671,711|-30|-0.0%
cpu : 637,211|636,387|-824|-0.1%
mu  : 40,771,304|40,771,304|0|0.0%
pmu : 40,894,968|40,894,968|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52353d82849e7&...

=== 8.x..1939064-twig-links compared (52353d82849e7..52353e715a325):

ct  : 70,607|71,206|599|0.8%
wt  : 671,741|678,259|6,518|1.0%
cpu : 637,211|643,214|6,003|0.9%
mu  : 40,771,304|40,880,232|108,928|0.3%
pmu : 40,894,968|41,003,104|108,136|0.3%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52353d82849e7&...

jibran’s picture

+++ b/core/includes/theme.inc
@@ -1664,21 +1663,17 @@ function theme_links($variables) {
+      $heading['text'] = check_plain($heading['text']);

We can use String::checkPlain now.

thedavidmeister’s picture

Status: Needs review » Needs work

So, the function theme_links() no longer exists in this patch yet we have a bunch of documentation that still references it. I found 14 matches for 'theme_links()' with this patch applied.

Also, in reference to #60, as well as check_plain() being used, it's also referenced in the documentation of links.html.twig

joelpittet’s picture

Status: Needs work » Needs review
FileSize
8.35 KB
15.34 KB
PASSED: [[SimpleTest]]: [MySQL] 59,276 pass(es). View

Thanks @jibran and @thedavidmeister! Good catches!

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

The last submitted patch, 1939064-62-twig-links.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review

#62: 1939064-62-twig-links.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1939064-62-twig-links.patch, failed testing.

joelpittet’s picture

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

#62: 1939064-62-twig-links.patch queued for re-testing.

farrington’s picture

FileSize
14.17 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site. View

In the preprocess the heading.level is not assigned with any value, and on the other hand in the old theme_links() it only sett statically to 'h2'.

So there seems be no reason to keep any of that in the preprocess nor in the twig file.

The documentation is also edited to reflect these changes.

Status: Needs review » Needs work

The last submitted patch, 1939064-67-twig-links.patch, failed testing.

farrington’s picture

Status: Needs work » Needs review
FileSize
16.56 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1939064-68-twig-links.patch. Unable to apply patch. See the log in the details link for more information. View

A small error got in to the previous patch file. (The twig file was missing)

Here's the correct file.

Status: Needs review » Needs work

The last submitted patch, 1939064-68-twig-links.patch, failed testing.

farrington’s picture

Status: Needs work » Needs review
FileSize
16.54 KB
FAILED: [[SimpleTest]]: [MySQL] 58,672 pass(es), 3 fail(s), and 0 exception(s). View

Use the correct (?) method of adding new files "git diff"...

BarisW’s picture

Status: Needs review » Needs work

The last submitted patch, 1939064-71-twig-links.patch, failed testing.

joelpittet’s picture

@farrington could you please provide an interdiff for the changes between #62 - #69 and #69to #71?

That way we can easily what you changed between the patches.

Thank you for working on this!

BarisW’s picture

FileSize
4.51 KB
7.66 KB

Here are the interdiffs.

drupalninja99’s picture

Status: Needs work » Needs review
FileSize
15.98 KB
FAILED: [[SimpleTest]]: [MySQL] 59,364 pass(es), 3 fail(s), and 0 exception(s). View

I had to do a reroll of the patch as it would no longer apply.

Status: Needs review » Needs work

The last submitted patch, 1939064-76-twig-links.patch, failed testing.

joelpittet’s picture

Assigned: Unassigned » joelpittet

I really like that the template now has the h2 in it instead of a variable for it's level. Thanks @farrington:) and thank you @drupalninja99 for the re-roll. I'm going to see if I can't get rid of those errors, now which are very likely related to the removal of the level variable. This may constitute as an API change... but let's hope not because it shouldn't be so.

joelpittet’s picture

Assigned: joelpittet » Unassigned
Status: Needs work » Needs review
FileSize
14.53 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1939064-79-twig-links.patch. Unable to apply patch. See the log in the details link for more information. View
4.02 KB

On second thought, I'd really like to keep this issue as a straight conversion issue, get it into core first. I've created a follow up issue once this is complete and in core.
#2116359: Leave the heading level in the template and out of the variables for links.html.twig

Meantime this is the re-rolled and reverted those changes.

joelpittet’s picture

Issue summary: View changes

Remove sandbox link

joelpittet’s picture

79: 1939064-79-twig-links.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 79: 1939064-79-twig-links.patch, failed testing.

mark.labrecque’s picture

Assigned: Unassigned » mark.labrecque
Issue summary: View changes
mark.labrecque’s picture

Status: Needs work » Needs review
FileSize
18.74 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/theme.inc. View

Rerolled the latest patch

Status: Needs review » Needs work

The last submitted patch, 83: 1939064-79-covert-theme-links.patch, failed testing.

mark.labrecque’s picture

Status: Needs work » Needs review
mark.labrecque’s picture

FileSize
19.67 KB
FAILED: [[SimpleTest]]: [MySQL] 57,933 pass(es), 70 fail(s), and 23 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 86: 1939064-85-convert-theme-links.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 86: 1939064-85-convert-theme-links.patch, failed testing.

joelpittet’s picture

+++ b/core/includes/menu.inc
@@ -2186,6 +2186,114 @@ function _menu_get_legacy_tasks($router_item, &$data, &$root_path) {
+ * Retrieves contextual links for a path based on registered local tasks.
+ *
+ * This leverages the menu system to retrieve the first layer of registered
+ * local tasks for a given system path. All local tasks of the tab type
+ * MENU_CONTEXT_INLINE are taken into account.
+ *
...
+function menu_contextual_links($module, $parent_path, $args) {
...

looks like menu_contextual_links snuck back into this patch. I doubt that has to do with the errors but compare yours to the one you are rerolling and you may spot the answer.

joelpittet’s picture

Sorry, @mark.labrecque this issue is a bit trickier than I originally thought for the re-roll. A bunch of router related stuff for links got in there and some of the code improvements were made similarly when those got in. I'll try to do a re-roll here from scratch.

joelpittet’s picture

Assigned: mark.labrecque » Unassigned
Status: Needs work » Needs review
Parent issue: » #1757550: [Meta] Convert core theme functions to Twig templates
Related issues: +#1885560: [meta] Convert everything in theme.inc to Twig
FileSize
13.93 KB
FAILED: [[SimpleTest]]: [MySQL] 59,208 pass(es), 4 fail(s), and 0 exception(s). View

I am sure this will fail, but it seems to be because the active classes aren't working... again. That static reset for the homepage didn't seem to fix it so I removed that for now. I also removed spaceless for now and just added more modifiers but would rather have no whitespace modifiers it's quite a task to rewrite all the tests to be xpath or something that doesn't care about whitespace.

It has to do with the fallback in _current_path in current_path... this is referencing #1269742: Make path lookup code into a pluggable class.

Let's see what the errors look like though from testbot...

joelpittet’s picture

Here's the failed output for reference:
Result:
'<h2>Links heading</h2><ul id="somelinks"><li class="a-link odd first"><a href="/a/link">A &lt;link&gt;</a></li><li class="plain-text even">Plain &quot;text&quot;</li><li class="front-page odd"><a href="/">Front page</a></li><li class="router-test even last"><a href="/router_test/test1">Test route</a></li></ul>'

Expected:

'<h2>Links heading</h2><ul id="somelinks"><li class="a-link odd first"><a href="/a/link">A &lt;link&gt;</a></li><li class="plain-text even">Plain &quot;text&quot;</li><li class="front-page odd active"><a href="/" class="active">Front page</a></li><li class="router-test even last"><a href="/router_test/test1">Test route</a></li></ul>'

Status: Needs review » Needs work

The last submitted patch, 92: 1939064-92-twig-links.patch, failed testing.

joelpittet’s picture

The good news: there are only 4 fails and they are the ones I knew would fail.
The bad news: I've no idea how to make the drupal_get_front_page() know it's active inside the test.

Help please!

joelpittet’s picture

Status: Needs work » Needs review
FileSize
15.12 KB
PASSED: [[SimpleTest]]: [MySQL] 59,341 pass(es). View
1.68 KB

@larowlan++ helped me out here. Turns out the l() was not being active because it was getting the query params from the Request service. So in the test I disabled them.

Cottser’s picture

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

Tagging for reroll.

mdrummond’s picture

Status: Needs work » Needs review
FileSize
13.64 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/theme.inc. View

Reroll.

Status: Needs review » Needs work

The last submitted patch, 98: twig-links-1939064-98.patch, failed testing.

InternetDevels’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
13.74 KB
PASSED: [[SimpleTest]]: [MySQL] 63,276 pass(es). View
joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

I've got some better performance this time, likely due to the twig engine updates to 1.15.

=== 8.x..8.x compared (52e6fc5cc809e..52e6fca507bc1):

ct  : 73,852|73,852|0|0.0%
wt  : 502,535|502,970|435|0.1%
cpu : 493,614|493,689|75|0.0%
mu  : 32,302,512|32,302,512|0|0.0%
pmu : 32,425,320|32,425,320|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52e6fc5cc809e&...

=== 8.x..1939064-twig-links compared (52e6fc5cc809e..52e6fcee66138):

ct  : 73,852|74,438|586|0.8%
wt  : 502,535|505,093|2,558|0.5%
cpu : 493,614|496,230|2,616|0.5%
mu  : 32,302,512|32,393,072|90,560|0.3%
pmu : 32,425,320|32,514,400|89,080|0.3%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52e6fc5cc809e&...

Also did another markup review and there are no differences there too.
Passes testbot, this one is ready to go.

Cottser’s picture

FileSize
13.73 KB
PASSED: [[SimpleTest]]: [MySQL] 63,630 pass(es). View
1.31 KB
  1. +++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
    @@ -2843,7 +2843,7 @@ protected function assertNoTitle($title, $message = '', $group = 'Other') {
    -   *   The name of the theme function to invoke; e.g. 'links' for theme_links().
    +   *   The name of the theme function to invoke; e.g. 'links' for links.html.twig.
    

    This line is over 80 characters now so should be wrapped.

  2. +++ b/core/includes/theme.inc
    @@ -1258,15 +1258,14 @@ function theme_links($variables) {
    +      // Convert the attributes array into an attributes object.
    

    This should probably say "an Attribute object".

Rolling in those changes since they are so minor. For #1 I just reworded the comment.

joelpittet’s picture

Still RTBC:) Thanks @Cottser

alexpott’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

1939064-102.patch no longer applies.


alexpott’s picture

Issue summary: View changes
joelpittet’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll

That was a tricky re-roll for such a simple change. Just dropped the hunk:

diff --git a/core/modules/system/tests/modules/ajax_test/ajax_test.module b/core/modules/system/tests/modules/ajax_test/ajax_test.module
index 2ea9eb9..e78d163 100644
--- a/core/modules/system/tests/modules/ajax_test/ajax_test.module
+++ b/core/modules/system/tests/modules/ajax_test/ajax_test.module
@@ -120,7 +120,7 @@ function ajax_test_dialog() {
     ),
   );

-  // Dialog behavior applied to links rendered by theme_links().
+  // Dialog behavior applied to links rendered by links.html.twig.
   $build['links'] = array(
     '#theme' => 'links',
     '#links' => array(

Back to RTBC.

joelpittet’s picture

FileSize
13.17 KB
PASSED: [[SimpleTest]]: [MySQL] 63,765 pass(es). View

Oh yeah here's the patch.

xjm’s picture

Issue tags: +API change

I think this probably needs some change notice action, or maybe an update to an existing change notice for theme_*() removals? Can someone list change records that might need an update here? Otherwise, if we should draft a new one instead, we can do that now!
https://groups.drupal.org/node/402688

Cottser’s picture

Issue tags: -API change

Talked to @xjm briefly in IRC about this.

This is just a conversion so probably not an API change (calling code still specifies '#theme' => 'links' in a render array), but we'll want to update the change records listed at the following link (ones that reference theme_links()) after commit:

https://drupal.org/list-changes/published/drupal?keywords_description=th...

joelpittet’s picture

I don't think we did that because we just remove the theme function and swap with a template file. Maybe should just haven't done it yet for any of them in #1757550: [Meta] Convert core theme functions to Twig templates

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

After applying this patch there are still 4 mentions of theme_links() in the code. As this function is being removed that's surprising.

$ grep -r "theme_links" ./
.//includes/theme.inc: *   - set_active_class: (optional) Whether theme_links() should compare the
.//includes/theme.inc: * theme_links() unfortunately duplicates the "active" class handling of l() and
.//modules/system/system.module:  // @see theme_links()
.//modules/system/tests/modules/ajax_test/lib/Drupal/ajax_test/Controller/AjaxTestController.php:    // Dialog behavior applied to links rendered by theme_links().
joelpittet’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
16.62 KB
PASSED: [[SimpleTest]]: [MySQL] 63,357 pass(es). View
3.74 KB

Thanks @alexpott those must have snuck in there from the looks of them when some of the url routing changes and theme_links type=>link consolidation was going on.

Cottser’s picture

+1, looks good. Thanks @joelpittet and thanks for catching that @alexpott!

I re-ran the grep from #111 and just got some .patch files I had kicking around from this issue :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 112: 1939064-twig-links-112.patch, failed testing.

The last submitted patch, 112: 1939064-twig-links-112.patch, failed testing.

joelpittet’s picture

112: 1939064-twig-links-112.patch queued for re-testing.

joelpittet’s picture

Random testbot fails.

The last submitted patch, 112: 1939064-twig-links-112.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review

Testbot you can do it!

joelpittet’s picture

112: 1939064-twig-links-112.patch queued for re-testing.

Cottser’s picture

Status: Needs review » Reviewed & tested by the community
Cottser’s picture

Issue tags: +Twig conversion
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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