Move twig_without() to the TwigExtension where all the other filters are and deprecate

Yup.

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.

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.

Splitting #2568181: [META] Update to Twig 2.x in Drupal 9 and these changes make the most sense here. Also have some other BC ideas but want to get this up.

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

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.

Never mind that idea: https://github.com/twigphp/Twig/commit/9774f4f1a43ae7c1dfd952430b1826217...

#14.1 was intentional, that change is needed for Twig 2.x and wasn't sure where to put it when splitting #2568181: [META] Update to Twig 2.x in Drupal 9.

Regarding #14.3 what method?

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.

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

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.

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.

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

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

1. +++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
   @@ -58,10 +58,6 @@ class TwigEnvironment extends \Twig_Environment {
   -    // 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';
   
   +++ b/core/tests/Drupal/KernelTests/Core/Theme/TwigWhiteListTest.php
   @@ -113,6 +113,9 @@ protected function setUp() {
      public function testWhiteListChaining() {
   +    // Include the Twig engine so we can call twig_render_template().
   +    require_once $this->root . '/core/themes/engines/twig/twig.engine';
   

   I don't think we can do this in a minor as it could result in code breaking that is relying on it. For example if something calls the deprecated twig_without function. We should file a follow-up to remove this in Drupal 9.

2. +++ b/core/themes/engines/twig/twig.engine
   @@ -133,20 +133,13 @@ function twig_render_template($template_file, array $variables) {
   + * @deprecated in Drupal 8.5.x and will be removed before 9.0.0. Use
   + *   \Drupal\Core\Template\TwigExtension::withoutFilter() instead.
   

   We only deprecate stuff in the next minor release - so 8.6.x.

3. +++ b/core/themes/engines/twig/twig.engine
   @@ -133,20 +133,13 @@ function twig_render_template($template_file, array $variables) {
   +  /** @var \Drupal\Core\Template\TwigExtension $extension */
   +  $extension = \Drupal::service('twig.extension');
   +
   +  return call_user_func_array([$extension, 'withoutFilter'], func_get_args());
   

   This needs to trigger a silenced error - see https://www.drupal.org/core/deprecation - we also need a change record.

Draft Change Record
Also added trigger error and removed potentially breaking changes as noted in #2591515-46: Move twig_without() to the TwigExtension where all the other filters are and deprecate

Opened the follow-up and added a todo #3011393: Remove twig_without() and manual inclusion of twig.engine TwigEnvironment to address #46.1.

Thanks for the review @joelpittet!

+++ b/core/themes/engines/twig/twig.engine
@@ -133,20 +133,14 @@ function twig_render_template($template_file, array $variables) {
+  @trigger_error('twig_without() is deprecated in Drupal 8.7.x and will be removed before Drupal 9.0.0. Use \Drupal\Core\Template\TwigExtension::withoutFilter(). See https://www.drupal.org/node/3011154.', E_USER_DEPRECATED);
+  /** @var \Drupal\Core\Template\TwigExtension $extension */
+  $extension = \Drupal::service('twig.extension');
+
+  return call_user_func_array([$extension, 'withoutFilter'], func_get_args());

Given there is some complexity here of calling with a number of args I think it is worth a deprecation test. I think a kernel test would probably be easiest.

@joelpittet can you add the @expectedDeprecation annotation to the legacy test. That way we are testing that the code changes don't work and that the expected deprecation message is triggered.

+++ b/core/modules/system/tests/src/Kernel/Theme/TwigFilterTest.php
@@ -120,4 +121,39 @@ public function testTwigWithoutFilter() {
+   * @expectedDeprecation The twig_without function is deprecated since version 8.7.x and will be removed in 9.0.0.

This should be
* @expectedDeprecation twig_without() is deprecated in Drupal 8.7.x and will be removed before Drupal 9.0.0. Use \Drupal\Core\Template\TwigExtension::withoutFilter(). See https://www.drupal.org/node/3011154.

It needs to be the full deprecation message.

It's worth running tests before you upload them :) ie. something like:
./vendor/bin/phpunit -v -c ./core core/modules/system/tests/src/Kernel/Theme/TwigFilterTest.php --filter testLegacyTwigWithoutFunction

This addresses feedback from @alexpott! Thank you @joelpittet!

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

Remaining issue for 2.0 compatibility is #2572605: Make Drupal 8 optionally working with Twig 2, while keeping its dependency on Twig 1