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
- #2002606: Allow themes to provide services.yml - Will allow themes to define Twig extensions via services as well.
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:
- Create a class that extends \Twig_Extension
- 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.
Comment | File | Size | Author |
---|---|---|---|
#42 | 1964156-42.patch | 13.77 KB | star-szr |
#42 | interdiff.txt | 1.41 KB | star-szr |
#40 | 1964156-40.patch | 13.84 KB | star-szr |
#40 | interdiff.txt | 3.46 KB | star-szr |
#39 | 1964156-39.patch | 14 KB | star-szr |
Comments
Comment #1
Fabianx CreditAttribution: Fabianx commentedHave you tried replacing that line with:
cache() is deprecated by now anyway ...
Does it work in general if you hack it into CoreBundle temporarily?
Comment #2
geoffreyr CreditAttribution: geoffreyr commentedThanks 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?
Comment #3
Fabianx CreditAttribution: Fabianx commentedI think this is a base system problem and not related to Twig in itself as this is now coming from the container ...
Comment #4
geoffreyr CreditAttribution: geoffreyr commentedIt 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.
Comment #5
Hydra CreditAttribution: Hydra commentedThis 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.
Every comment has to end with a ./!/?
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
Comment #6
geoffreyr CreditAttribution: geoffreyr commentedI'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.
Comment #7
Fabianx CreditAttribution: Fabianx commented+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!
Comment #8
Fabianx CreditAttribution: Fabianx commentedThis is at least major, because contrib should be able to define Twig Extensions using services.yml.
Comment #9
geoffreyr CreditAttribution: geoffreyr commentedCleaned up a bit.
Comment #10
geoffreyr CreditAttribution: geoffreyr commentedThis time for sure. Cleaned up whitespace.
Comment #11
chrisroane CreditAttribution: chrisroane commented#10: 1964156-v4.patch queued for re-testing.
Comment #12
chrisroane CreditAttribution: chrisroane commentedManually testing this patch.
Comment #13
chrisroane CreditAttribution: chrisroane commentedI created a custom module and manually tested. Looked through the code and this worked great for me!
Comment #14
Hydra CreditAttribution: Hydra commented+1 for RTBC
Comment #15
joelpittet+1 for RTBC, tagging for someone to help with summary.
Comment #16
star-szrFantastic 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.
I think this comment was from TwigSettingsTest.php, should be updated.
Contains \Drupal\Core… per http://drupal.org/node/1353118.
Class and method are missing docblocks per http://drupal.org/coding-standards/docs#classes.
This file needs an @file docblock and methods are missing docblocks per http://drupal.org/coding-standards/docs#classes.
Comment #17
geoffreyr CreditAttribution: geoffreyr commentedSince 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.
Comment #18
geoffreyr CreditAttribution: geoffreyr commentedRemoved .tpl.php files, removed tests targetting absence of .tpl.php files, and updated documentation.
Comment #19
geoffreyr CreditAttribution: geoffreyr commentedComment #21
joelpittetHave a feeling those errors just may have been someone renamed that test module.
This passes locally (see attached)
Comment #22
star-szrMore documentation nitpicks, patch coming in a day or two. Leaving at CNR because they are really minor.
Comment #23
star-szrFirst a reroll, some code was close to the changes in #1813762: Introduce unified interfaces, use dependency injection for interface translation.
Comment #25
star-szrSummary of changes (no functional changes):
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.
Comment #26
star-szrI can confirm the test failures in #23 locally, but nothing changed between #21 and #23 in this issue. Maybe something has changed elsewhere.
Comment #28
joelpittet@Cottser here's the diff between #21 and #23
No idea if the order matters... so going to re-run #21 test.
Comment #29
joelpittet#21: 1964156-22-twig-extensions-in-contrib.patch queued for re-testing.
Comment #30
joelpittetWow that was silly, shouldn't have done that...
Comment #31
star-szr@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.
Comment #32
c4rl CreditAttribution: c4rl commentedThis 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.
Comment #33
star-szrReroll.
Comment #34
star-szrDoing some git bisect to figure out where things went wrong.
Comment #36
star-szrTracked down the issue to #1998140: Remove backward compatible ControllerInterface, an easy fix. Also changing the docblock to {@inheritdoc} based on other controllers.
Comment #37
markhalliwellreferencing: #2002606-2: Allow themes to provide services.yml which would allow themes to extend twig!
Comment #38
star-szrReturning 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.
Comment #39
star-szrMore tweaks :)
Comment #40
star-szrJust 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.
Comment #41
star-szrThese should not be use'd per https://drupal.org/node/1353118.
Comment #42
star-szrHere we go.
Comment #42.0
star-szrAdd commit message to give @geoffreyr more credit
Comment #43
markhalliwellUpdated issue summary.
Comment #43.0
markhalliwellUpdated issue summary.
Comment #43.1
markhalliwellUpdated issue summary.
Comment #43.2
markhalliwellUpdated issue summary.
Comment #43.3
markhalliwellUpdated issue summary.
Comment #43.4
markhalliwellUpdated issue summary.
Comment #44
Fabianx CreditAttribution: Fabianx commentedOh, 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
Comment #45
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #46
alexpottCommitted 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
Comment #48
mgiffordI got this error today (with a fresh version of the repo) with essentially this error:
Which is very similar to the original report.
Comment #49
Fabianx CreditAttribution: Fabianx commentedThis is unrelated. A not found template usually means a syntax error or something like that, please open a new issue.
Comment #50
dagomar CreditAttribution: dagomar commentedIs 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.
(edit) Was able to get it to run by adding this in my settings.php:
Comment #51
star-szr@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.Comment #52
dagomar CreditAttribution: dagomar commentedHi Cottser,
thanks, I'll try that next time. Is that like clearing the caches? Because using drush cc all doesn't do this.
Comment #53
discipolo CreditAttribution: discipolo commentedgot this on a fresh install too.
$settings['twig_cache'] = FALSE;
this worked for me,sudo rm -rf sites/default/files/php,
this did notComment #53.0
discipolo CreditAttribution: discipolo commentedUpdated issue summary.