Problem/Motivation

#1053648: Convert site elements (site name, slogan, site logo) into blocks brought with it the first use of Twig {% extends %} in core. That patch has {% extends "@block/block.html.twig" %} which uses namespaces to extend templates based on Drupal extensions (modules and themes). This functionality is commonplace in the Symfony world and was introduced in #2143557: Add modules and themes as twig namespaces.

While working on #1053648: Convert site elements (site name, slogan, site logo) into blocks @mdrummond brought forward the idea that template extending should be easier. If you initially start out by extending @block/block.html.twig in your theme but then decide to override block.html.twig in your theme, you need to go back and change your extends to @mytheme/block.html.twig.

Proposed resolution

Add a custom Twig loader to core that extends the filesystem loader to allow for extending based on the theme registry. This would allow for this simple syntax:

{% extends "block.html.twig" %}

Which by default will extend core/modules/block/template/block.html.twig, but if a theme overrides block.html.twig, it will extend from the theme's block.html.twig, and so on.

The patch also adds services tagged with twig.loader to the Twig environment, allowing contrib to do custom template loading and this is something that the Symfony TwigBundle does already: https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/TwigBu...

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it improves TX/DX by making things work as people expect.
Issue priority Normal
Unfrozen changes Mostly an additive change and changes the theme system which is not frozen.
Prioritized changes (not sure)
Disruption Backwards compatible, minimal impact

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Add automated tests Instructions Done

User interface changes

n/a

API changes

An addition along the lines of #2143557: Add modules and themes as twig namespaces.

Files: 
CommentFileSizeAuthor
#77 2291449-77-testonly.patch12.35 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,104 pass(es), 6 fail(s), and 0 exception(s). View
#76 interdiff.txt3 KBCottser
#76 2291449-76.patch24.24 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,115 pass(es). View
#75 interdiff.txt5.26 KBCottser
#75 2291449-75.patch22.1 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,013 pass(es). View

Comments

Cottser’s picture

Status: Active » Needs review
FileSize
3.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,634 pass(es). View

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;
      }
    }
Cottser’s picture

Assigned: Unassigned » Cottser
Status: Needs review » Needs work
Cottser’s picture

Status: Needs work » Needs review
FileSize
2.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,154 pass(es). View
547 bytes

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

Working on tests.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Template/TwigThemeRegistryLoader.php
    @@ -0,0 +1,75 @@
    + */
    +class TwigThemeRegistryLoader extends \Twig_Loader_Filesystem {
    +
    ...
    +    $theme_registry = \Drupal::service('theme.registry')->getRuntime();
    

    If you write new code, use proper dependency injection, please.

  2. +++ b/core/lib/Drupal/Core/Template/TwigThemeRegistryLoader.php
    @@ -0,0 +1,75 @@
    +    $name = (string) $name;
    +
    +    // Normalize name.
    +    $name = preg_replace('#/{2,}#', '/', strtr($name, '\\', '/'));
    +
    +    if (isset($this->cache[$name])) {
    +      return $this->cache[$name];
    +    }
    +
    +    $this->validateName($name);
    +
    +    $namespace = self::MAIN_NAMESPACE;
    +    $shortname = $name;
    +    if (isset($name[0]) && '@' == $name[0]) {
    +      if (false === $pos = strpos($name, '/')) {
    +        throw new \Twig_Error_Loader(sprintf('Malformed namespaced template name "%s" (expecting "@namespace/template_name").', $name));
    +      }
    +
    +      $namespace = substr($name, 1, $pos - 1);
    +      $shortname = substr($name, $pos + 1);
    +    }
    +
    +    if (!isset($this->paths[$namespace])) {
    +      throw new \Twig_Error_Loader(sprintf('There are no registered paths for namespace "%s".', $namespace));
    +    }
    +
    +    foreach ($this->paths[$namespace] as $path) {
    +      if (is_file($path . '/' . $shortname)) {
    +        return $this->cache[$name] = $path . '/' . $shortname;
    +      }
    +    }
    +
    

    Given that this full codeblock is all coming from the parent class, it would be nice to extract into a helper method, to mark it clear that this code actually belongs to the parent method, so upstream changes can be applied better.

Cottser’s picture

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.

Cottser’s picture

FileSize
8.08 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,164 pass(es), 6 fail(s), and 1 exception(s). View
12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,169 pass(es). View

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.

Cottser’s picture

Issue tags: -Needs tests
FileSize
13.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,166 pass(es). View
4.28 KB

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

Cottser’s picture

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.

dawehner’s picture

  1. +++ b/core/core.services.yml
    @@ -948,8 +948,8 @@ services:
    +    class: Drupal\Core\Template\TwigThemeRegistryLoader
    +    arguments: ['%app.root%', '@theme.registry']
    

    perfect!

  2. +++ b/core/lib/Drupal/Core/Template/TwigThemeRegistryLoader.php
    @@ -0,0 +1,131 @@
    +  public function __construct($paths = array(), Registry $theme_registry) {
    +    if ($paths) {
    +      $this->setPaths($paths);
    +    }
    

    Wait, shouldn't app.root be a single value? just a string?

The last submitted patch, 6: 2291449-5-testonly.patch, failed testing.

Cottser’s picture

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.

Cottser’s picture

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…

Cottser’s picture

FileSize
13.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,485 pass(es). View
6.26 KB

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.

Cottser’s picture

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

joelpittet’s picture

Issue tags: +sprint

This is looking pretty cool! Not sure what "tagged service" is care to explain?

Cottser’s picture

FileSize
13.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,483 pass(es). View
1.87 KB

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

joelpittet’s picture

Very interesting... could you re-order this chain or replace it too?

Cottser’s picture

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.

Jeff Burnz’s picture

+++ b/core/lib/Drupal/Core/Template/TwigThemeRegistryLoader.php
@@ -0,0 +1,69 @@
+  protected function findTemplate($name) {

This looks like it's going to solve an issue I ran into this morning (see https://www.drupal.org/node/2143557#comment-9259581), where you can't have templates in a sub directory or extend won't work, i.e. themeName/templates/subdir/template.html.twig - I found this out trying to extend block. If so that would be great, sorry I did not have time today to test this patch.

Cottser’s picture

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.

Cottser’s picture

FileSize
13.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,501 pass(es). View

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

Cottser’s picture

FileSize
14.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,521 pass(es). View
2.21 KB

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!

Cottser’s picture

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

Cottser’s picture

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

Cottser’s picture

Title: Add Twig template inheritance based on the theme registry » Add Twig template inheritance based on the theme registry, enable adding Twig loaders
Issue summary: View changes
Issue tags: +Needs tests
FileSize
15.97 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,485 pass(es). View
5.25 KB

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

Cottser’s picture

Status: Needs review » Needs work
Cottser’s picture

Status: Needs work » Needs review
Issue tags: +Needs change record
FileSize
18.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,879 pass(es), 1 fail(s), and 0 exception(s). View
2.57 KB

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

Cottser’s picture

Status: Needs review » Needs work
joelpittet’s picture

I say we use it, it's useful:) The main idea (in my head at least) behind inline templates and string loader is that it's used to "rapid prototype" an idea and flush out the real template later if that idea becomes viable. Instead of going through the rigamarole of defining hook_theme in the module file, creating a template and building a render array, you just build the template in the string and pass in the data it should expect.

Preaching to the choir here a bit, but that's how I see the role which to me fits inline with the unit test purposes that sensio is proposing. In core I'd actually like to discourage using inlineTemplating just as much as we discourage using ['#markup' => '<p>test</p>']

But for getting things done, they are both awesome tools to have!

The last submitted patch, 27: 2291449-27.patch, failed testing.

Cottser’s picture

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.

joelpittet’s picture

Oh yeah, good call. It would be better is maybe the StringLoader is only explicitly added when it's needed like renderInline() and 'inline_template' ilk

Cottser’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
19.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,945 pass(es). View
3.22 KB

This should turn green.

Cottser’s picture

Status: Needs review » Needs work

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

Fabianx’s picture

See symfony docs, but the gist is:

foreach ($loaderIds as $id => $tag_attributes) {
  foreach ($tag_attributes as $attributes ) {
       $priorities[$id] = isset($attributes['priority']) ? $attributes['priority'] : 0;

...

http://symfony.com/doc/current/components/dependency_injection/tags.html...

The trickiest part is the $attributes variable. Because you can use the same tag many times on the same service (e.g. you could theoretically tag the same service 5 times with the acme_mailer.transport tag), $attributes is an array of the tag information for each tag on that service.

Cottser’s picture

Status: Needs work » Needs review
FileSize
19.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2291449-37.patch. Unable to apply patch. See the log in the details link for more information. View
909 bytes

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

Cottser’s picture

I do like the $tag_attributes var name better though.

Status: Needs review » Needs work

The last submitted patch, 37: 2291449-37.patch, failed testing.

joelpittet’s picture

@Cottser me too:) too many things named attribute, indistinguishable in grep:)

Cottser’s picture

FileSize
19.67 KB

Here's a reroll.

Cottser’s picture

Status: Needs work » Needs review
Issue tags: +Needs issue summary update
FileSize
19.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,749 pass(es), 4 fail(s), and 0 exception(s). View
1.25 KB

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.

Cottser’s picture

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.

Cottser’s picture

Issue summary: View changes

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

Status: Needs review » Needs work

The last submitted patch, 42: 2291449-42.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
FileSize
19.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,743 pass(es). View
725 bytes
lauriii’s picture

+++ b/core/modules/system/src/Tests/Theme/TwigRegistryLoaderTest.php
@@ -0,0 +1,70 @@
+   * Checks to see if a value is a twig template.

Just a nitpick s/twig/Twig :)

Jeff Burnz’s picture

{% extends "block.html.twig" %}, epic win!

joelpittet’s picture

Assigned: Cottser » Fabianx

Putting up the bat signal, @Fabianx can you check this over?

Cottser’s picture

FileSize
636 bytes
19.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,778 pass(es). View
8.24 KB

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

Cottser’s picture

Issue summary: View changes
Fabianx’s picture

Assigned: Fabianx » Unassigned

Twig has since 1.1.4 the ability to use chained loaders, can we use that here? And inject the chain-of-responsibility loaders?

The rest looks pretty good already.

Fabianx’s picture

Nvm my comment, we cleared in IRC:

Yes, we should subclass the twig loader filesystem and inject our service instead, which registers the paths on construct.

Cottser’s picture

Assigned: Unassigned » Cottser
Cottser’s picture

Assigned: Cottser » Unassigned
FileSize
23.16 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,766 pass(es). View
6.47 KB

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.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
  1. +++ b/core/lib/Drupal/Core/Template/Loader/Filesystem.php
    @@ -0,0 +1,52 @@
    + */
    +class Filesystem extends \Twig_Loader_Filesystem {
    +
    

    Not a blocker:

    I don't know Core best practices, I like to re-use the last part of the namespace with the class name.

    A file system is very generic, but not saying what that class does.

    A FilesystemLoader while redundant immediate tells what this is.

    The same for a ThemeRegistry vs. a ThemeRegistryLoader ...

  2. +++ b/core/lib/Drupal/Core/Template/Loader/Filesystem.php
    @@ -0,0 +1,52 @@
    +      if (file_exists($templatesDirectory)) {
    +        $this->addPath($templatesDirectory, $name);
    +      }
    

    This is just moved around, so out of scope to change here, but:

    Could we open a major follow-up bug to remove the file_exists.

    We should have no file_exists calls during run-time.

    This information needs at the minimum be at least cached.

    The parent can be '[meta] Resolve known performance regressions in Drupal 8.'

RTBC, except for those two things, pushing to a core committer to review.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. I just committed #2358037: Add search form block Twig template file so we have some more stuff to convert.
  2. +++ b/core/core.services.yml
    @@ -1085,11 +1085,22 @@ services:
    +  twig.loader.chain:
    +    class: Twig_Loader_Chain
    

    I think this should be marked as private.

  3. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TwigLoaderPass.php
    @@ -0,0 +1,62 @@
    +class TwigLoaderPass implements CompilerPassInterface {
    

    Why are we not using the TaggedHandlersPass here? The logic for the singleton here looks unnecessary.

  4. Yep lets do #56.1 and get a followup from #56.2
Cottser’s picture

Assigned: Unassigned » Cottser

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.

Fabianx’s picture

#57.2:

Can we make it private if we put it as an alias for twig.loader?

I assume yes, just making sure.

#57.3. I think the extra twig pass is okay and I like having the extra checks, too.

Cottser’s picture

Status: Needs work » Needs review
FileSize
24.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,007 pass(es). View
4.47 KB

#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

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, looks great and nice follow-up to the CKEditor fragile issue.

Cottser’s picture

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

davidhernandez’s picture

Regarding, #57.1 I think Cottser is right. It is extending a template with the same name, so there is confusion. But, I think we can actually fix that one by extending the Bartik block template, instead of the classy search form block template. This is something I didn't realize when first making it. We can manipulate the attribute in the child template, so the changes needed can probably all be done in the child. Then, we won't need to specify "@classy".

If it works I'll create an issue for it with a fix.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I'm not that convinced by the arguments in #58 and #59. And I still don't get why we special case the singleton.

Cottser’s picture

@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));
+    }
Cottser’s picture

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;
}
Cottser’s picture

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’s picture

This issue sounds like a possible source of the issue I found in #2402181: Optimize toolbar-menu template with Twig template inheritance. Can someone with Twig skil verify, please?

Cottser’s picture

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

Cottser’s picture

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

Cottser’s picture

Status: Needs work » Needs review
FileSize
24.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,994 pass(es). View
705 bytes

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

Status: Needs review » Needs work

The last submitted patch, 71: 2291449-71.patch, failed testing.

Cottser queued 71: 2291449-71.patch for re-testing.

Cottser’s picture

Status: Needs work » Needs review

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

Cottser’s picture

Issue tags: +SprintWeekend2015
FileSize
22.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,013 pass(es). View
5.26 KB

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

Cottser’s picture

FileSize
24.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,115 pass(es). View
3 KB
  • Updated docs in TaggedHandlersPass to document the 'required' attribute.
  • A simpler exception message.
  • Test coverage!

I think this is ready for more reviews now :)

Cottser’s picture

FileSize
12.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,104 pass(es), 6 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 77: 2291449-77-testonly.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
mdrummond’s picture

  1. +++ b/core/modules/system/tests/modules/twig_theme_test/templates/twig-registry-loader-test-extend.html.twig
    @@ -0,0 +1,5 @@
    +This text is in a block.
    

    Should this text be indented?

  2. +++ b/core/modules/system/tests/themes/test_theme_twig_registry_loader/templates/twig-registry-loader-test-extend.html.twig
    @@ -0,0 +1,5 @@
    +This text is in a block.
    

    Should this be indented?

I did a quick pass, looks like progress is being made. Yay! Couple indentation things I wondered about listed above.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

I don't think the indentation matters for the tests.

RTBC - Patch looks good and uses the Service Collector as proposed by Alex.

Not sure if the addition of required => TRUE needs a change record or some documentation somewhere beneath the API function.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I'm very excited about twig template inheritance working with the theme registry. Committed 0f93b42 and pushed to 8.0.x. Thanks!

  • alexpott committed 0f93b42 on 8.0.x
    Issue #2291449 by Cottser: Add Twig template inheritance based on the...
davidhernandez’s picture

Great work, everyone. This is going to be really useful.

Cottser’s picture

Assigned: Cottser » Unassigned

Yay! I updated most of the related issues.

Jeff Burnz’s picture

Oh wow, amazing! Thank-you everyone who worked on this, especially Cottser, outstanding work, I'm very happy indeed :)

joelpittet’s picture

Sa-weet! Thanks @Cottser:)

eatings’s picture

1. Wait -- it wasn't like this before?

2. Awesome, either way!

Jeff Burnz’s picture

I believe we have a follow up issue, I have post it here, this is probably afaict critical #2414255: Subtheme template inheritance working in reverse order

Status: Fixed » Closed (fixed)

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