Task

Convert theme_ functions to Twig templates.

Steps to Test

  1. function node_add_list -- view source at /node/add
  2. function custom_block_add_list -- view source at /block/add
    Must add a custom block type so as to see the list
    [admin/structure/block/block-content]
  3. function admin_block_content -- view source at /admin/structure/display-modes/view/add
  4. function seven_preprocess_tablesort_indicator /admin/reports/dblog

Remaining

  • theme_ function -> Twig conversions (and preprocess function additions or changes) moved over from the existing conversion issue
  • Patch review
Theme function name Conversion status
seven_admin_block_content converted
seven_custom_block_add_list converted
seven_node_add_list converted
seven_tablesort_indicator converted
Files: 
CommentFileSizeAuthor
#178 interdiff.txt882 bytesjoelpittet
#178 1987424-twig-seven-theme-178.patch9.78 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,932 pass(es).
[ View ]
#171 interdiff.txt4.82 KBCottser
#171 interdiff-3-label-to-title.txt1.05 KBCottser
#171 interdiff-2-block_content.txt2.12 KBCottser
#171 interdiff-1-docs.txt2.7 KBCottser
#171 1987424-171.patch9.76 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,125 pass(es).
[ View ]

Comments

Cottser’s picture

Issue summary:View changes

Updated issue summary.

c4rl’s picture

Title:Seven theme - Convert PHPTemplate templates to Twig» seven.theme - Convert theme_ functions to Twig

Per #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to theme_ functions rather than templates. See #1938848: seven.theme - Convert PHPTemplate templates to Twig for template conversion.

c4rl’s picture

Issue summary:View changes

Updated issue summary.

jenlampton’s picture

Status:Active» Needs review
StatusFileSize
new8.77 KB
PASSED: [[SimpleTest]]: [MySQL] 55,893 pass(es).
[ View ]

here's the conversion of only the theme function overrides.

TommyK’s picture

TommyK’s picture

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

I found that the description of the content type and the custom block is missing from the lists. I will add this in an upcoming patch.

TommyK’s picture

Assigned:TommyK» Unassigned
Status:Needs work» Needs review
StatusFileSize
new1.01 KB
new8.95 KB
PASSED: [[SimpleTest]]: [MySQL] 55,626 pass(es).
[ View ]

Adds description keys to the arrays for custom blocks and content types.

Pierre Paul Lefebvre’s picture

I've set the default theme to Seven. Created a node and promoted it to frontpage.
Run 519fc53e46a65 uploaded successfully for drupal-perf-drupalcon.
Run 519fc817b51ae uploaded successfully for drupal-perf-drupalcon.

=== 8.x..8.x compared (519fc53e46a65..519fc817b51ae):

ct  : 26,146|26,146|0|0.0%
wt  : 89,244|84,689|-4,555|-5.1%
cpu : 88,000|80,000|-8,000|-9.1%
mu  : 12,862,056|12,862,056|0|0.0%
pmu : 12,954,008|12,954,008|0|0.0%

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

Run 519fc53e46a65 uploaded successfully for drupal-perf-drupalcon.
Run 519fc84e17238 uploaded successfully for drupal-perf-drupalcon.

=== 8.x..twig-convert_seven_theme_funcs-1987424-2.patch compared (519fc53e46a65..519fc84e17238):

ct  : 26,146|26,146|0|0.0%
wt  : 89,244|88,702|-542|-0.6%
cpu : 88,000|84,000|-4,000|-4.5%
mu  : 12,862,056|12,869,864|7,808|0.1%
pmu : 12,954,008|12,964,552|10,544|0.1%

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

rupl’s picture

Status:Needs review» Reviewed & tested by the community

Functional review went great. Steps:

  1. Standard install was successful with patch applied
  2. Loaded /node/add, saw list as expected
  3. Deleted both content types, went to /node/add, saw message as expected
  4. Added a content type with malicious description (<script> tag), no problems there

I can't find an example of the 'administrative block' template, but the format is similar to the other twig files that it seems good to go.

sarahjean’s picture

I was able to apply the patch, I didn't test install.
I can verify the message was on node/add, as well as block/add. The message was also present for a custom block type.

OpenChimp’s picture

Issue tags:-Needs profiling
OpenChimp’s picture

Issue tags:+Needs profiling

profiling not run correctly. Needs to be redone

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

The last submitted patch, twig-convert_seven_theme_funcs-1987424-5.patch, failed testing.

tlattimore’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, twig-convert_seven_theme_funcs-1987424-5.patch, failed testing.

Cottser’s picture

Status:Needs work» Needs review
Issue tags:+Needs profiling, +Twig
tlattimore’s picture

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

I'll work on profiling this.

tlattimore’s picture

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

Here is some of profiling results.

Scenario:

  • Made seven as default theme
  • Set node/add as the frontpage
  • Gave anonymous users permission to access all node / content related admin pages

Run 51af3d98520b7 uploaded successfully for drupal-perf-drupalcon.
Run 51af430daedb2 uploaded successfully for drupal-perf-drupalcon.
=== 8.x..8.x compared (51af3d98520b7..51af430daedb2):

ct  : 33,916|33,916|0|0.0%
wt  : 301,287|299,935|-1,352|-0.4%
cpu : 288,685|286,864|-1,821|-0.6%
mu  : 42,690,888|42,690,888|0|0.0%
pmu : 42,725,560|42,725,560|0|0.0%

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

Run 51af3d98520b7 uploaded successfully for drupal-perf-drupalcon.
Run 51af435181b2e uploaded successfully for drupal-perf-drupalcon.
=== 8.x..1987424-5 compared (51af3d98520b7..51af435181b2e):

ct  : 33,916|34,004|88|0.3%
wt  : 301,287|300,489|-798|-0.3%
cpu : 288,685|287,731|-954|-0.3%
mu  : 42,690,888|42,759,296|68,408|0.2%
pmu : 42,725,560|42,792,624|67,064|0.2%

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

tlattimore’s picture

Issue tags:-Needs profiling

Removing needs profiling tag. We'll see what others think about the profiling results.

joelpittet’s picture

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

Minor nitpick re #5

+++ b/core/themes/seven/seven.theme
@@ -39,93 +39,62 @@ function seven_preprocess_page(&$vars) {
/**
- * Overrides theme_admin_block_content().
+ * Implements hook_preprocess_HOOK() for theme_admin_block_content().

Should be for admin-block-content.html.twig. like the rest.

eromero1’s picture

Assigned:Unassigned» eromero1

dibs

sbudker1’s picture

Assigned:eromero1» sbudker1

dibS!

eromero1’s picture

Assigned:sbudker1» eromero1

dibs

sbudker1’s picture

Assigned:eromero1» sbudker1

dibs!

eromero1’s picture

Assigned:sbudker1» eromero1

dibs!!

sbudker1’s picture

Assigned:eromero1» sbudker1

dibs!

eromero1’s picture

Assigned:sbudker1» eromero1
StatusFileSize
new68.07 KB

dibs

sbudker1’s picture

Only local images are allowed.

eromero1’s picture

I tried to reroll the patch, but I got an error
Warning: Missing argument 3 for template_preprocess(), called in /drupal/core/includes/theme.inc on line 1130 and defined in template_preprocess() (line 2474 of core/includes/theme.inc).

Cottser’s picture

@eromero1 - good catch, I ran across that myself last night. That is an unrelated issue, here is the issue for that bug:
#2030457: Warning: Missing argument 3 for template_preprocess() for theme functions overridden by templates

So if you rerolled and that was the only error you got, please post your reroll! It's almost always easier to work with a newly rerolled patch even if it's not perfect.

Carolyn’s picture

Assigned:eromero1» Carolyn
StatusFileSize
new11.29 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch seven-1987424-29.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Reroll of #5. This needs work so that the arrows show up on table headers.

Carolyn’s picture

Assigned:Carolyn» Unassigned
joelpittet’s picture

Issue tags:+Needs reroll

Carolyn could you do another re-roll it looks like all the $vars to $variables got reverted in in #29

joelpittet’s picture

+++ b/core/themes/seven/templates/admin-block-content.html.twig
@@ -0,0 +1,26 @@
+ * @see template_preprocess()

+++ b/core/themes/seven/templates/custom-block-add-list.html.twig
@@ -0,0 +1,30 @@
+ * @see template_preprocess()

+++ b/core/themes/seven/templates/node-add-list.html.twig
@@ -0,0 +1,26 @@
+ * @see template_preprocess()

These can be removed too.

drupalninja99’s picture

Status:Needs work» Needs review
StatusFileSize
new179.79 KB
PASSED: [[SimpleTest]]: [MySQL] 56,644 pass(es).
[ View ]

-Rerolled patch
-Changed $vars to $variables
-Removed references to @see template_preprocess()

drupalninja99’s picture

Status:Needs review» Needs work

Hold on, patch is wrong, let me take another crack at it.

drupalninja99’s picture

StatusFileSize
new10.1 KB
PASSED: [[SimpleTest]]: [MySQL] 57,432 pass(es).
[ View ]

-Fixed the re-roll.

drupalninja99’s picture

Status:Needs work» Needs review
Les Lim’s picture

Issue tags:-Needs reroll

removed tag.

Les Lim’s picture

EDIT: blergh, issue queue fail.

LewisNyman’s picture

#35: seven-1987424-35.patch queued for re-testing.

LewisNyman’s picture

The latest patch is also trying to add ie.css back in the theme. It also removes the CSS from the install page.

Cottser’s picture

Status:Needs review» Needs work
jenlampton’s picture

Status:Needs work» Needs review
StatusFileSize
new5.88 KB
PASSED: [[SimpleTest]]: [MySQL] 57,894 pass(es).
[ View ]

trying again.

siccababes’s picture

If test bot turns green, then I think this can be considered RTBC. After applying the patch, I made seven the default theme. I played around a little and everything looked fine.

LewisNyman’s picture

Status:Needs review» Needs work
+++ b/core/themes/seven/seven.themeundefined
@@ -145,9 +102,6 @@ function seven_tablesort_indicator($variables) {
function seven_preprocess_install_page(&$variables) {
   drupal_add_js(drupal_get_path('theme', 'seven') . '/js/mobile.install.js');
-  drupal_add_css(drupal_get_path('theme', 'seven') . '/install-page.css', array('group' => CSS_AGGREGATE_THEME));
-  $variables['styles'] = new RenderWrapper('drupal_get_css');
-  $variables['scripts'] = new RenderWrapper('drupal_get_js');
}

This patch still removes css from preprocess_install_page, apart from that we're good to go.

LewisNyman’s picture

Issue summary:View changes

Revise summary

Cottser’s picture

Issue tags:+Needs reroll

This also needs a reroll.

oshelach’s picture

Assigned:Unassigned» oshelach
Gaelan’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new5.87 KB
PASSED: [[SimpleTest]]: [MySQL] 58,431 pass(es).
[ View ]

Rerolled!

Cottser’s picture

Status:Needs review» Needs work

Thanks @Gaelan. @oshelach had just assigned it yesterday, but can still work on #44. Seems to me like we shouldn't be changing seven_preprocess_install_page() at all in this patch.

oshelach’s picture

Status:Needs work» Needs review
StatusFileSize
new648 bytes
new5.42 KB
PASSED: [[SimpleTest]]: [MySQL] 58,556 pass(es).
[ View ]
Cottser’s picture

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

Thanks @oshelach :) This needs a reroll again. A couple of these might be dependent on other issues getting in, #1987406: node.module - Convert theme_ functions to Twig for node-add-list and #1987410: [meta] system.module - Convert theme_ functions to Twig for admin-block-content.

+++ b/core/themes/seven/seven.theme
@@ -39,104 +39,62 @@ function seven_preprocess_page(&$variables) {
+ * Implements hook_preprocess_HOOK() for node-add-list.html.twig.
...
+ * Implements hook_preprocess_HOOK() for custom-block-add-list.html.twig.
...
+ * Implements hook_preprocess_HOOK() for theme_admin_block_content().
...
+ * Implements hook_preprocess_HOOK() for tablesort-indicator.html.twig.

Based on the work being done on #2049207: [Follow up] Replace .tpl.php with .html.twig in documentation, these should probably be formatted like "Implements hook_preprocess_HOOK for ____ templates."

oshelach’s picture

Status:Needs work» Needs review
StatusFileSize
new1001 bytes
new5.4 KB
FAILED: [[SimpleTest]]: [MySQL] 58,813 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
oshelach’s picture

Um.... lemme just fix those function description comments.

oshelach’s picture

Status:Needs review» Needs work
oshelach’s picture

StatusFileSize
new2.63 KB
new0 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,496 pass(es).
[ View ]

Spotted a couple additional documentation comments to modify, I hope this isn't off the mark.

oshelach’s picture

Status:Needs work» Needs review
oshelach’s picture

StatusFileSize
new2.63 KB
new6.64 KB
FAILED: [[SimpleTest]]: [MySQL] 58,699 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Oops. Here's the actual patch.

Status:Needs review» Needs work

The last submitted patch, seven-twig-1987424-54.patch, failed testing.

oshelach’s picture

StatusFileSize
new729 bytes
new6.64 KB
FAILED: [[SimpleTest]]: [MySQL] 58,498 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

Sorry, white space.

oshelach’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, seven-twig-1987424-58.patch, failed testing.

oshelach’s picture

Status:Needs work» Needs review
StatusFileSize
new741 bytes
new6.74 KB
PASSED: [[SimpleTest]]: [MySQL] 58,514 pass(es).
[ View ]
rteijeiro’s picture

Status:Needs review» Reviewed & tested by the community

Code seems ok and patch applies well. Let go for RTBC?

Cottser’s picture

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

Thanks @oshelach! I don't think this is quite ready yet…

  1. +++ b/core/themes/seven/seven.theme
    @@ -31,11 +31,11 @@ function seven_library_info() {
    + * Implements hook_preprocess_HOOK() for maintenance-page templates.

    @@ -62,104 +62,62 @@ function seven_preprocess_page(&$variables) {
    + * Implements hook_preprocess_HOOK() for node-add-list templates.
    ...
    + * Implements hook_preprocess_HOOK() for custom-block-add-list templates.
    ...
    + * Implements hook_preprocess_HOOK() for theme_admin_block_content templates.
    ...
    + * Implements hook_preprocess_HOOK() for tablesort-indicator templates.

    No hyphens or underscores in these descriptions please. You should be able to copy descriptions from the existing ones in core or see the latest patch in #2049207: [Follow up] Replace .tpl.php with .html.twig in documentation for more examples.

  2. +++ b/core/themes/seven/seven.theme
    @@ -43,14 +43,14 @@ function seven_preprocess_maintenance_page(&$variables) {
    + * Implements hook_preprocess_HOOK() for html templates.

    This is good, just capitalize HTML please.

  3. +++ b/core/themes/seven/seven.theme
    @@ -62,104 +62,62 @@ function seven_preprocess_page(&$variables) {
    -function seven_admin_block_content($variables) {
    ...
    +function seven_preprocess_admin_block_content(&$variables) {

    This is premature I think since it's still a theme function - might need to be moved to this issue: #1987410: [meta] system.module - Convert theme_ functions to Twig.

    Unless it works fine like this too :)

joelpittet’s picture

+++ b/core/themes/seven/seven.theme
@@ -62,104 +62,62 @@ function seven_preprocess_page(&$variables) {
-    $output .= '</ul>';
   }
-  else {
-    $output = '<p>' . t('You have not created any content types yet. Go to the <a href="@create-content">content type creation page</a> to add a new content type.', array('@create-content' => url('admin/structure/types/add'))) . '</p>';
-  }
-  return $output;

This looks like someone may have forgot to convert the 'You have not created any content types yet. '... bit from node_add_list.

Can someone manually test this?

rteijeiro’s picture

@joelpittet: The message is still there. It seems to be in the template that makes sense and it's better ;)

joelpittet’s picture

StatusFileSize
new51.79 KB

@rteijeiro so the steps to reproduce:
Remove content types
Go to /node/add

Screenshot of what it should say after the content types are removed. This needs test with the patch:)
Add_page___Site-Install.png

rteijeiro’s picture

Status:Needs work» Needs review
StatusFileSize
new7.93 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Yes, you are right! Just confused the page.

Re-created the patch after discussing with you ;)

Added core/themes/seven/templates/node-add-list.html.twig template missed from #2 patch.

rteijeiro’s picture

StatusFileSize
new7.4 KB
PASSED: [[SimpleTest]]: [MySQL] 58,678 pass(es).
[ View ]

Forget Cottser comments in #63.

This is the good patch and I hope final one :P

joelpittet’s picture

Status:Needs review» Needs work

It looks like the twig templates got lost in #42 So we'll need to make sure those get added back in from #35

Thanks @rteijeiro for adding in the node-add-list.html.twig

These two need to be added as well.
+++ b/core/themes/seven/templates/admin-block-content.html.twig
+++ b/core/themes/seven/templates/custom-block-add-list.html.twig

rteijeiro’s picture

Assigned:oshelach» rteijeiro

Ok, I will try to fix this issue today ;)

rteijeiro’s picture

Status:Needs work» Needs review
StatusFileSize
new2.27 KB
new9.67 KB
FAILED: [[SimpleTest]]: [MySQL] 58,567 pass(es), 2 fail(s), and 2 exception(s).
[ View ]

Sorry for the delay. Here is the patch with the two templates from comment #35.

Status:Needs review» Needs work

The last submitted patch, seven-layout-styles-2017257-71.patch, failed testing.

joelpittet’s picture

Thanks for the patch and adding those twig files back in @rteijeiro. One of the exceptions/fails is related to no {{ compact }} variable passed by preprocess, check #35 again in the preprocess and compare to see where those got lost.

rteijeiro’s picture

Status:Needs work» Needs review
StatusFileSize
new1.04 KB
new10.21 KB
FAILED: [[SimpleTest]]: [MySQL] 59,249 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Let's see if now passes the test. It seems that seven_preprocess_admin_block_content function was not created :(

Status:Needs review» Needs work
Issue tags:-Needs manual testing, -Twig

The last submitted patch, 1987424-74.patch, failed testing.

joelpittet’s picture

Status:Needs work» Needs review

#74: 1987424-74.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Needs manual testing, +Twig

The last submitted patch, 1987424-74.patch, failed testing.

joelpittet’s picture

Status:Needs work» Needs review
+++ b/core/themes/seven/templates/node-add-list.html.twig
@@ -0,0 +1,25 @@
++{% if content %}

Doubl ++ on this node-add-list.html.twig

Most likely why it's failing.

Also I think this is missing an tablesort-indicator.html.twig. It may not be needed but the width/height of the image may need changing from the system version? Just need a markup check on that one.

joelpittet’s picture

Status:Needs review» Needs work

Whoops. Needs work, see #78

rteijeiro’s picture

Status:Needs work» Needs review
StatusFileSize
new9.23 KB
FAILED: [[SimpleTest]]: [MySQL] 58,678 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Re-rolled!

Status:Needs review» Needs work

The last submitted patch, 1987424-80.patch, failed testing.

rteijeiro’s picture

It seems that for seven theme the test is using:

$this->assertLinkByHref(url('block/add/foo', $options));

But for bartik and startk themes is using:

$this->clickLink('foo');

I will try to fix it tomorrow. Today my simpletest module is not working and I don't know why :(

rteijeiro’s picture

Status:Needs work» Needs review
StatusFileSize
new8.95 KB
FAILED: [[SimpleTest]]: [MySQL] 58,870 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

New re-roll.

robmc’s picture

Status:Needs review» Needs work
StatusFileSize
new8.95 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Re-rolled

rteijeiro’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1987424-83_0.patch, failed testing.

rteijeiro’s picture

Status:Needs work» Needs review
StatusFileSize
new903 bytes
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Patch for the failing test. It seems to be something weird in $options of $this->assertLinkByHref(url('block/add/foo', $options));

Status:Needs review» Needs work

The last submitted patch, custom-block-type-test.patch, failed testing.

rteijeiro’s picture

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

Sorry for the crappy patch in #87. It seems I uploaded the interdiff :(

It seems that there is no theme parameter for block/add so that's because I removed $options in $this->assertLinkByHref(url('block/add/foo', $options));

Hope now patch it's right.

joelpittet’s picture

Status:Needs review» Needs work

Some nitpicks and regression stuff after another review.
@rteijeiro thanks for the new patch.

  1. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockTypeTest.php
    @@ -171,7 +171,7 @@ public function testsCustomBlockAddTypes() {
    -          $this->assertLinkByHref(url('block/add/foo', $options));
    +          $this->assertLinkByHref(url('block/add/foo'));

    I think we need to keep that... but I could be wrong. It looks like it makes sure the block is added to the right theme.

  2. +++ b/core/themes/seven/seven.theme
    @@ -43,104 +43,62 @@ function seven_preprocess_page(&$variables) {
    +      $variables['types'][$type->type]['label'] = check_plain($type->name);
    +      $variables['types'][$type->type]['description'] = filter_xss_admin($type->description);
    ...
    +      $variables['types'][$id]['label'] = check_plain($type->label());
    +      $variables['types'][$id]['description'] = filter_xss_admin($type->description);
    ...
    +      $variables['content'][$key]['label'] = filter_xss_admin($item['title']);
    ...
    +      $variables['content'][$key]['description'] = filter_xss_admin($item['description']);

    check_plain and filter_xss_admin have been replaced with String::checkPlain() and Xss::filterAdmin

  3. +++ b/core/themes/seven/seven.theme
    @@ -43,104 +43,62 @@ function seven_preprocess_page(&$variables) {
    -      $output .= \Drupal::l($content, 'custom_block.add_form', array('custom_block_type' => $id), $options);
    ...
    +      $variables['types'][$id]['path'] = url('block/add/' . $type->id);

    This this needs be the \Drupal::l() above for the route to stay in tact.

  4. +++ b/core/themes/seven/templates/node-add-list.html.twig
    @@ -0,0 +1,25 @@
    +  <p>{{ 'You have not created any content types yet. Go to the <a href="@create-content">content type creation page</a> to add a new content type.'|t({'@create-content': url('admin/structure/types/add')}) }}</p>

    This would be nicer as a trans block.

rteijeiro’s picture

Status:Needs work» Needs review
StatusFileSize
new4.56 KB
new9.73 KB
FAILED: [[SimpleTest]]: [MySQL] 58,921 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

It seems that tests fail now in a different place. Trying to figure out how to fix it.

Status:Needs review» Needs work

The last submitted patch, 1987424-91.patch, failed testing.

joelpittet’s picture

Assigned:rteijeiro» Unassigned
Status:Needs work» Needs review
StatusFileSize
new9.8 KB
PASSED: [[SimpleTest]]: [MySQL] 58,055 pass(es).
[ View ]
new2.13 KB

Sorry @rteijeiro that custom_block_add_list is confusing because we changed it from using $variables['types'] to using ['content'] which is what types is originally built from. This had all sorts of things done in it. So I moved the query params up to seven's url.

The trans block was a bit trickier but the purpose is you don't need a giant string any longer, the trick is that only simple variables can be used inside so I had to set the url above to use it inside.

joelpittet’s picture

Issue summary:View changes

Update conversion table

skt’s picture

I got part-way through comparing before & after....

For function node_add_list, no output markup change.

Before:
node-add before patch

After:
node_add after patch

For function custom_block_add_list, no output markup change.

Before:
custom_block_add_list before patch

After:
custom_block_add_list after patch

For admin_block_content, only change was an added space in the ul tag:
<ul class="admin-list ">

Before:
admin_block_content-before

After:
admin_block_content-after

skt’s picture

Issue summary:View changes

Add steps to test

Cottser’s picture

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

That's awesome, thanks a million @skt!

+++ b/core/themes/seven/seven.theme
@@ -43,104 +45,63 @@ function seven_preprocess_page(&$variables) {
+    $variables['compact'] = system_admin_compact_mode() ? 'compact' : '';

+++ b/core/themes/seven/templates/admin-block-content.html.twig
@@ -0,0 +1,25 @@
+  <ul class="admin-list {{ compact }}">

I think there are a few ways to solve the minor markup discrepancy.

1. Change 'compact' to ' compact' and remove the space between admin-list and {{ compact }}.

2. Build out the attributes array/object in preprocess and then do <ul{{ attributes }}>.

Looking at what is happening over in #1987410: [meta] system.module - Convert theme_ functions to Twig (code snippet from that issue below) and in general I think we should do 2 and also add a @todo to remove the code in the Seven preprocess once #1987410: [meta] system.module - Convert theme_ functions to Twig is resolved.

+++ b/core/modules/system/system.admin.inc
@@ -333,29 +294,29 @@ function theme_admin_block($variables) {
+    $compact = system_admin_compact_mode();
+    $variables['attributes'] = array('class' => array('admin-list'));
+    if ($compact) {
+      $variables['attributes']['class'][] = 'compact';
     }
Cottser’s picture

Issue summary:View changes

Added steps to test info

joelpittet’s picture

Status:Needs work» Needs review
Cottser’s picture

Issue summary:View changes
Status:Needs review» Needs work
Issue tags:+Needs reroll

Tagging for reroll.

longwave’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new9.8 KB
PASSED: [[SimpleTest]]: [MySQL] 59,112 pass(es).
[ View ]

Rerolled.

joelpittet’s picture

@longwave nice work on the re-roll, any chance you want to grab the items from #95?

longwave’s picture

StatusFileSize
new10 KB
PASSED: [[SimpleTest]]: [MySQL] 59,017 pass(es).
[ View ]
new2.27 KB

Followed the suggestion in #95, removed a blank comment line, and fixed a function name reference.

template_preprocess_admin_block_content() and template_preprocess_node_add_list() don't actually exist yet, though they will after #1987410: [meta] system.module - Convert theme_ functions to Twig and #1987406: node.module - Convert theme_ functions to Twig get in.

joelpittet’s picture

diff --git a/core/modules/system/templates/image.html.twig b/core/modules/system/templates/image.html.twig
new file mode 100644
index 0000000..b6b238a
--- /dev/null
+++ b/core/modules/system/templates/image.html.twig
@@ -0,0 +1,14 @@
+{#
+/**
+ * @file
+ * Default theme implementation of an image.
+ *
+ * Available variables:
+ * - attributes: HTML attributes for the img tag.
+ *
+ * @see template_preprocess_image()
+ *
+ * @ingroup themeable
+ */
+#}
+<img{{ attributes }} />

I think this image template got into this patch by accident. It doesn't look to be part of the seven theme.

longwave’s picture

StatusFileSize
new9.53 KB
PASSED: [[SimpleTest]]: [MySQL] 59,242 pass(es).
[ View ]

Rerolled without image.html.twig.

joelpittet’s picture

+++ b/core/themes/seven/seven.theme
@@ -57,104 +59,69 @@ function seven_preprocess_page(&$variables) {
+function seven_preprocess_tablesort_indicator(&$variables) {
+  $variables['uri'] = drupal_get_path('theme', 'seven') . '/images/arrow-' . ($variables['style'] == 'asc' ? 'asc' : 'desc') . '.png';
+  $variables['attributes']['src'] = file_create_url($variables['uri']);
}

I found that the preprocess_tablesort_indicator is not working:( when I was doing manual testing. The image doesn't change to the seven image. All 3 other templates work perfectly and output matches.

joelpittet’s picture

StatusFileSize
new9.54 KB
PASSED: [[SimpleTest]]: [MySQL] 59,212 pass(es).
[ View ]
new1.68 KB

Ok here is a fix to #103 and #102. Removed the image.html.twig that got in there by accident and fixed the tablesort indicator output.

joelpittet’s picture

Here's 4 theme's markup comparisons:
Node Add List
custom-block-add-list
admin-block-content
seven_preprocess_tablesort_indicator

Cottser’s picture

Status:Needs review» Needs work

Thanks for all the work here @joelpittet!

We can rework this and make the tablesort override lean and mean now that #2152261: Clean up for tablesort-indicator.html.twig is in!

joelpittet’s picture

Status:Needs work» Needs review
StatusFileSize
new10.03 KB
PASSED: [[SimpleTest]]: [MySQL] 59,377 pass(es).
[ View ]
new2.04 KB

This of course won't work... but it should. There should be some globally accessible variables in the twig context and maybe a few more functions like 'drupal_get_path' or something of it's kin.

joelpittet’s picture

FYI, re #108 even though It passed... theme_path is not a real variable... yet.

Cottser’s picture

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

Tagging for reroll.

InternetDevels’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new10.27 KB
PASSED: [[SimpleTest]]: [MySQL] 63,568 pass(es).
[ View ]

Here is the reroll. I'm not sure about preprocess I've added, maybe someone may suggest better solution.

joelpittet’s picture

@InternetDevels what did you add, could you provide an interdiff for the changes you made from #108?

Cottser’s picture

Issue tags:+Twig conversion
InternetDevels’s picture

StatusFileSize
new1.77 KB

It's a bit difficult to create interdiff between an applicable and a non-applicable patches, but I hope that attached file may help you to see what changes I've added.

joelpittet’s picture

@interdevels sorry, wasn't clear there. Don't worry about interdiffs on rerolls. Just for your changes. It helps see clearly what you changed from the previous patch. I know it's tricky if your changes are right after a reroll though if you are using branches in it helps to commit and post the reroll first then commit and post the patch + interdiff. I could have miss read your post when you said your not sure about stuff you added. So if that was part of the conflict resolution I apologize but maybe you can point out that section of code?

Cottser’s picture

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

Tagging for reroll.

IshaDakota’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new10.48 KB
PASSED: [[SimpleTest]]: [MySQL] 64,363 pass(es).
[ View ]

rerolled.

joelpittet’s picture

+++ b/core/themes/seven/seven.theme
@@ -73,6 +74,7 @@ function seven_preprocess_page(&$variables) {
+<<<<<<< HEAD

@@ -150,103 +152,70 @@ function seven_menu_local_task($variables) {
+=======
...
+>>>>>>> applying old patch

Some merge markers got into the re-roll. Could you give it another crack?

longwave’s picture

StatusFileSize
new10.27 KB
PASSED: [[SimpleTest]]: [MySQL] 64,420 pass(es).
[ View ]
new725 bytes
Cottser’s picture

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

Tagging for reroll. Thanks everyone for keeping this one up to date!

Sutharsan’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new10.29 KB
PASSED: [[SimpleTest]]: [MySQL] 64,785 pass(es).
[ View ]

Patch #119 rerolled.

joelpittet’s picture

Thanks for the re-roll here are some notes we should take care of to avoid any confusion or pushback.

  1. +++ b/core/themes/seven/seven.theme
    @@ -112,104 +113,68 @@ function seven_menu_local_task($variables) {
    +      $variables['types'][$type->type]['path'] = url('node/add/' . $type->type);
    ...
    +      $variables['types'][$type->id()]['path'] = \Drupal::url('custom_block.add_form', array('custom_block_type' => $type->id()), $options);
    ...
    +      $variables['content'][$key]['path'] = url($item['link_path']);

    'url' or 'href' may be a better key to not confuse people who have a conceptual concept of 'path'

  2. +++ b/core/themes/seven/templates/node-add-list.html.twig
    @@ -0,0 +1,28 @@
    +  {% set add_content_type_url = url('admin/structure/types/add') %}

    I'd rather do this in the preprocess and as a route because we'll very likely change these paths all to routes and depreciate/change the url() function in twig.

  3. +++ b/core/themes/seven/templates/tablesort-indicator.html.twig
    @@ -0,0 +1,17 @@
    +  <img src="{{ url(theme_path ~ '/images/arrow-asc.png') }}" width="9" height="5" alt="{{ 'Sort ascending'|t }}" title="{{ 'Sort ascending'|t }}" />
    ...
    +  <img src="{{ url(theme_path ~ '/images/arrow-desc.png') }}" width="9" height="5" alt="{{ 'Sort descending'|t }}" title="{{ 'Sort descending'|t }}" />

    I know I put this url/theme_path stuff in here but can we move that back to preprocess because again we may be changing url() to routes and hopefully coming up with a nicer way to get a global theme_path variable or function into twig. @see [2168231]

Cottser’s picture

Status:Needs review» Needs work
Related issues:+#2168231: Functions/variables needed in Twig templates

Adding the issue referenced in #3 above as a related issue.

JeroenT’s picture

Status:Needs work» Needs review
StatusFileSize
new10.36 KB
PASSED: [[SimpleTest]]: [MySQL] 64,763 pass(es).
[ View ]
new5.02 KB

Made changes as suggested by joelpittet.

Patch attached.

joelpittet’s picture

Couple more things, hope this goes green.

+++ b/core/themes/seven/seven.theme
@@ -120,8 +120,10 @@
+    $variables['add_content_type_url'] = url('admin/structure/types/add');

So we don't get in trouble by the routing police, the url() call can become:
\Drupal::url('node.type_add');

+++ b/core/themes/seven/seven.theme
@@ -112,104 +113,71 @@ function seven_menu_local_task($variables) {
+  $variables['arrow_asc'] = url(drupal_get_path('theme','seven') . '/images/arrow-asc.png');
+  $variables['arrow_desc'] = url(drupal_get_path('theme','seven') . '/images/arrow-desc.png');

Have been told that url() will likely break things for multilingual by prefixing the file path with /fr/ or something. So we should be using file_create_url() here.

JeroenT’s picture

StatusFileSize
new10.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,717 pass(es).
[ View ]
new885 bytes

I made the routing police happy again.

joelpittet’s picture

Thanks for the routing policing fix:)

+++ b/core/themes/seven/seven.theme
@@ -112,104 +113,71 @@ function seven_menu_local_task($variables) {
+  $variables['arrow_asc'] = file_create_url('/images/arrow-asc.png');
+  $variables['arrow_desc'] = file_create_url('/images/arrow-desc.png');

Any chance you can confirm this actually works? I'd assume you need to still use the drupal_get_path('theme', 'seven') prefix in there, but I could be wrong.

longwave’s picture

StatusFileSize
new10.34 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,710 pass(es).
[ View ]
new713 bytes

The drupal_get_path() call is still needed; without it, the tablesort indicators do not display properly.

Status:Needs review» Needs work

The last submitted patch, 128: 1987424-twig-seven-theme-128.patch, failed testing.

joelpittet’s picture

Status:Needs work» Needs review

lol,

Failed to run tests: @reason.
joelpittet’s picture

willieseabrook’s picture

Issue tags:+TUNIS_2014_MARCH
Cottser’s picture

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

Tagging for reroll.

mark.labrecque’s picture

StatusFileSize
new10.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
mark.labrecque’s picture

Assigned:Unassigned» mark.labrecque
mark.labrecque’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 134: 1987424-twig-seven-theme-134.patch, failed testing.

mark.labrecque’s picture

Assigned:mark.labrecque» Unassigned
Issue tags:-Needs reroll

Re-rolled, but this fails at the Drupal installation test. Need to do a trace to determine the issue here.

joelpittet’s picture

Thanks for the re-roll @mark.labrecque, here are a couple notes though in general the re-roll looks great. I think it's the duplicate use statement.

  1. +++ b/core/themes/seven/seven.theme
    @@ -8,6 +8,7 @@
    use Drupal\Component\Utility\Xss;
    ...
    +use Drupal\Component\Utility\Xss;

    This is now a duplicate, likely due to the patch getting in that did the remainder of these. Remove one 'use' statement.

  2. +++ b/core/themes/seven/seven.theme
    @@ -113,104 +114,72 @@ function seven_menu_local_task($variables) {
    +  } else {

    Just noticed this else is not to code standards. The word else should start on the next line as it did before.

mark.labrecque’s picture

Thanks for spotting that @joelpittet. I will try that.

mark.labrecque’s picture

Assigned:Unassigned» mark.labrecque
mark.labrecque’s picture

StatusFileSize
new10.14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,161 pass(es).
[ View ]

Let's try this...

mark.labrecque’s picture

Status:Needs work» Needs review
joelpittet’s picture

Awesome could you post the interdiff as well? Docs here https://drupal.org/documentation/git/interdiff
Or the bottom of this little reference. http://pittet.ca/drupal/sprint/patch

joelpittet’s picture

Status:Needs review» Needs work
+++ b/core/themes/seven/seven.theme
@@ -113,104 +113,72 @@ function seven_menu_local_task($variables) {
+  } ¶

Somehow a an extra space got added to the end of this line. You can make your editor strip whitespace from the end of lines.
If you are using sublime add these two lines to your settings:

"trim_trailing_white_space_on_save": true,
"ensure_newline_at_eof_on_save": true,

JeroenT’s picture

StatusFileSize
new10.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,771 pass(es).
[ View ]
new19 bytes

Made changes as suggested by joelpittet.

Patch attached.

JeroenT’s picture

Status:Needs work» Needs review

Marking as needs review.

JeroenT’s picture

StatusFileSize
new699 bytes

And this is the right interdiff.

LewisNyman’s picture

Issue tags:+frontend
a-fro’s picture

StatusFileSize
new3.51 KB
+++ b/core/themes/seven/seven.theme
@@ -113,104 +113,72 @@ function seven_menu_local_task($variables) {
+      $variables['types'][$type->type]['url'] = url('node/add/' . $type->type);

Sorry to bring up the routing police again, but shouldn't this be changed to the route name?

\Drupal::url('node.add', array('node_type' => $type->type));

Otherwise, the code looks good. I'm also attaching a before/after diff on the themed output, which looked fine to me.

a-fro’s picture

StatusFileSize
new10.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,755 pass(es).
[ View ]
new876 bytes

I went ahead and made the change so we can try and get this through at DrupalCon.

a-fro’s picture

Status:Needs review» Needs work
a-fro’s picture

Status:Needs work» Needs review
a-fro’s picture

Issue summary:View changes
a-fro’s picture

StatusFileSize
new2.68 KB

I ran a before/after diff on the output again after applying the last patch. I'm attaching the results.

What you'll see is that the only difference (other than the twig debug output) is that the tablesort indicator is now missing

typeof="foaf:Image"

@joelpittet suggests that we note it and ask @scor if that is an issue.

Also, we noted that there is a difference between how system.module and seven are rendering the tablesort indicator.

The implementation of the twig template at core/modules/system/templates/tablesort-indicator.html.twig might benefit from the approach taken here in seven.

mark.labrecque’s picture

Assigned:mark.labrecque» Unassigned
scor’s picture

Status:Needs review» Needs work

Don't worry about typeof="foaf:Image", it's printed as a side effect of the way we render images. It's perfectly fine if it is no longer there, since those images are just from the presentation layer (not the actual data being displayed).

+++ b/core/themes/seven/seven.theme
@@ -113,104 +113,72 @@ function seven_menu_local_task($variables) {
+ * Implements hook_preprocess_HOOK() for custom block add list templates.
+ *
+ * Add variables for the label and the path separately.

Any reason why this one is not at the third person? ("Adds ...")

a-fro’s picture

StatusFileSize
new866 bytes
new16.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,749 pass(es).
[ View ]

After talking with @cottser about @scor's documentation feedback, we decided it was clearer to remove the line altogether and update the description. Attaching an interdiff and the new patch.

Edited: ugh, forgot the commas. Patch coming momentarily.

a-fro’s picture

StatusFileSize
new16.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,753 pass(es).
[ View ]
new868 bytes

Trying again, with oxford commas ;) Think this is rtbc.

okami’s picture

Status:Needs work» Needs review
Issue tags:-TUNIS_2014_MARCH
a-fro’s picture

StatusFileSize
new868 bytes
new10.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,751 pass(es).
[ View ]

Extra files removed. Sorry about that @cottser!

joelpittet’s picture

Issue summary:View changes
Status:Needs review» Reviewed & tested by the community

@a-fro Thanks for the doc fixes and manual testing.

This is RTBC. No profiling needed as they are all admin ui facing.

Cottser’s picture

Status:Reviewed & tested by the community» Needs work

Kicking this back because the docs are not up to par and there is a @todo that can be addressed. Thanks @a-fro and everyone who has worked on this one so far :) we are close!

  1. +++ b/core/themes/seven/seven.theme
    @@ -113,104 +113,70 @@ function seven_menu_local_task($variables) {
    +    // @todo Remove once theme_admin_block() is converted to Twig.
    +    // See https://drupal.org/node/1987410

    Hang on, theme_admin_block() has been converted to twig. Let's see what can be done with this.

  2. +++ b/core/themes/seven/seven.theme
    @@ -326,7 +292,7 @@ function seven_form_node_form_alter(&$form, &$form_state) {
    -  $form['meta'] = array (
    +  $form['meta'] = array(

    Completely out of scope, please remove this hunk from the patch :) thanks!

  3. +++ b/core/themes/seven/templates/admin-block-content.html.twig
    @@ -0,0 +1,25 @@
    + *   - path: Path to the admin section.
    ...
    +      <li><a href="{{ item.url }}"><span class="label">{{ item.label }}</span><div class="description">{{ item.description }}</div></a></li>

    'docblock' refers to path, template uses 'url'.

  4. +++ b/core/themes/seven/templates/admin-block-content.html.twig
    @@ -0,0 +1,25 @@
    + *
    + * @ingroup themeable

    +++ b/core/themes/seven/templates/custom-block-add-list.html.twig
    @@ -0,0 +1,29 @@
    + *
    + * @ingroup themeable

    +++ b/core/themes/seven/templates/node-add-list.html.twig
    @@ -0,0 +1,27 @@
    + *
    + * @ingroup themeable

    +++ b/core/themes/seven/templates/tablesort-indicator.html.twig
    @@ -0,0 +1,17 @@
    + *
    + * @ingroup themeable

    Remove these @ingroup themeable per #2226185: Remove @ingroup themeable from core theme Twig template docblocks please :)

  5. +++ b/core/themes/seven/templates/custom-block-add-list.html.twig
    @@ -0,0 +1,29 @@
    + * - types: A collection of all the available custom block types.
    + *   Each type contains:
    + *   - type: The custom block type, containing all the items below.
    + *   - link: A link to add a block of this type.
    + *   - description: A description of this custom block type.
    + *   - label: The title of the custom block type.
    + *   - path: A path for the link to add a block of this type.

    It looks like from the template and preprocess that only url, label, and description variables are actually available inside each block type.

  6. +++ b/core/themes/seven/templates/node-add-list.html.twig
    @@ -0,0 +1,27 @@
    + * - types: List of content types. Each content type contains:
    + *   - add_link: Link to create a piece of content of this type.
    + *   - description: Description of this type of content.

    Each item in types contains url, label, and description (as per the preprocess docs). This should be updated.

  7. +++ b/core/themes/seven/templates/node-add-list.html.twig
    @@ -0,0 +1,27 @@
    +    <p>You have not created any content types yet. Go to the <a href="{{ add_content_type_url }}">content type creation page</a> to add a new content type.</p>

    The add_content_type_url variable should be documented.

JeroenT’s picture

Assigned:Unassigned» JeroenT
JeroenT’s picture

Status:Needs work» Needs review

1.

function seven_preprocess_admin_block_content(&$variables) {
  if (!empty($variables['content'])) {
    foreach ($variables['content'] as $key => $item) {
      $variables['content'][$key]['url'] = url($item['link_path']);
    }
  }
}

I was unable to delete this preprocess function completely because the url is in link_path and the twig file expects it to be in url.

2. Fixed
3. Fixed
4. Fixed
5. Fixed
6. Fixed
7. Fixed

JeroenT’s picture

StatusFileSize
new9.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,850 pass(es).
[ View ]
new3.98 KB

Forgot patch..

JeroenT’s picture

Assigned:JeroenT» Unassigned
Cottser’s picture

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

Thanks @JeroenT, the changes look great! This needs a reroll now though.

lokapujya’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new9.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,498 pass(es).
[ View ]

Reroll.

joelpittet’s picture

Status:Needs review» Reviewed & tested by the community

Awesome thanks @lokapujya and @Cottser, this is back to RTBC as per #168

Cottser’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new9.76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,125 pass(es).
[ View ]
new2.7 KB
new2.12 KB
new1.05 KB
new4.82 KB

Updating the patch with a few changes here, I didn't say RTBC in 168 just that the changes looked good :)

I made 3 sets of changes, with an interdiff for each (or I made one interdiff with all changes if you prefer). 2 and 3 came from another round of manual testing where I found a couple templates were pretty broken.

  1. I found some more docs to add/fix up/move around.
  2. custom_block.module is now block_content.module.
  3. admin-block-content.html.twig was broken, label is now title.
lokapujya’s picture

do we need to set the title variable in the preprocess function?

Cottser’s picture

Title is passed in by the calling code, for example:

<?php
 
public function viewModeTypeSelection() {
   
$entity_types = array();
    foreach (
$this->entityManager()->getDefinitions() as $entity_type_id => $entity_type) {
      if (
$entity_type->isFieldable() && $entity_type->hasViewBuilderClass()) {
       
$entity_types[$entity_type_id] = array(
         
'title' => $entity_type->getLabel(),
         
'link_path' => 'admin/structure/display-modes/view/add/' . $entity_type_id,
         
'localized_options' => array(),
        );
      }
    }
    return array(
     
'#theme' => 'admin_block_content',
     
'#content' => $entity_types,
    );
  }
?>
lokapujya’s picture

Issue summary:View changes

admin/structure/block/custom-blocks/types => admin/structure/block/block-content

lokapujya’s picture

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

@Cottser Ah sorry, I did a deductive RTBC. RTBC in #162 +- good on doc changes in #163- #168 + reroll = Back to RTBC.

Thanks for changing Path to URL, that is a bit clearer as path has special meaning to drupal.

hass’s picture

Status:Reviewed & tested by the community» Needs work

There is a translatable string, but it cannot translated. Aside from this, isn't there no way of using a theme_image in twig?

Edit: i was not aware of trans, but the <p> need to be moved outside the translatable string.

joelpittet’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new9.78 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,932 pass(es).
[ View ]
new882 bytes

@hass, as far as I know, you can't build renderable arrays in twig(maybe you can with {{ {'#theme': 'image', '#uri': 'path/to/file.jpg', ...} }}, though I've not tried). At the moment I see no need to do this though. If that is a need it be opened up into a followup issue, please? There are a number of twig filter/function related issues, though we really need to meta those up together...

Thanks you for the catch of the p tag on the inside. The trans tag in twig is an easier way to write translatable strings with long or multi-line copy.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed bec1967 and pushed to 8.x. Thanks!

  • alexpott committed bec1967 on 8.x
    Issue #1987424 by rteijeiro, oshelach, a-fro, JeroenT, joelpittet,...
pwolanin’s picture

We might need to revert part of this - hard-coding the A tag contents for the system admin block is the wrong approach. We should have blocked that part on: #2073811: Add a url generator twig extension

Status:Fixed» Closed (fixed)

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