Commit Message

Issue #1964156 by geoffreyr, Cottser, joelpittet: Fixed Contrib cannot define additional Twig extensions.

Problem/Motivation

Twig functions and filters cannot be extended outside of core. This basic feature of Twig is a necessity, otherwise we will be stuck with whatever is committed to core.

Goal

Allow Twig functions and filters to be extended via services.

Proposed Solution

Add a compiler pass for Twig when the service container is built. The compiler pass will look for any services tagged twig.extension and add them to the service container.
@see patch at #42

Related Issues

Original post by geoffreyr

I've been taking a look around trying to work out how to set up Twig extensions for a particular module, and after looking at TwigEnvironment and TwigExtension classes, have attempted to do it the following way:

  1. Create a class that extends \Twig_Extension
  2. Attach it to the twig service in my module's Bundle::build function, using the following method:
    $container->get('twig')->addMethodCall('addExtension', array(new Definition('Drupal\mymodule\MyModuleTwigFilters')));

However, right now it's dying in the following manner:
PHP Fatal error: Call to undefined function Drupal\\Core\\Template\\cache() in /path/to/drupal8/core/lib/Drupal/Core/Template/TwigEnvironment.php on line 30

Here we have the line $this->cache_object = cache(); which seems to be trying to invoke http://api.drupal.org/api/drupal/core%21includes%21cache.inc/function/ca...
However, it looks as though either core/includes/cache.inc isn't being called at the right time, or the namespacing for this particular class is clobbering the function call.

I don't have a proper stack trace yet, but I might be able to try and sort one out. I might also try and whip up some sample code to reproduce this issue once I get some time. I'm not even certain that this is to be the officially sanctioned way to set up Twig functions/filters/etc, so feel free to correct me on the issue.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Fabianx’s picture

Have you tried replacing that line with:

$cache_object = Drupal::cache('cache');

cache() is deprecated by now anyway ...

Does it work in general if you hack it into CoreBundle temporarily?

geoffreyr’s picture

Thanks for the suggestion.

Tried using your method and it gives me
PHP Fatal error: Class 'Drupal\\Core\\Template\\Drupal' not found in /path/to/drupal/core/lib/Drupal/Core/Template/TwigEnvironment.php on line 30
which suggests that the namespace is clobbering it.

Also tried replacing it with
$this->cache_object = \Drupal::cache('cache');
but hit the following error:
PHP Fatal error: Call to a member function get() on a non-object in /path/to/drupal/core/lib/Drupal.php on line 171
which appears to be
return static::$container->get('cache.' . $bin);

Not sure why this should be failing, I think I might have to do a bit more reading about how this works.

Update: OK, so it's not finding the global static container. This isn't good, that shouldn't happen right? I wonder if the bundles are being somehow enumerated before the container is set?

Fabianx’s picture

Component: theme system » base system

I think this is a base system problem and not related to Twig in itself as this is now coming from the container ...

geoffreyr’s picture

Status: Active » Needs review
FileSize
2.93 KB
2.69 KB

It looks as though since the last time I checked, this has been fixed.
Furthermore, I've got a patch and a module to demo it with.

twig_extensions.patch contains the bits necessary to register Twig extensions during the compiler pass.
bigfrog.zip is a demo module containing a very simple Twig filter to turn all lowercase "frog" in a bit of text into uppercase "FROG". It also contains a page callback to demonstrate it.

Hydra’s picture

Status: Needs review » Needs work

This is a very nice patch and a highly needed feature for contrib to be able to provide twig extentions. Good work, I agree this needs test, you may talk to Cottser about that.

There are a few documentation tweaks, nothing to worry about.

+++ b/core/lib/Drupal/Core/CoreBundle.phpundefined
@@ -60,6 +61,8 @@ public function build(ContainerBuilder $container) {
+    // Register Twig extensions

Every comment has to end with a ./!/?

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterTwigExtensionsPass.phpundefined
@@ -0,0 +1,38 @@
+
+      // We must assume that the class value has been correcly filled, even if the service is created by a factory

No need for this white line. Comment has to end with a point or something. Max length for comments is 80chars, maybe you'll have a look at Codingstandarts for comments http://drupal.org/coding-standards#comment

geoffreyr’s picture

Status: Needs work » Needs review
FileSize
12.94 KB

I've re-rolled the above patch with the changes above, and included a test module & WebTestBase class. The tests are pretty simple, but they should be able to demo that the service is being included, and that filters & functions are working.

Fabianx’s picture

Status: Needs review » Needs work

+1000 for this

This is really great and allows to define Twig Extensions just as in Symphony. The code would be RTBC besides that comments need some little cleanup and whitespace issues and coding standards should be checked again.

So: Almost RTBC!

Freaking awesome patch!

Fabianx’s picture

Priority: Normal » Major

This is at least major, because contrib should be able to define Twig Extensions using services.yml.

geoffreyr’s picture

Status: Needs work » Needs review
FileSize
4.91 KB
12.89 KB

Cleaned up a bit.

geoffreyr’s picture

FileSize
4.91 KB
12.89 KB

This time for sure. Cleaned up whitespace.

chrisroane’s picture

#10: 1964156-v4.patch queued for re-testing.

chrisroane’s picture

Assigned: Unassigned » chrisroane

Manually testing this patch.

chrisroane’s picture

Assigned: chrisroane » Unassigned
Status: Needs review » Reviewed & tested by the community

I created a custom module and manually tested. Looked through the code and this worked great for me!

Hydra’s picture

+1 for RTBC

joelpittet’s picture

Title: Cannot define additional Twig extensions » Contrib cannot define additional Twig extensions
Issue tags: +Needs issue summary update

+1 for RTBC, tagging for someone to help with summary.

star-szr’s picture

Status: Reviewed & tested by the community » Needs work

Fantastic work on this @geoffreyr.

One thing that strikes me as odd: I'm not sure the .tpl.php files are needed here unless I'm missing something.

Documentation needs some work before this is ready.

  1. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigExtensionTest.phpundefined
    @@ -0,0 +1,96 @@
    + * Tests Twig engine configuration via settings.php.

    I think this comment was from TwigSettingsTest.php, should be updated.

  2. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterTwigExtensionsPass.phpundefined
    @@ -0,0 +1,38 @@
    + * Definition of Drupal\Core\DependencyInjection\Compiler\RegisterTwigExtensionsPass.

    Contains \Drupal\Core… per http://drupal.org/node/1353118.

  3. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterTwigExtensionsPass.phpundefined
    @@ -0,0 +1,38 @@
    +class RegisterTwigExtensionsPass implements CompilerPassInterface {
    +  public function process(ContainerBuilder $container) {

    Class and method are missing docblocks per http://drupal.org/coding-standards/docs#classes.

    +++ b/core/modules/system/tests/modules/twig_extension_test/lib/Drupal/twig_extension_test/TwigExtension/TestExtension.phpundefined
    @@ -0,0 +1,45 @@
    +<?php
    +
    +namespace Drupal\twig_extension_test\TwigExtension;
    +
    +use Drupal\Core\Template\TwigExtension;
    +use Drupal\Core\Template\TwigReferenceFunction;
    +
    +class TestExtension extends TwigExtension {

    This file needs an @file docblock and methods are missing docblocks per http://drupal.org/coding-standards/docs#classes.

geoffreyr’s picture

Since Twig has gone in the .tpl.php files won't be needed any more, assuming all themes have been converted. Will reroll the patch with the doc changes noted above and the .tpl.php files removed.

geoffreyr’s picture

FileSize
6.94 KB
13.72 KB

Removed .tpl.php files, removed tests targetting absence of .tpl.php files, and updated documentation.

geoffreyr’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1964156.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.23 KB
13.7 KB

Have a feeling those errors just may have been someone renamed that test module.

This passes locally (see attached)

star-szr’s picture

Assigned: Unassigned » star-szr

More documentation nitpicks, patch coming in a day or two. Leaving at CNR because they are really minor.

star-szr’s picture

FileSize
13.7 KB

Status: Needs review » Needs work

The last submitted patch, 1964156-23.patch, failed testing.

star-szr’s picture

Assigned: star-szr » Unassigned
Status: Needs work » Needs review
FileSize
5.4 KB
5.6 KB
13.82 KB

Summary of changes (no functional changes):

  1. Minor coding standards tweaks.
  2. Updated docs throughout; rewrapping, tweaking and adding @see references.
  3. Moved \Drupal\twig_extension_test\TwigExtension\TestExtension::testFilter() to the bottom to match the order that things are declared earlier in the file.

interdiff-no-move.txt is an easier to read interdiff that only shows changes #1 and #2 from the list above.

Edit: didn't see the test failure, not sure what's up with that.

star-szr’s picture

I can confirm the test failures in #23 locally, but nothing changed between #21 and #23 in this issue. Maybe something has changed elsewhere.

Status: Needs review » Needs work

The last submitted patch, 1964156-24.patch, failed testing.

joelpittet’s picture

@Cottser here's the diff between #21 and #23

--- from_file 
+++ (clipboard) 
@@ -1,19 +1,19 @@
 diff --git a/core/lib/Drupal/Core/CoreBundle.php b/core/lib/Drupal/Core/CoreBundle.php
-index d29da83..19aa7e8 100644
+index 472105a..77b7995 100644
 --- a/core/lib/Drupal/Core/CoreBundle.php
 +++ b/core/lib/Drupal/Core/CoreBundle.php
-@@ -16,6 +16,7 @@
- use Drupal\Core\DependencyInjection\Compiler\RegisterRouteEnhancersPass;
+@@ -17,6 +17,7 @@
  use Drupal\Core\DependencyInjection\Compiler\RegisterParamConvertersPass;
  use Drupal\Core\DependencyInjection\Compiler\RegisterServicesForDestructionPass;
+ use Drupal\Core\DependencyInjection\Compiler\RegisterStringTranslatorsPass;
 +use Drupal\Core\DependencyInjection\Compiler\RegisterTwigExtensionsPass;
  use Symfony\Component\DependencyInjection\ContainerBuilder;
  use Symfony\Component\DependencyInjection\ContainerInterface;
  use Symfony\Component\DependencyInjection\Reference;
-@@ -60,6 +61,8 @@ public function build(ContainerBuilder $container) {
-     // Add the compiler pass that will process the tagged services.
-     $container->addCompilerPass(new RegisterPathProcessorsPass());
+@@ -63,6 +64,8 @@ public function build(ContainerBuilder $container) {
      $container->addCompilerPass(new ListCacheBinsPass());
+     // Add the compiler pass for appending string translators.
+     $container->addCompilerPass(new RegisterStringTranslatorsPass());
 +    // Register Twig extensions.
 +    $container->addCompilerPass(new RegisterTwigExtensionsPass());
    }

No idea if the order matters... so going to re-run #21 test.

joelpittet’s picture

Status: Needs work » Needs review
joelpittet’s picture

Wow that was silly, shouldn't have done that...

star-szr’s picture

Status: Needs review » Needs work

@joelpittet - Sorry, should have been clearer. Yes, the string translation pass was added in #1813762: Introduce unified interfaces, use dependency injection for interface translation. But the code here did not change and tests still fail locally when switching the passes around or removing the string translation pass.

Rolling back HEAD to June 6 and applying #21, the tests do indeed pass. So a git bisect may be in order unless someone knows something more.

Edited to add more information.

c4rl’s picture

Priority: Major » Critical

This seems like a big blocker for contrib modules to extend and leverage Twig. On the Twig call today, we agreed that this is likely a critical bug.

star-szr’s picture

Status: Needs work » Needs review
FileSize
13.87 KB

Reroll.

star-szr’s picture

Assigned: Unassigned » star-szr

Doing some git bisect to figure out where things went wrong.

Status: Needs review » Needs work

The last submitted patch, 1964156-33.patch, failed testing.

star-szr’s picture

Assigned: star-szr » Unassigned
Status: Needs work » Needs review
FileSize
985 bytes
13.87 KB

Tracked down the issue to #1998140: Remove backward compatible ControllerInterface, an easy fix. Also changing the docblock to {@inheritdoc} based on other controllers.

markhalliwell’s picture

referencing: #2002606-2: Allow themes to provide services.yml which would allow themes to extend twig!

star-szr’s picture

FileSize
2.05 KB
14 KB

Returning render arrays instead of calling theme() from the menu callbacks (adds missing variables/render element to twig_extension_test_theme()), and adding a missing @file docblock to twig_extension_test.module.

star-szr’s picture

FileSize
2.36 KB
14 KB

More tweaks :)

star-szr’s picture

FileSize
3.46 KB
13.84 KB

Just unwrapping some lines of code, at this point I can't find anything else to change. This is definitely ready for more reviews!

Edit: Oh, and I removed an unnecessary variable assignment.

star-szr’s picture

Assigned: Unassigned » star-szr
Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterTwigExtensionsPass.phpundefined
@@ -0,0 +1,49 @@
+use InvalidArgumentException;
+use ReflectionClass;
+use Twig_Extension;

These should not be use'd per https://drupal.org/node/1353118.

star-szr’s picture

Assigned: star-szr » Unassigned
Status: Needs work » Needs review
FileSize
1.41 KB
13.77 KB

Here we go.

star-szr’s picture

Issue summary: View changes

Add commit message to give @geoffreyr more credit

markhalliwell’s picture

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Oh, sweet. This looks good to me.

Has extensive test coverage, had several rounds of review, all feedback has been incorporated, useful functionality, issue summary is up-to-date.

Lets get this in => RTBC

YesCT’s picture

Issue tags: +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 436fe57 and pushed to 8.x. Thanks!

Perhaps the Twig change notice needs updating to reflect the functionality this gives us... https://drupal.org/node/1831138

Status: Fixed » Closed (fixed)

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

mgifford’s picture

I got this error today (with a fresh version of the repo) with essentially this error:

Fatal error: Class '__TwigTemplate_d82601a628282988589f4f7fdc2caa31' not found in DRUPAL8/core/lib/Drupal/Core/Template/TwigEnvironment.php on line 107

Which is very similar to the original report.

Fabianx’s picture

This is unrelated. A not found template usually means a syntax error or something like that, please open a new issue.

dagomar’s picture

Is there a new issue available? I just got this on a fresh install, I only surfed to admin/config/development/sync. This install is now corrupted.

PHP Fatal error: Class '__TwigTemplate_f9341dca449da57d0230eefef7e01001' not found in /path/to/drupal/core/lib/Drupal/Core/Template/TwigEnvironment.php on line 107

(edit) Was able to get it to run by adding this in my settings.php:

$settings['twig_cache'] = FALSE;

star-szr’s picture

@dagomar - had you already installed Drupal there before? If so, try sudo rm -rf sites/default/files/php, that will clear out the PHPStorage which includes compiled Twig templates.

dagomar’s picture

Hi Cottser,

thanks, I'll try that next time. Is that like clearing the caches? Because using drush cc all doesn't do this.

discipolo’s picture

got this on a fresh install too.

$settings['twig_cache'] = FALSE; this worked for me,

sudo rm -rf sites/default/files/php,this did not

discipolo’s picture

Issue summary: View changes

Updated issue summary.