Add Twig template inheritance based on the theme registry, enable adding Twig loaders

Here's an initial patch, needs tests and some more/better docs still. This copies findTemplate() from the vendor Twig_Loader_Filesystem (with coding standards in line with ours) and adds our logic to the end before the final throw:

    // Allow for loading based on the theme registry.
    $hook = str_replace('.html.twig', '', strtr($name, '-', '_'));
    $theme_registry = \Drupal::service('theme.registry')->getRuntime();

    if ($theme_registry->has($hook)) {
      $info = $theme_registry->get($hook);
      print_r($info);
      if (isset($info['path'])) {
        $path = $info['path'] . '/' . $name;
      }
      elseif (isset($info['template'])) {
        $path = $info['template'] . '.html.twig';
      }
      if (isset($path) && is_file($path)) {
        return $this->cache[$name] = $path;
      }
    }

Rerolled for #2251113: Use container parameters instead of settings and removed the print_r() :)

Working on tests.

Thanks @dawehner! I thought DI would make sense for #1 but I didn't know how to do it immediately. I like your suggestion about a helper method, I will see what I can do.

Tests are coming along well, will post those soon then look at #4.

Better docs and initial tests, based heavily on the Twig namespace tests. May need a bit more tweaking so they make more sense but it's a start.

Here's a shot at #4. Hope this is roughly what you meant @dawehner, if not let me know :)

I forgot to mention, in #6 I removed the namespace from both block--system-branding-block.html.twig templates to show how this would work.

You can pass multiple paths as an array although we only pass one as a string. I copied that from Twig_Loader_Filesystem::__construct() although I guess the if could be removed in our case.

After looking at #2317557: renderInline not compatible with twig_auto_reload again I'm curious if we could just add a smaller filesystem loader class with only the registry logic rather than extending the whole Twig_Loader_Filesystem class. I have no idea if this is possible, if it's possible it seems like it should be done on the service level so it can be swapped out. Maybe we can specify multiple classes to pass to the constructor of TwigEnvironment?

Part of me thinks the throw in Twig_Loader_Filesystem will prevent this from happening and yet the string loader works even though it's second in the chain…

Indeed after some playing around I came up with something that seems to work. Twig_Loader_Chain catches the Twig_Error_Loader exceptions, very clever!

@dawehner I think I'll need your thoughts on this one! It was a lot of fun playing around in core.services.yml :) I left a commented line in there, I thought it might also be worth exploring/discussing the idea of Twig loaders as a tagged service.

Just thinking out loud, but maybe we could register Twig_Loader_Filesystem and pass to the Twig environment as we do now, and then tag the theme registry and string loaders as twig.loader. That would potentially allow adding the namespace paths to Twig_Loader_Filesystem in Drupal\Core\Template\TwigEnvironment::__construct() and also adding the other 2 loaders (and potentially more from contrib).

Simplification and small tweaks for the registry loader.

@joelpittet same idea as Twig extensions are now, in your MODULE.services.yml you would register your loader service and tag it with twig.loader to have it added to the loader chain.

More info:
http://symfony.com/doc/current/cookbook/templating/twig_extension.html#r...
Bottom of https://www.drupal.org/node/1831138

I do know you can specify a "weight" when you tag something. It seems like it would be possible to replace the entire loader chain as well if you wanted similar to how you can swap out other services.

Thanks @Jeff Burnz! I want to push this forward in some way so I will try to play around with the idea of twig loaders being a tagged service, I don't want to keep changing that part of core.services.yml.

Here's a reroll to start with, no changes.

This seems to work, let's see if anything fails. Of course this would need some more tests added to make sure other loaders can be added.

Feedback please!

And woohoo this is not a new idea! :D Kinda looks like this code would be almost what we want too, because we want at least one and we want the chain loader to be the default:

https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/TwigBu...

So I think the next step will be to add this as a compiler pass using code very similar to the Symfony TwigBundle rather than the standard service_collector mechanism (\Drupal\Core\DependencyInjection\Compiler\TaggedHandlersPass).

Something like this. Re-tagging for tests and updating the issue summary a bit.

Here's a rough skeleton for some tests, I also did some poking around the web to get a feel for the space.

@joelpittet recently clued me into the string loader being discouraged and newer versions of Twig even say "This loader should only be used for unit testing as it has many limitations (for instance, the include or extends tag does not make any sense for a string loader).".

So I'd hesitate to make it the job of this issue but we should probably give some thought to how we're using the string loader.

This GitHub issue suggests using the array loader or template_from_string. I don't see how the latter would be practical for inline templates but the array loader might be a possibility:

https://github.com/symfony/symfony/issues/10865

Based on that thread (I haven't played with it yet) at least one of the problems is that when a template is not found and you are using the string loader in the Twig loader chain, instead of a useful error/exception you will just see the name of the template you called on screen.

Issue created to figure out the string loader: #2369981: Not found templates are displayed literally instead of throwing an Exception due to string loader

This should turn green.

Just remembered @Fabianx said it shouldn't be a hardcoded 0 index for the attributes.

Here's what I came up with at BADCamp, but I feel like I am probably missing something.

I do like the $tag_attributes var name better though.

Here's a reroll.

Minor: Coding standards update and variable renaming. I think the next step will be to write two change records and add the beta evaluation criteria to the issue summary.

Two change record drafts created, edits welcome:

https://www.drupal.org/node/2381097
https://www.drupal.org/node/2381103

Initial beta evaluation added to the issue summary, it's my first one so I probably didn't do it right.

For example, it helps if you uncomment the table rows :D

Fix for #2376791: Move all _content routing definitions to _controller.

@lauriii, yeah I copy/pasted that. Fixed here!

I also decided it was about time to use hyphens instead of underscores in the template names, which also allows removing the 'template' lines too :)

+++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
@@ -57,7 +57,9 @@ public function __construct($root, \Twig_LoaderInterface $loader = NULL, ModuleH
-        $loader->addPath($templatesDirectory, $name);
+        // @todo Inject this? Might be weird injecting two TwigLoaderInterface?
+        //   Or solve this another way.
+        \Drupal::service('twig.loader.filesystem')->addPath($templatesDirectory, $name);

I also want to call out the above @todo, I don't know the best way to handle that, maybe @Fabianx or someone else has an idea that I can implement :)

Edit: Didn't mean to attach 2 interdiffs but the first one shows the twig/Twig change, the second one shows that and the template updates.

Subclassed the filesystem loader, this allows us to simplify the TwigEnvironment a bit. I also moved the registry loader and new subclassed filesystem loader into a Loader namespace.

Thanks @Fabianx @alexpott for the reviews!

1 - Sure.

2 - I don't know how to do that… asking in IRC now.

3 - Initially I brought over the compiler pass from the TwigBundle because of the below, but if we are saying this doesn't matter then I can reimplement it as a TaggedHandlersPass fairly easily, attached to the twig.loader.chain service I suppose.

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TwigLoaderPass.php
@@ -0,0 +1,62 @@
+    if (count($loaderIds) === 0) {
+      throw new LogicException('No twig loaders found. You need to tag at least one loader with "twig.loader"');
+    }
...
+    if (count($loaderIds) === 1) {
+      $container->setAlias('twig.loader', key($loaderIds));
+    }

These will probably never be true but I like having them there. Theoretically you could replace the whole chain with your own.

4 - Sounds good, I can create the follow-up. I think we can just remove the file_exists() check entirely, it seems unnecessary.

#57.1:

I think in the case of #2358037: Add search form block Twig template file we need to leave the classy namespace there, otherwise it would be referencing itself. I'm not sure if it's possible to protect from those, in other words I'm not sure if the loader class can be aware of the context from which it's called.

core/themes/bartik/templates/block--search-form-block.html.twig:
{% extends "@classy/block--search-form-block.html.twig" %}

The template for block--search-form-block.html.twig in the registry would be the one in Bartik.

However I did convert all the remaining unnecessary namespace usage in core I could find (in text.module).

2 - done and so far nothing seems to have broken :)

3 - left as is, reasoning is in #58 and #59.

4 - I re-did the namespacing, here is the follow-up to remove the file_exists although it may need more than just a straight removal: #2383413: Remove file_exists() when registering namespaces for Twig template paths

Created this issue as a spin-off from what I found when playing around here: #2387069: {% extends "foo.html.twig" %} in Twig templates does not respect theme inheritance

@alexpott I'm not sure exactly what you mean by "the singleton", this might be easier to hash out on IRC though. Below is my guess:

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TwigLoaderPass.php
@@ -0,0 +1,62 @@
+    if (count($loaderIds) === 1) {
+      $container->setAlias('twig.loader', key($loaderIds));
+    }

Talked through this on IRC, I will look at adding an optional tag attribute for when you tag services with service_collector to allow indicating that at least one service/handler is required (name suggestions welcome :)). Then we can use the standard TaggedHandlersPass, and we'll always use Twig_Loader_Chain even if there is only one loader.

This way if other services have this use case it's available and no need to make a custom compiler pass to cover it.

Within TaggedHandlersPass this is where an exception can be thrown if the service_collector tag has the relevant tag attribute:

if (empty($handlers)) {
  continue;
}

I'm not ready to unassign but the lack of activity is because I've been ill. Hoping to get back to this soon but if anyone is itching to continue don't let me stop you :)

@hass perhaps if you replace "source" with "solution"? Nothing has been committed here.

I am hoping to (finally) come back to this within the next week. Not ill anymore just busy with client work.

Reroll! One line changed to catch up with current state, see interdiff.

Seeming random fail in Drupal\migrate_drupal\Tests\d6\MigrateFileTest.

Need to add some tests to TaggedHandlersPassTest but this seems to work so far.

• Updated docs in TaggedHandlersPass to document the 'required' attribute.
• A simpler exception message.
• Test coverage!

I think this is ready for more reviews now :)

And here are just the tests since I added the PHPUnit coverage. This should fail.

Yay! I updated most of the related issues.