Problem/Motivation

$current_theme from D7 has no equivalent in Drupal 8.

Templates knowing what theme is active is useful when paired with the functionality we see with modules such as Theme Key (https://www.drupal.org/project/themekey), where complex theme switching rules are used. There are also use cases like the front end theme loading a piece of content using the admin theme and vice versa. In these cases markup changes may be desired, but the template needs context to understand how it is being presented.

Proposed resolution

Add an active_theme twig function.

Remaining tasks

User interface changes

API changes

The new twig function, active_theme.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because we are adding a function to twig that is missing as compared to D7.
Issue priority Major. The parent is Major because this and some related functions that existed in D7 are missing from the theme system in D8. Lack of this particular functionality prevents D7 modules which use it from being ported to D8.
Disruption This should produce little disruption, except to patches on similar issues that are adding twig functions.
Files: 
CommentFileSizeAuthor
#65 add_an_active_theme-2416831-65.patch4.33 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,268 pass(es). View
#65 interdiff.txt2.78 KBcilefen
#61 add_an_active_theme-2416831-61.patch5.04 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,255 pass(es). View
#61 interdiff.txt2.86 KBcilefen
#60 add_an_active_theme-2416831-60.patch5.09 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,262 pass(es). View
#60 interdiff.txt5.23 KBcilefen
#55 241631-55.patch7.22 KBNoe_
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,265 pass(es). View
#47 interdiff.txt1.04 KBlauriii
#47 add_an_active_theme-2416831-47.patch7.2 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch add_an_active_theme-2416831-47.patch. Unable to apply patch. See the log in the details link for more information. View
#45 add_an_active_theme-2416831-44.patch7.2 KBvasi
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,426 pass(es). View
#45 interdiff-41-44.txt1.47 KBvasi
#41 add_an_active_theme-2416831-41.patch7.66 KBvasi
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,402 pass(es). View
#41 interdiff-34-41.txt2.86 KBvasi
#34 interdiff-28-34.txt1.15 KBvasi
#34 add_an_active_theme-2416831-34.patch7.36 KBvasi
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,042 pass(es). View
#28 2416831-add-an-active-theme-28.patch7.5 KBNoe_
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,782 pass(es). View
#25 add_an_active_theme-2416831-25.patch7.23 KBtadityar
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,829 pass(es). View
#24 interdiff-22-24.txt1010 bytestadityar
#24 add_an_active_theme-2416831-24.patch7.27 KBtadityar
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,824 pass(es). View
#22 add_an_active_theme-2416831-22.patch7.29 KBJeroenT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,394 pass(es). View
#20 2416831-add_active_theme-20.patch7.29 KBakalata
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,490 pass(es), 522 fail(s), and 294 exception(s). View
#18 add_an_active_theme-2416831-18.patch6.94 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,128 pass(es). View
#18 interdiff-14-18.txt3 KBcilefen
#14 add_an_active_theme-2416831-14.patch7.27 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,127 pass(es). View
#14 interdiff-12-14.txt578 bytescilefen
#12 add_an_active_theme-2416831-12.patch7.18 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,134 pass(es). View
#12 interdiff-11-12.txt2.92 KBcilefen
#11 add_an_active_theme-2416831-11.patch4.05 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,873 pass(es). View
#11 interdiff-9-11.txt624 bytescilefen
#9 add_an_active_theme-2416831-9.patch4.05 KBcilefen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#9 interdiff-2-9.txt3.18 KBcilefen
#2 add_an_active_theme-2416831-2.patch1.09 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,975 pass(es). View

Comments

cilefen’s picture

Title: Create an active_theme twig function (similar current_theme() in D7) » Add an active_theme twig function (similar to $current_theme in D7)
cilefen’s picture

Status: Active » Needs review
FileSize
1.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,975 pass(es). View
davidhernandez’s picture

Issue tags: +Twig
cilefen’s picture

Issue summary: View changes
cilefen’s picture

Title: Add an active_theme twig function (similar to $current_theme in D7) » Add an active_theme twig function
Cottser’s picture

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

Thanks for getting this going @cilefen!

Should we inject the theme manager?

I think the tests could be novice.

cilefen’s picture

@Cottser: Do you why there is both a setGenerators() and setLinkGenrator()? It seems that now we will inject three services, all of them should be in the same method.

Or we could extend ContainerAware and set the entire container as it is likely we will need more services in the future.

Cottser’s picture

Yeah, I noticed that @cilefen. Might be worth doing a git blame to try and find out why they were injected that way.

cilefen’s picture

Status: Needs work » Needs review
FileSize
3.18 KB
4.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

@Cottser Having looked over the issues that inserted these methods, the redundancy was simply overlooked in review.

Status: Needs review » Needs work

The last submitted patch, 9: add_an_active_theme-2416831-9.patch, failed testing.

cilefen’s picture

Status: Needs work » Needs review
FileSize
624 bytes
4.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,873 pass(es). View
cilefen’s picture

Issue tags: -Needs tests, -Novice
FileSize
2.92 KB
7.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,134 pass(es). View

Tests: the way you find out the patch doesn't work. Ha.

cilefen’s picture

+++ b/core/modules/system/src/Tests/Theme/EngineTwigTest.php
@@ -111,4 +111,12 @@ public function testTwigFileUrls() {
+    $this->assertRaw('<div>active theme: classy</div>');

The theme name for the assert should be found from the theme manager service.

cilefen’s picture

FileSize
578 bytes
7.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,127 pass(es). View
tim.plunkett’s picture

+++ b/core/core.services.yml
@@ -1219,8 +1219,7 @@ services:
-      - [setGenerators, ['@url_generator']]
-      - [setLinkGenerator, ['@link_generator']]
+      - [setDependencies, ['@url_generator', '@link_generator', '@theme.manager']]

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -40,26 +41,28 @@ class TwigExtension extends \Twig_Extension {
-  public function setLinkGenerator(LinkGeneratorInterface $link_generator) {
+  public function setDependencies(UrlGeneratorInterface $url_generator, LinkGeneratorInterface $link_generator, ThemeManagerInterface $theme_manager) {
+    $this->urlGenerator = $url_generator;
     $this->linkGenerator = $link_generator;
+    $this->themeManager = $theme_manager;
     return $this;

This is odd to me. Everywhere else we have a single method per service. Why the change?

cilefen’s picture

It looked like a mistake to me. There is a method called setGenerators() that has a function comment making it look like a constructor. It sets only one generator. I don't care either way but I will change its name when I put this back to three methods.

cilefen’s picture

FileSize
5.62 KB
0 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

@tim.plunkett I forgot to thank you for reviewing.

cilefen’s picture

FileSize
3 KB
6.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,128 pass(es). View

Oops

The last submitted patch, 17: add_an_active_theme-2416831-17.patch, failed testing.

akalata’s picture

FileSize
7.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,490 pass(es), 522 fail(s), and 294 exception(s). View

Rerolling #18.

Status: Needs review » Needs work

The last submitted patch, 20: 2416831-add_active_theme-20.patch, failed testing.

JeroenT’s picture

Status: Needs work » Needs review
FileSize
7.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,394 pass(es). View

Created reroll.

Patch attached.

Cottser’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/tests/modules/twig_theme_test/templates/twig_theme_test.active_theme.html.twig
@@ -0,0 +1 @@
+<div>active theme: {{ active_theme() }}</div>
\ No newline at end of file

+++ b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.routing.yml
@@ -55,3 +55,10 @@ twig_theme_test_registry_loader:
+    _access: 'TRUE'
\ No newline at end of file

These need newlines at the end of the file per https://www.drupal.org/coding-standards#indenting.

tadityar’s picture

Status: Needs work » Needs review
FileSize
7.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,824 pass(es). View
1010 bytes

Added those newlines.

tadityar’s picture

FileSize
7.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,829 pass(es). View

Added too many spaces

cilefen’s picture

cilefen’s picture

Issue summary: View changes
Issue tags: -Needs beta evaluation
Noe_’s picture

Issue tags: -Needs reroll
FileSize
7.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,782 pass(es). View

Rerolled the patch.

cilefen’s picture

Status: Needs review » Needs work

@Noe_ That is nice work for a first reroll! Normally, you should avoid changing the order of statements in a reroll. In this case, it doesn't matter much. So, I have some tiny requests below. Please include an interdiff with the next patch.

  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -77,7 +112,8 @@ public function getFunctions() {
    +      new \Twig_SimpleFunction('active_theme', array($this, 'activeTheme'))
    

    There should be a comma after the last element of a multiline array.

  2. +++ b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module
    @@ -54,6 +54,10 @@ function twig_theme_test_theme($existing, $type, $theme, $path) {
    @@ -93,5 +97,6 @@ function _test_theme_twig_php_values() {
    
    @@ -93,5 +97,6 @@ function _test_theme_twig_php_values() {
           'value' => 'Hello world!',
           'expected' => 'Hello world!',
         ),
    +
       );
    

    An extra space crept in here. I think it was in a prior patch.

cilefen’s picture

Issue tags: +Novice

The next steps are accessible to a novice.

vasi’s picture

fengtan’s picture

Assigned: Unassigned » fengtan

We are working on this at DrupalCon Los Angeles

HakS’s picture

vasi’s picture

Status: Needs work » Needs review
FileSize
7.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,042 pass(es). View
1.15 KB

Added trailing comma, removed extra space.

m4olivei’s picture

Hey now, at DC LA, we're gonna go ahead and review this guy.

fengtan’s picture

Assigned: fengtan » Unassigned
vgriffin’s picture

Reviewing the patch as well, also at DC LA

vgriffin’s picture

Issue summary: View changes

Reviewing the patch as well, also at DC LA

vgriffin’s picture

Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community

Reviewed with others at DC Los Angeles. Ready for commit.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work

Good work on the patch & review folks! I'm sorry but I Just wanted to point few nitpicks I found:

  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -16,6 +16,8 @@
    +use Drupal\Core\Utility\LinkGeneratorInterface;
    +use Drupal\Core\Theme\ThemeManagerInterface;
    

    These could be in alphabetical order (t before u)

  2. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -77,7 +112,8 @@ public function getFunctions() {
    +      new \Twig_SimpleFunction('attach_library', array($this, 'attachLibrary')),
    +      new \Twig_SimpleFunction('active_theme', array($this, 'activeTheme')),
    
    +++ b/core/modules/system/tests/modules/twig_theme_test/src/TwigThemeTestController.php
    @@ -84,4 +84,11 @@ public function registryLoaderRender() {
    +    return array('#theme' => 'twig_theme_test_active_theme');
    
    +++ b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module
    @@ -54,6 +54,10 @@ function twig_theme_test_theme($existing, $type, $theme, $path) {
    +  $items['twig_theme_test_active_theme'] = array(
    +    'variables' => array(),
    +    'template' => 'twig_theme_test.active_theme',
    +  );
    

    Could we use the new array syntax here.

vasi’s picture

Status: Needs work » Needs review
FileSize
2.86 KB
7.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,402 pass(es). View

Changed to new array syntax, re-ordered namespace imports to be in alphabetical order.

Todd Zebert’s picture

Status: Needs review » Reviewed & tested by the community

I've reviewed the interdiff and the only differences are the use ordering, and the array syntax, as expected.

Cottser’s picture

Status: Reviewed & tested by the community » Needs work

Thanks everyone for working on this!

+++ b/core/core.services.yml
@@ -1312,7 +1312,9 @@ services:
+      - [setLinkGenerator, ['@link_generator']]

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -15,7 +15,9 @@
+use Drupal\Core\Utility\LinkGeneratorInterface;

@@ -58,16 +67,42 @@ public function __construct(RendererInterface $renderer) {
   /**
+   * Sets the link generator.
+   *
+   * @param \Drupal\Core\Utility\LinkGeneratorInterface $link_generator
+   *   The link generator.
+   *
+   * @return $this
+   */
+  public function setLinkGenerator(LinkGeneratorInterface $link_generator) {
+    $this->linkGenerator = $link_generator;
+    return $this;
+  }

I don't think we need the link generator anymore because of #2335661: Outbound path & route processors must specify cacheability metadata.

lauriii’s picture

Just wanted to leave a comment here to tell that you are making good progress here and this seems to be really close to be ready to be commited! Keep up the good work folks!

vasi’s picture

Status: Needs work » Needs review
FileSize
1.47 KB
7.2 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,426 pass(es). View

I've removed the link generator.

Cottser’s picture

Status: Needs review » Needs work

I agree with @lauriii this looks very close! Thanks all.

  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -77,8 +98,9 @@ public function getFunctions() {
    -      new \Twig_SimpleFunction('attach_library', array($this, 'attachLibrary'))
    ...
    +      new \Twig_SimpleFunction('attach_library', array($this, 'attachLibrary')),
    

    If we are changing this line to add the trailing comma I think we should also change the array to short array syntax. But only on this line :) The motivation is that the next line uses short array syntax here, and we are already changing the line.

  2. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -77,8 +98,9 @@ public function getFunctions() {
    +      new \Twig_SimpleFunction('active_theme', [$this, 'activeTheme']),
    
    @@ -239,6 +261,16 @@ public function getLink($text, $url) {
    +  public function activeTheme() {
    

    This method should probably be called getActiveTheme, especially because the docblock starts with "Gets" :)

lauriii’s picture

Status: Needs work » Needs review
FileSize
7.2 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch add_an_active_theme-2416831-47.patch. Unable to apply patch. See the log in the details link for more information. View
1.04 KB
Cottser’s picture

Status: Needs review » Reviewed & tested by the community

This looks great to me and I tested it as well. I can't find any faults. Tests look good. Thanks @all!

Noe_’s picture

Status: Reviewed & tested by the community » Needs work
@@ -77,8 +98,9 @@ public function getFunctions() {
       new \Twig_SimpleFunction('url_from_path', array($this, 'getUrlFromPath'), array('is_safe_callback' => array($this, 'isUrlGenerationSafe'))),
       new \Twig_SimpleFunction('link', array($this, 'getLink')),
       new \Twig_SimpleFunction('file_url', 'file_create_url'),
-      new \Twig_SimpleFunction('attach_library', array($this, 'attachLibrary'))
-    );
+      new \Twig_SimpleFunction('attach_library', [$this, 'attachLibrary']),
+      new \Twig_SimpleFunction('active_theme', [$this, 'getActiveTheme']),
+    ];
   }

This part is inconsistent. I think we should only do new array syntax or old array syntax. But not both intermingled.
So make the first three also new syntax.

lauriii’s picture

Status: Needs work » Needs review

@noe_ thanks for the review! We have agreed at least on the theme system to start using the new syntax. However we don't want to change existing lines to use the new array syntax at once because it would make us write lots of rerolls for patches because they would contain changes for lines that they are actually not changing. We have agreed to be a little inconsistent on this matter since it makes things lots of easier and this is the way this has worked also on other issues. I'm setting this to Needs review, maybe you want to set this back to RTBC if you agree with me?

Noe_’s picture

Status: Needs review » Reviewed & tested by the community

@lauriii If you guys decided on that I am fine with it, and I only said I wanted to change this in the function that was changed. There are lots of places the other array format is used and I didn't specify those.

It was just that I thought it might be a good idea.

just my 0.02c

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

The issue summary is missing a justification of why we need this? It seems strange that a twig template needs to know the current theme.

davidhernandez’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 47: add_an_active_theme-2416831-47.patch, failed testing.

Noe_’s picture

Status: Needs work » Needs review
FileSize
7.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,265 pass(es). View

Weird, the bot failed testing.
I thought that there must be a conflict with the 8.0.x branch so I started rerolling the patch.

But after applying the patch successfully and rebasing it with 8.0.x it did the 3-way merge and it worked like a charm.

So I added the new patch as an attachment to this comment, but I couldn't figure out how to make an interdiff because the rebase succeeded.

The last submitted patch, 47: add_an_active_theme-2416831-47.patch, failed testing.

davidhernandez’s picture

Status: Needs review » Reviewed & tested by the community

#55 looks right, compared to #47.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/core.services.yml
@@ -1322,7 +1322,8 @@ services:
-      - [setGenerators, ['@url_generator']]
+      - [setUrlGenerator, ['@url_generator']]

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -58,16 +66,29 @@ public function __construct(RendererInterface $renderer) {
-  public function setGenerators(UrlGeneratorInterface $url_generator) {
+  public function setUrlGenerator(UrlGeneratorInterface $url_generator) {

+++ b/core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php
@@ -32,7 +32,7 @@ public function testEscaping($template, $expected) {
-    $twig->addExtension((new TwigExtension($renderer))->setGenerators($this->getMock('Drupal\Core\Routing\UrlGeneratorInterface')));
+    $twig->addExtension((new TwigExtension($renderer))->setUrlGenerator($this->getMock('Drupal\Core\Routing\UrlGeneratorInterface')));

This is now unrelated and unnecessary API change.

Also rather than adding the additional web test method this looks eminently unit testable. We could add a test method to \Drupal\Tests\Core\Template\TwigExtensionTest to replace the added web test.

cilefen’s picture

Status: Needs work » Needs review
FileSize
5.23 KB
5.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,262 pass(es). View

The test needs work.

cilefen’s picture

FileSize
2.86 KB
5.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,255 pass(es). View

This is a crude, but correct, test. We could remove some repetition by setting some class properties in a setUp method. And, the mock builder setup chaining could be improved.

cilefen’s picture

If you are new to phpunit tests, you run this test like:

phpunit -c core/ --filter testActiveTheme

cilefen’s picture

Status: Needs review » Needs work

@alexpott via IRC mentioned that we cannot change setGenerators as it is in HEAD. It is preferable to re-add a setThemeManager method to do what is needed.

Cottser’s picture

Thanks @cilefen, cannot in general or cannot in this issue because it's out of scope?

cilefen’s picture

Status: Needs work » Needs review
FileSize
2.78 KB
4.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,268 pass(es). View

@Cottser I think I should have written "ought not in this issue as such", not "cannot".

Cottser’s picture

Yeah that makes a lot more sense, thanks @cilefen. I can file a separate issue for that change.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Looks good!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed ffe9803 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed ffe9803 on 8.0.x
    Issue #2416831 by cilefen, vasi, tadityar, Noe_, lauriii, akalata,...
davidhernandez’s picture

This should have had a change record. I added one.

Cottser’s picture

Actually I'm not seeing anything else in core.services.yml that has a setter injecting multiple services, so renaming setGenerators seems to make more sense given that but I'm no injection pro. Feel free to disagree over here: #2496801: Change setGenerators to setUrlGenerator on TwigExtension :)

Thanks all!

Status: Fixed » Closed (fixed)

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