Make Drupal 8 optionally working with Twig 2, while keeping its dependency on Twig 1

Note to self, this is postponed on the release of Twig 2.x

Looks like the error was whitespace related give it another try @slasher13?

Removed the unnecessary trailing whitespace in the previous patch.

Let's try again.

Is testbot incompatible with composer 1.3? Got these time property changes after update to composer 1.3

rebased & composer self-update --rollback

+ /usr/local/bin/composer install --no-progress --no-suggest
Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Nothing to install or update

What's wrong with composer.lock file? Twig should be updated on PHP7 environments.

test again after https://www.drupal.org/node/2842443 and https://www.drupal.org/node/2840596.

Thanks for tackling this @slasher13! It looks like there are some changes that are not needed in here. Can you tell my why the changes to core/lib/Drupal/Core/Field/BaseFieldDefinition.php were made?

PHP 7 tests seem to get closer to 0

wrong patch

@slasher13 this change is not going to work :(

We can't update the composer.lock whilst still supporting PHP 5.5.9

But I don't know how to test PHP 7 only. After PHP 7 is green I will update to twig 1.31. So we are compatible with twig 2 and have no problems with composer.lock.

@slasher13 the problem with changing the requirements to ^1.23.1|^2 is that we can't change the composer.lock only for PHP7 so we have no idea of whether all the bridge code works.

@alexpott couldn't we have the testbot build the composer per environment?

But we fix it in composer.lock. If someone uses composer to handle dependencies. You have to verify it yourself or you have to wait until Drupal drops support of PHP 5.5. and PHP 5.6. So I think it's worth trying to be compatible with twig 2.

You can stay with ^1.23.1, too. Then I have to change it myself.

Created #2864031: Update twig/twig from v1.25.0 to v1.32.0 for minor version upgrade in 8.3.x.

Re-rolled.

Someone understood this issue to be an expectation that 8.4.x or 8.5.x would actually require Twig 2 and PHP 7. Retitling the issue to be less scary and clarifying in the summary.

Thanks @xjm, nice title change. We are hoping to have both work for BC.

Needs reroll after #2912542: Update twig/twig from v1.32.0 to v1.35.0.

Here's a re-roll of the last patch, I explicitly changed semver to ^2.4.0, hope that's ok?

Mostly applied, except for the composer.lock file and Drupal\twig_extension_test\TwigExtension\TestExtension which I fixed the conflict manually.

Corrected the test failures and rectified some coding standards errors.

I'm rerolled patch from #50
Please review

@aleevas your patch contains the patch - core/2572605-50.patch - can you generate the patch without that file? Thanks.

Remove .patch from patch in #54.

I'm

+++ b/core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php
@@ -177,10 +193,11 @@ public function testActiveThemePath() {
-    $loader = new \Twig_Loader_String();
+    $name = sprintf('__string_template__%s', hash('sha256', uniqid(mt_rand(), TRUE), FALSE));
+    $loader = new \Twig_Loader_Array([$name => '{{ active_theme_path() }}']);

These look a bit sketch, I wonder if we can get away without having to generate a name like this.

Use a hard-coded string for $name (in multiple locations).

Just a little help with cs issues.

Re-roll.

Re-roll without redundant file(!)

After 2(!) empty patches, perhaps this re-roll will work.

@Jo Fitzgerald perhaps you can review #2600154: Update our Twig code to be ready for Twig 2.x which takes care of some of the test stuff and deprecations. It'll make doing this one much easier.

1. +++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.php
   @@ -17,13 +17,21 @@ class TwigNodeTrans extends \Twig_Node {
   +    if (NULL !== $body) {
   +      $nodes['body'] = $body;
   +    }
   

   Body can't be NULL it's not optional.

2. +++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.php
   @@ -32,29 +40,27 @@ public function __construct(\Twig_Node $body, \Twig_Node $plural = NULL, \Twig_N
   -    $compiler->write('echo ' . (empty($plural) ? 't' : '\Drupal::translation()->formatPlural') . '(');
   +    $compiler->write('echo ' . ($this->hasNode('plural') ? '\Drupal::translation()->formatPlural' : 't') . '(');
    
        // Move the count to the beginning of the parameters list.
   -    if (!empty($plural)) {
   -      $compiler->raw('abs(')->subcompile($this->getNode('count'))->raw('), ');
   +    if ($this->hasNode('plural')) {
   +      $compiler->raw('abs(')->subcompile($this->hasNode('count') ? $this->getNode('count') : NULL)->raw('), ');
        }
    
        // Write the singular text parameter.
        $compiler->subcompile($singular);
    
        // Write the plural text parameter, if necessary.
   -    if (!empty($plural)) {
   +    if ($this->hasNode('plural')) {
   

   These changes are unnecessary aren't needed.

3. +++ b/core/lib/Drupal/Core/Template/TwigTransTokenParser.php
   @@ -21,10 +21,10 @@ class TwigTransTokenParser extends \Twig_TokenParser {
   -    $body = NULL;
   -    $options = NULL;
   -    $count = NULL;
        $plural = NULL;
   +    $count = NULL;
   +    $options = NULL;
   +    $body = NULL;
   

   This change looks unnecessary.

In some ways I think it'd be good to split the TwigNodeTrans into a separate issue and remove the related deprecations in \Drupal\Tests\Listeners\DeprecationListenerTrait::getSkippedDeprecations()

+++ b/core/composer.json
@@ -31,7 +31,7 @@
-        "twig/twig": "^1.35.0",
+        "twig/twig": "^1.35.0|^2.4.0",

It is important to test the patch on both php5 and php7 to make sure tests are passing on twig 1 and 2.

Fixing the deprecations should be the aim of #2600154: Update our Twig code to be ready for Twig 2.x. This issue should only be about the possible change to composer.json to support twig 2.0.

This issue has never actually tested Twig 2.0 it has just permitted a project to install it. Before we change the constraint we need a patch on this issue that forces testbot to run a test with Twig 2. All the other work to stop us triggering deprecated code should take place in #2600154: Update our Twig code to be ready for Twig 2.x

Postponing on #2600154: Update our Twig code to be ready for Twig 2.x

#2600154: Update our Twig code to be ready for Twig 2.x is fixed

Since we're no longer going to be supporting PHP 5.6 in 8.7, is there any reason at all to keep the Twig 1 support now that this issue targets 8.7 instead of 8.6 ?

To not force BC breaks onto contrib modules/themes that use Twig 1.x features that were removed in 2.x.

reroll after #2591515: Move twig_without() to the TwigExtension where all the other filters are and deprecate & #2600154: Update our Twig code to be ready for Twig 2.x

Check for upgrade

Are the new problems found new in Twig 2.x since #2600154: Update our Twig code to be ready for Twig 2.x or are we finding them because we did not have these tests to prove it worked?

Either way it sounds like we'll need a testing issue at least ongoing to prove we keep compatibility since we cannot just include a test in core to keep it tested (patch composer files and then build a test environment) can we?

I think we should open an issue like #3020303: Consider using Symfony flex to allow switching between major Symfony versions to make an install with Twig 2 possible, this will then allow regular core tests against Twig 2 once we have min/max composer testing. Although an ongoing issue with a patch we can run tests against would be good until then.

Ir looks like most of them are new, not clear why testloader fails and why it was not updated early

FYI #2976394: Allow Symfony 4.4 to be installed in Drupal 8 found deprecated API use for removal in Twig 3. I am surprised that did not come up here as fails since they are already marked deprecated without updating twig at all.

@Gabor: That is because those deprecation messages are again from the debug classloader that's new in Symfony 4 and not triggered by Twig itself. Also I guess those are more examples that we just need to ignore, as I assume that whatever's the replacement for that interface isn't available yet in Twig 1.x?

@berdir: yeah the concrete issue is #3030494: [Twig 3] The "Drupal\Core\Template\Loader\StringLoader" class implements "Twig_ExistsLoaderInterface" and "Twig_SourceContextLoaderInterface" that are deprecated since 1.12 and 1.27 (to be removed in 3.0). where I proposed to ignore them but that did not work :/ There are still genuine fails here that would be great to fix as soon as we can so we can say people can try out Drupal 8.7 with Twig 2 :)

Trying to fix the fails

Yay, thanks @larowlan.

Since you did not post an interdiff, I compared the patches and looks like the effective change you made is this:

Patch in #81:

  public function getSourceContext($name) {
    $name = (string) $name;
    return new Source($name, $name);
  }

Patch in #89:

  public function getSourceContext($name) {
    $name = (string) $name;
    $value = $this->getSource($name);
    return new Source($value, $name);
  }

On the other hand you added 50k worth of additional composer file changes, so not only updating twig anymore but also a bunch of other things, including phpunit and adding various new vendors. We should cut back the testing patch to only updating to twig 2 unless some dependency updates are required to do alongside it.

Ok I removed the composer changes from #89 and rolled a patch with just that. Also rolled in the composer changes from #81 that were actually limited to twig2 updates for testing. This should better represent that #89 is working (or not).

I merely spliced together patches, did not change anything :)

Here we go. No interdiff cuz it only reverts composer.lock changes.

Hm, there is zero diff between @jibran's twig-2572605-2.x-91.patch and my 2572605-89-with-composer-twig2-from-81.patch and yet mine failed in Drupal\Tests\system\Functional\Module\InstallUninstallTest which seemed suspiciously unrelated anyway. Random fail I assume. Ok now we need reviews so we can get this small fix in ASAP :)

I opened the Twig 2 counterpart of #3020303: Consider using Symfony flex to allow switching between major Symfony versions at #3031639: [PP-1] Relax composer.json requirements to allow Drupal 8 to be installed with Twig 2 but marked it postponed on the Symfony 4 one as the mechanics and concerns may be the same and are still under discussion there.

Reuploading the patch to review for clarity. It prepares core to Twig 2 but makes no changes to which twig we ship with. That is the goal of the issue. The patches with composer changes were merely to test.

+++ b/core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php
@@ -10,6 +10,20 @@
 use Drupal\Tests\UnitTestCase;
+/*
+if (!function_exists(__NAMESPACE__ . 't')) {
+
+  function t($string, array $args = []) {
+    return strtr($string, $args);
+  }
+
+}
+if (!function_exists(__NAMESPACE__ . 'file_create_url')) {
+
+  function file_create_url() {
+  }
+
+}*/

any reason to add this commented?

1. This is looking good

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

   This variable should have a comment.

3. +++ b/core/lib/Drupal/Core/Template/TwigTransTokenParser.php
   @@ -55,14 +55,14 @@ public function parse(\Twig_Token $token) {
   -  public function decideForFork($token) {
   +  public function decideForFork(\Twig_Token $token) {
   ...
   -  public function decideForEnd($token) {
   +  public function decideForEnd(\Twig_Token $token) {
   

   As we are changing the argument we should also fix the missing docs.

4. It's nice that the code in #96 is not needed!

Re #96/@andypost:

Removed debug code.

Re #97/@alexpott:

1. Yay!
2a. I don't think that variable is needed at all, it is inherited either way from Twig_Environment and it existed prior to this patch as well with the same value. Based on https://github.com/twigphp/Twig/blob/2.x/lib/Twig/Environment.php this property or its value did not change in Twig 2 either. Just removed it.
2b. While I was there I added docs to the single undocumented variable above it which is our own code. I copied the text verbatim from TwigPhpStorageCache
3. We are not changing the argument, Twig 1 was/is also passing $token as the same type. I noticed we copied the method names from the 'for' tag, and Twig otherwise uses different method naming based on the tag name, so interesting... Either way, not a good opportunity to change now.
4. Removed as per above.

@Gábor Hojtsy thanks for addressing the feedback. This looks good to go now.

Here is a version with the twig2 composer changes only for testing. Also uploading #98 again for sake of committer clarity.

Also updating issue summary heavily.

Adding Drupal 9 tag and further clarify the title.

Glad we had the testing patch :) TwigEnvironment::$templateClassPrefix became private (from protected) between 1.x and 2.x

Patch attached addresses the fails in #101 by calling parent::getTemplateClass() so we no longer need to access the private.

Whilst making this change I noticed that \Drupal\Core\Template\TwigEnvironment::invalidate() was trying to set \Twig_Environment::$loadedTemplates to an empty array. This will no longer work because it is also private. PHP does not complain because it just creates a public property of the same name. However, setting this to an empty array has always been bogus because this property is filled by doing:

$this->loadedTemplates[$cls] = new $cls($this);

As there is no way of unloading code in PHP once for the same value of $cls PHP will never return something different. So I think we should remove the $this->loadedTemplates = []; line.

+++ b/core/tests/Drupal/Tests/Core/Template/AttributeTest.php
@@ -280,7 +280,7 @@ public function testTwigAddRemoveClasses($template, $expected, $seed_attributes
-      ["{{ attributes.class }}", ''],
+      ["{{ attributes }}", ''],

Isn't this a BC break?

@Wim Leers I think Twig is allowed to break its APIs between version 1 and 2.

Isn't this a BC break?

This change is in the 1.x patch, so perhaps needs further investigation?

Well let's see what happens without that change. The following tests after where that change was still test the .class so that should work as well as before. The other test changes made only affect how the test template loading happens which should not affect this.

There is existing use of attributes.class in tests and some modules, neither of which needed to change so I am hopeful that this was not necessary to change in the test.

core/modules/system/tests/modules/twig_theme_test/templates/twig_theme_test.filter.html.twig:<div><span class="{{ attributes.class }}"{{ attributes|without('class') }}>Class attributes in front, remainder at the back:</span></div>
core/modules/system/tests/modules/twig_theme_test/templates/twig_theme_test.filter.html.twig:<div><span{{ attributes|without('class') }} data-class="{{ attributes.class }}">Class attributes in back, remainder at the front:</span></div>
core/modules/system/tests/modules/twig_theme_test/templates/twig_theme_test.filter.html.twig:<div><span class="{{ attributes.class }}">Class attributes only:</span></div>
core/modules/update/templates/update-version.html.twig:<div class="{{ attributes.class }} project-update__version"{{ attributes|without('class') }}>
core/themes/stable/templates/admin/update-version.html.twig:<div class="{{ attributes.class }} project-update__version"{{ attributes|without('class') }}>

Ok this fails with the following error:

Drupal\Tests\Core\Template\AttributeTest::testTwigAddRemoveClasses with data set #0 ('{{ attributes.class }}', '')
ArgumentCountError: Too few arguments to function Drupal\Core\Template\Attribute::hasClass(), 0 passed in /var/www/html/vendor/twig/twig/lib/Twig/Extension/Core.php on line 1626 and exactly 1 expected

Directly this is because Drupal\Core\Template\Attribute::hasClass() expects a class name and returns true if that class is set on the attributes object, otherwise returns FALSE. But why is that called in the first place?

From https://twig.symfony.com/doc/2.x/templates.html

For convenience's sake foo.bar does the following things on the PHP layer:

• check if foo is an array and bar a valid element;
• if not, and if foo is an object, check that bar is a valid property;
• if not, and if foo is an object, check that bar is a valid method (even if bar is the constructor - use __construct() instead);
• if not, and if foo is an object, check that getBar is a valid method;
• if not, and if foo is an object, check that isBar is a valid method;
• if not, and if foo is an object, check that hasBar is a valid method;
• if not, return a null value.

Why is this a new problem with Twig 2? Because Twig 1 does not have the hasBar() part. See https://twig.symfony.com/doc/1.x/templates.html:

For convenience's sake foo.bar does the following things on the PHP layer:

• check if foo is an array and bar a valid element;
• if not, and if foo is an object, check that bar is a valid property;
• if not, and if foo is an object, check that bar is a valid method (even if bar is the constructor - use __construct() instead);
• if not, and if foo is an object, check that getBar is a valid method;
• if not, and if foo is an object, check that isBar is a valid method;
• if not, return a null value.

So a method we have on Drupal\Core\Template\Attribute is now in the middle of Twig's expected extension API.

Removing testing of this is certainly not the solution :D It would also not be a solution to make this method return TRUE when there was some class (and make the $class argument optional) because that would change the twig output from the empty output we expect (based on an expected null return value).

The sole reason we have this hasClass() method seems to be to test attributes. At least according to this grep:

$ git grep "\->hasClass"
core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditJavascriptTestBase.php:    $this->assertFalse($toolbar_edit_button->hasClass('is-active'), 'The "Edit" button in the toolbar is not yet marked as active.');
core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditJavascriptTestBase.php:      $this->assertTrue($dom_node->hasClass('visually-hidden'), 'The contextual links trigger "' . $dom_node->getParent()->getAttribute('data-contextual-id') . '" is hidden.');
core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditJavascriptTestBase.php:    $this->assertTrue($toolbar_edit_button->hasClass('is-active'), 'The "Edit" button in the toolbar is marked as active.');
core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditJavascriptTestBase.php:      $this->assertFalse($dom_node->hasClass('visually-hidden'), 'The contextual links trigger "' . $dom_node->getParent()->getAttribute('data-contextual-id') . '" is visible.');
core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditJavascriptTestBase.php:    $this->assertFalse($entity_contextual_links_container->hasClass('open'));
core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditJavascriptTestBase.php:    $this->assertTrue($entity_contextual_links_container->hasClass('open'));
core/modules/system/tests/src/Functional/Pager/PagerTest.php:    $this->assertTrue($element->hasClass($class) !== FALSE, $message);
core/modules/system/tests/src/Functional/Pager/PagerTest.php:    $this->assertTrue($element->hasClass($class) === FALSE, $message);
core/modules/toolbar/tests/src/FunctionalJavascript/ToolbarIntegrationTest.php:    $this->assertFalse($tray->hasClass('toolbar-tray-vertical'), 'Toolbar tray is not vertically oriented by default.');
core/modules/toolbar/tests/src/FunctionalJavascript/ToolbarIntegrationTest.php:    $this->assertTrue($tray->hasClass('toolbar-tray-vertical'), 'After toggling the orientation the toolbar tray is now displayed vertically.');
core/modules/toolbar/tests/src/FunctionalJavascript/ToolbarIntegrationTest.php:    $this->assertTrue($tray->hasClass('toolbar-tray-horizontal'), 'After toggling the orientation a second time the toolbar tray is displayed horizontally again.');
core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php:    $this->assertEquals(TRUE, $machine_name_1_wrapper->hasClass('visually-hidden'), 'The ID field must not be visible');
core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php:    $this->assertEquals(TRUE, $machine_name_2_wrapper->hasClass('visually-hidden'), 'The ID field must not be visible');
core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php:    $this->assertEquals(FALSE, $machine_name_1_wrapper->hasClass('visually-hidden'), 'The ID field must now be visible');
core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php:    $this->assertEquals(TRUE, $machine_name_2_wrapper->hasClass('visually-hidden'), 'The ID field must not be visible');
core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php:    $this->assertTrue($assert->fieldExists('id')->hasClass('error'));
core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php:    $this->assertFalse($assert->fieldExists('id2')->hasClass('error'));
core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php:    $this->assertTrue($assert->fieldExists('id')->hasClass('error'));
core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php:    $this->assertTrue($assert->fieldExists('id2')->hasClass('error'));
core/tests/Drupal/FunctionalJavascriptTests/TableDrag/TableDragTest.php:      return $handle->hasClass($class);
core/tests/Drupal/FunctionalJavascriptTests/TableDrag/TableDragTest.php:      return !$handle->hasClass($this::DRAGGING_CSS_CLASS);
core/tests/Drupal/Tests/Core/Template/AttributeTest.php:    $this->assertFalse($attribute->hasClass('a-class-nowhere-to-be-found'));
core/tests/Drupal/Tests/Core/Template/AttributeTest.php:    $this->assertTrue($attribute->hasClass('we-totally-have-this-class'));

So the question is how dangerous would it be to rename this method at this point on Drupal\Core\Template\Attribute or make some ugly workaround to make it work like an offsetGet('class') if no argument was given and add a todo to remove this ugly thing in D9 :)

Here is the ugly workaround solution, let's discuss.

+++ b/core/lib/Drupal/Core/Template/Attribute.php
@@ -264,13 +264,27 @@ public function removeClass() {
+   * @todo Rename this method in Drupal 9. Maybe deprecate it now?
...
+    if (!isset($class)) {
+      return $this->offsetGet('class');
+    }

I don't think changing this API to specifically support Twig 2 in this way is nice. I also don't think we should be deprecating this method since it was added for a reason and I don't think that has changed. I assume this problem will occur in other places as well, and we don't want to suggest this being an ideal solution for the problem. I suggest we try to solve this by an if clause that would allow us to specifically check if ->class has been set.

I think we can fix this in a more elegant way. I'm not sure that I agree with hassers being getters especially with respect to Twig - it was added in https://github.com/twigphp/Twig/pull/1969 - bascially to make Twig more like https://symfony.com/doc/current/components/property_access.html#using-ha... - but making a has* return anything other than a bool is really really odd so I don't think twig_get_attribute or \Twig_Template::getAttribute() should use them. Will open an issue against Twig later.

The more elegant fix it to implement getClass() to get the class attribute and call offsetGet() in it. That way it will take precedence over our hasClass() implementation which I don't think we should be changing - hasClass() is used a lot in contrib - see http://grep.xnddx.ru/search?text=-%3EhasClass%28&filename=

Duh, I should have just added a getClass() method based on the twig lookup list, yeah :D

Opened https://github.com/twigphp/Twig/issues/2833 to address the hassers as getters.

+++ b/core/lib/Drupal/Core/Template/TwigTransTokenParser.php
@@ -54,15 +54,21 @@ public function parse(\Twig_Token $token) {
-  public function decideForFork($token) {
+  public function decideForFork(\Twig_Token $token) {
...
-  public function decideForEnd($token) {
+  public function decideForEnd(\Twig_Token $token) {

As per #2887179-5: Fix the typehinting in CurrentPathStack

So adding a typehint can break BC if a passed-in value does not match the typehint.

As this method doesn't exist on an interface an can be used directly I agree this is a BC break.

+++ b/core/tests/Drupal/KernelTests/Core/Theme/TwigEnvironmentTest.php
@@ -216,7 +216,7 @@ public function testTemplateInvalidation() {
+    $reflection = new \ReflectionClass('\Twig_Environment');

I'd better used \Twig_Environment::class instead of string for better code navigation

@jibran: these are callbacks passed onto the Twig parser, I don't believe there is any meaningful result if called otherwise, they are merely public so the Twig parser can invoke them. They also already requires the argument to be an object instance with a test method on it that takes a string or array even if we don't add the typehint.

re @jibran, I looked through Twig 2.6.2's Parser class code and there only seems to be an expectation that the callable's class be an instanceof Twig_TokenParserInterface and even that only in an error condition. That said, Twig 2 does typehint its callables this way but there may not be a reason to force us to do so. (Again I did not write this patch, and I am contributing by poking at things :D).

So it does not hurt to try to not to add the typehint for safety sake. And see what happens then.

Fixed #118, IMO it's ready to go

This looks good now.

Agreed this looks really solid now, good to see all the iterations.

Committed 9ec3433 and pushed to 8.7.x. Thanks!

Next step is to #3031639: [PP-1] Relax composer.json requirements to allow Drupal 8 to be installed with Twig 2 (which is dependent on some process discussions in another issue, see there) and #2976407: Use drupalci.yml for composer dependency min/max testing - DO NOT COMMIT to ensure there are no regressions in the future.

I opened #3032695: Manually test Drupal 8 with Twig 2 for occassional manual testing for now.

Seems worthy of calling this out!