Follow-up to #2568181: [meta] Get ready to upgrade to Twig 2.x

Problem/Motivation

We are currently using some deprecated code that will be removed or not workable in Twig 2.0.

To help move that along we would like to make the TwigExtension more UnitTestable.

Proposed resolution

  • Move twig_without() to the extension

Remaining tasks

User interface changes

n/a

API changes

n/a

Data model changes

n/a

Comments

joelpittet created an issue. See original summary.

joelpittet’s picture

Here is that patch only.

joelpittet’s picture

Issue summary: View changes
FileSize
487.78 KB
Cottser’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -rc target +rc target triage

Yup.

alexpott’s picture

Issue tags: -rc target triage

Discussed with @effulgentsia and we're not sure how this is necessary for Twig 2.0.x - yes it is nice to have more unit testable code but nice to have does not mean it is allowed in RC. We're trying to minimise all change. If we're wrong - re-add the tag and improve the issue summary to explain why it is necessary for RC.

joelpittet’s picture

@alexpott it just forces us to add fake methods at the bottom of the unit tests and 2.0.x changed how the extension was loaded and the unit tests fail, so instead of doing the janky if !function_exists('twig_without')) { at the bottom we clean up the tests at the same time. Also unlikely people are using .engine code, so super minimal change as this all should be considered internal anyway.

Cottser’s picture

Actually the unit test in question *actually* tests this function from what I recall. So we'd have to copy + paste it into the test class or include/require the file.

I don't think this needs to go in RC but it should probably be done along the path to Twig 2.0.x.

Cottser’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.3 KB
1.06 KB

Splitting #2568181: [meta] Get ready to upgrade to Twig 2.x and these changes make the most sense here. Also have some other BC ideas but want to get this up.

Status: Needs review » Needs work

The last submitted patch, 8: move_twig_without_to-2591515-8.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
FileSize
3.96 KB
1.39 KB

Just an idea to make it a bit less disruptive. Will look at that failing test.

Cottser’s picture

Not sure if this is the way to go, needs discussion.

Maybe we actually want to implement initRuntime() in our Twig extension. Seems kinda odd that the extension would be responsible for loading the theme engine though.

The last submitted patch, 10: move_twig_without_to-2591515-9.patch, failed testing.

Cottser’s picture

joelpittet’s picture

Thanks for trying to make this less disruptive @Cottser!

  1. +++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
    @@ -27,6 +27,8 @@ class TwigEnvironment extends \Twig_Environment {
    +  protected $templateClassPrefix = '__TwigTemplate_';
    +
    

    This looks like it snuck into the patch.

  2. +++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
    @@ -43,10 +45,6 @@ class TwigEnvironment extends \Twig_Environment {
       public function __construct($root, CacheBackendInterface $cache, $twig_extension_hash, \Twig_LoaderInterface $loader = NULL, $options = array()) {
    -    // Ensure that twig.engine is loaded, given that it is needed to render a
    -    // template because functions like TwigExtension::escapeFilter() are called.
    -    require_once $root . '/core/themes/engines/twig/twig.engine';
    

    Love that this gets removed!

  3. +++ b/core/modules/system/src/Tests/Theme/TwigWhiteListTest.php
    @@ -123,6 +123,10 @@ public function testWhiteListChaining() {
    +    // Include the Twig engine so we can call twig_render_template().
    +    require_once \Drupal::root() . '/core/themes/engines/twig/twig.engine';
    

    Strange that this test needed to add the engine, was this for that function? If so it's a test and maybe we can make it use the method.

joelpittet’s picture

Status: Needs review » Needs work
Cottser’s picture

#14.1 was intentional, that change is needed for Twig 2.x and wasn't sure where to put it when splitting #2568181: [meta] Get ready to upgrade to Twig 2.x.

Regarding #14.3 what method?

joelpittet’s picture

@Cottser Yeah that doesn't read right #14.3

twig_render_template() maybe go with the rest of the fire too?

Cottser’s picture

I think that would have to be part of an engine refactor as plugins or classes of some kind. It's a "hook" of sorts for theme engines, nyan cat should have one similar.

xjm’s picture

Issue tags: +rc target triage
Cottser’s picture

Status: Needs work » Needs review

I think the feedback from #14 was addressed without changing any code, if I'm wrong please let me know :)

Status: Needs review » Needs work

The last submitted patch, 11: move_twig_without_to-2591515-11.patch, failed testing.

The last submitted patch, 10: move_twig_without_to-2591515-9.patch, failed testing.

The last submitted patch, 8: move_twig_without_to-2591515-8.patch, failed testing.

The last submitted patch, 2: move_twig_without_to-2591515-2.patch, failed testing.

joelpittet’s picture

  1. +++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
    @@ -27,6 +27,8 @@ class TwigEnvironment extends \Twig_Environment {
    +  protected $templateClassPrefix = '__TwigTemplate_';
    +
    

    If this didn't sneak in and was intentional, it at the very least needs a docblock saying what it's for.

  2. +++ b/core/modules/system/src/Tests/Theme/TwigWhiteListTest.php
    @@ -123,6 +123,10 @@ public function testWhiteListChaining() {
    +    // Include the Twig engine so we can call twig_render_template().
    +    require_once \Drupal::root() . '/core/themes/engines/twig/twig.engine';
    

    Really wish we didn't need to add this.

  3. +++ b/core/themes/engines/twig/twig.engine
    @@ -133,20 +133,13 @@ function twig_render_template($template_file, array $variables) {
     function twig_without($element) {
    

    Really doubt anybody would use this function in *.engine, would still rather remove it than leave the BC laying about.

    If there is anybody is using this I am willing to console them in their loss.

alexpott’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Postponed

Discussed with @effulgentsia and @xjm we couldn't see why this is 100% necessary for Twig 2.0 and why the change is necessary for RC. I can see that the consistency of having everything together in the TwigExtension is nice - but it only looks like a nice-to-have to me.

Cottser’s picture

At least as it stands now, our test suite will fail with Twig 2.x.x without these changes. Sorry a bit short on time now otherwise I'd like to an example patch showing those fails.

xjm’s picture

Title: Move twig_without() to the TwigExtension where all the other filters are. » Copt twig_without() to the TwigExtension where all the other filters are, and deprecate

@Cottser, that's okay I think. #26 doesn't quite reflect my take on it, which is that these changes are safe to make in a minor, and therefore can be made in 8.1.x or whenever as we are introducing Twig 2.x.

Cottser’s picture

Yep, that's totally fine with me :) thanks!

joelpittet’s picture

Title: Copt twig_without() to the TwigExtension where all the other filters are, and deprecate » Move twig_without() to the TwigExtension where all the other filters are and deprecate
joelpittet’s picture

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Issue tags: -rc target triage
joelpittet’s picture

Status: Postponed » Needs work

Unpostponing so we can tidy the feedback up in #25 and keep on trucking

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.