Integrate Twig with Symfony VarDumper for improved debugging experience

🙈

I swear that test was passing locally.. But it did fail locally 😬

I improved the tests and it should pass now.

I think we should reconsider the naming of drupal_dump() and dd(). I think dd() originates in Laravel and means dump and die as in stop execution. It means the same thing in Symfony too - see vendor/symfony/var-dumper/Resources/functions/dump.php. For more info see https://laravel.com/docs/9.x/helpers#method-dd

I think we should be adding a single function here called dump - I don't think the "drupal_" adds anything. And dump would do pretty much what it does in Symfony and Laravel too.

Another argument in favour of #8 is if you have core's dev dependencies installed and you do dd() in PHP code it will dump and die.

Wondering if this could fly.. I'm overriding the dump() function when \Symfony\Component\VarDumper\VarDumper is available.

1. +++ b/core/lib/Drupal/Core/Template/DebugExtension.php
   @@ -0,0 +1,49 @@
   +    $var_dumper = '\Symfony\Component\VarDumper\VarDumper';
   ...
   +    $var_dumper = '\Symfony\Component\VarDumper\VarDumper';
   

   Let's make this a private class const

2. Also looking in twig_var_dump() there's

           foreach ($context as $key => $value) {
               if (!$value instanceof Template && !$value instanceof TemplateWrapper) {
                   $vars[$key] = $value;
               }
           }
   

   Should we be doing something similar? I wonder why this is there?

3. +++ b/core/core.services.yml
   @@ -1536,12 +1536,14 @@ services:
   +      - { name: twig.extension, priority: 25 }
   

   Weird - so a lower priority comes first?

Oh and the twig one does:

    if (!$env->isDebug()) {
        return;
    }

Which also seems reasonable in a way. Not sure.

#11.2: I think that may be something specific to the fact that twig_var_dump uses var_dump. The var dumper code is a bit hard to grasp but it looks like it may have some processing for this use case.

#11.3: One with lower priority takes precedent because it's loaded last.

Got some feedback from @alexpott on Slack. This should address that.

+++ b/core/lib/Drupal/Core/Template/DebugExtension.php
@@ -0,0 +1,70 @@
+        new TwigFunction('dump', [self::class, 'dump'], ['needs_context' => TRUE, 'needs_environment' => TRUE]),

I think this needs 'is_variadic' => TRUE in the options too now.

That's true 👍

1. I like all the new improvements

2. +++ b/core/lib/Drupal/Core/Template/DebugExtension.php
   @@ -0,0 +1,70 @@
   +final class DebugExtension extends AbstractExtension {
   ...
   +    if (class_exists($this::SYMFONY_VAR_DUMPER_CLASS)) {
   ...
   +        call_user_func(static::SYMFONY_VAR_DUMPER_CLASS . '::dump', $context);
   

   final and static or $this... imo we should be doing self:: everywhere here. There's no point doing $this::

3. +++ b/core/lib/Drupal/Core/Template/DebugExtension.php
   @@ -0,0 +1,70 @@
   +        foreach ($variables as $variable) {
   +          call_user_func(static::SYMFONY_VAR_DUMPER_CLASS . '::dump', $variable);
   +        }
   

   This can be a 1 liner with less variables :)

   array_walk($variables, self::SYMFONY_VAR_DUMPER_CLASS . '::dump');
   

This should address #18. Thanks for the reviews! 🙏

Committed and pushed 07994b06fb to 10.1.x and 6c1225de86 to 10.0.x and 6425af2663 to 9.5.x. Thanks!

I'm a bit late to the party, but don't you think that PHPDoc for the $variables parameter is wrong?

@param array $variables

References:
https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/modules/upda...
https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/lib/Drupal/C...

Also we could have native mixed typehint for that parameter.

I think we could fix that in a follow-up 👍 I'm happy to review that so feel free to link that here.

Submitted a follow-up.
#3307344: Wrong documentation for DebugExtension::dump