Add an active_theme twig function

Thanks for getting this going @cilefen!

Should we inject the theme manager?

I think the tests could be novice.

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

+++ 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?

Rerolling #18.

Created reroll.

Patch attached.

+++ 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.

Added those newlines.

Added too many spaces

Rerolled the patch.

We are working on this at DrupalCon Los Angeles

Added trailing comma, removed extra space.

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

Reviewing the patch as well, also at DC LA

Reviewing the patch as well, also at DC LA

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

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.

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

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

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.

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!

I've removed the link generator.

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" :)

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

@@ -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.

@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?

@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

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.

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.

#55 looks right, compared to #47.

+++ 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.

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

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

Looks good!

Committed ffe9803 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

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

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!