From 4d5b0c95191ed8219f766f377e6899babaa0c7c2 Mon Sep 17 00:00:00 2001
From: Simon Praetorius <simon@praetorius.me>
Date: Fri, 9 Aug 2024 15:03:14 +0200
Subject: [PATCH] [FEATURE] Support for boolean tag attributes (#880)

TagBasedViewHelpers now have proper support for boolean attributes.
Before this change, it was very cumbersome to generate these with Fluid,
now it's implemented similar to popular JavaScript frameworks:

```
<my:viewhelper async="{true}" />
Result: <tag async="async" />

<my:viewhelper async="{false}" />
Result: <tag />
```

This can also be used in combination with variable casting:

```
<my:viewhelper async="{myString as boolean}" />
```

For compatibility reasons empty strings still lead to the attribute
being omitted from the tag. This might change in the future, however
we don't want to break templates now. ViewHelpers can define
an argument manually and implement the desired behavior in the
render method (this is how the ImageViewHelper in TYPO3 deals with
empty `alt` attributes).
---
 .../ViewHelper/AbstractTagBasedViewHelper.php |   5 +-
 src/Core/ViewHelper/TagBuilder.php            |  12 +-
 tests/Functional/Cases/TagBasedTest.php       | 158 ++++++++++++++----
 .../ViewHelpers/TagBasedTestViewHelper.php    |   7 +
 tests/Unit/Core/ViewHelper/TagBuilderTest.php |  23 +++
 5 files changed, 171 insertions(+), 34 deletions(-)

diff --git a/src/Core/ViewHelper/AbstractTagBasedViewHelper.php b/src/Core/ViewHelper/AbstractTagBasedViewHelper.php
index 2eb2a5f95..ebfd791a5 100644
--- a/src/Core/ViewHelper/AbstractTagBasedViewHelper.php
+++ b/src/Core/ViewHelper/AbstractTagBasedViewHelper.php
@@ -116,8 +116,11 @@ public function initialize()
         }
 
         foreach ($this->additionalArguments as $argumentName => $argumentValue) {
+            // This condition is left here for compatibility reasons. Removing this will be a breaking change
+            // because TagBuilder renders empty strings as empty attributes (as it should be). We might remove
+            // this condition in the future to have a clean solution.
             if ($argumentValue !== null && $argumentValue !== '') {
-                $this->tag->addAttribute($argumentName, (string)$argumentValue);
+                $this->tag->addAttribute($argumentName, $argumentValue);
             }
         }
 
diff --git a/src/Core/ViewHelper/TagBuilder.php b/src/Core/ViewHelper/TagBuilder.php
index d302bf4ed..1e2091b03 100644
--- a/src/Core/ViewHelper/TagBuilder.php
+++ b/src/Core/ViewHelper/TagBuilder.php
@@ -170,7 +170,7 @@ public function ignoreEmptyAttributes(bool $ignoreEmptyAttributes): void
      * Adds an attribute to the $attributes-collection
      *
      * @param string $attributeName name of the attribute to be added to the tag
-     * @param string|\Traversable|array|null $attributeValue attribute value, can only be array or traversable
+     * @param string|bool|\Traversable|array|null $attributeValue attribute value, can only be array or traversable
      *                                                       if the attribute name is either "data" or "area". In
      *                                                       that special case, multiple attributes will be created
      *                                                       with either "data-" or "area-" as prefix
@@ -194,6 +194,16 @@ public function addAttribute(string $attributeName, $attributeValue, bool $escap
                 $this->addAttribute($attributeName . '-' . $name, $value, $escapeSpecialCharacters);
             }
         } else {
+            // This should probably also check for null, but we can't do that for now because of backwards compatibility
+            if ($attributeValue === false) {
+                $this->removeAttribute($attributeName);
+                return;
+            }
+
+            if ($attributeValue === true) {
+                $attributeValue = $attributeName;
+            }
+
             if (trim((string)$attributeValue) === '' && $this->ignoreEmptyAttributes) {
                 return;
             }
diff --git a/tests/Functional/Cases/TagBasedTest.php b/tests/Functional/Cases/TagBasedTest.php
index 195b38ae7..ac3db63b3 100644
--- a/tests/Functional/Cases/TagBasedTest.php
+++ b/tests/Functional/Cases/TagBasedTest.php
@@ -25,67 +25,136 @@ public static function renderTagBasedViewHelperDataProvider(): array
                 [],
                 '<div />',
             ],
-            'empty registered tag attribute' => [
-                '<test:tagBasedTest registeredTagAttribute="" />',
-                "{test:tagBasedTest(registeredTagAttribute: '')}",
+
+            // Arguments that are explicitly defined with type boolean
+            // still retain the original boolean behavior:
+            // string input is interpreted in a way that "true" equals true
+            // and "false" equals false
+            'string as registered bool attribute' => [
+                '<test:tagBasedTest registeredBooleanArgument="test" />',
+                "{test:tagBasedTest(registeredBooleanArgument: 'test')}",
+                [],
+                '<div registeredBooleanArgument="registeredBooleanArgument" />',
+            ],
+            'string true as registered bool attribute' => [
+                '<test:tagBasedTest registeredBooleanArgument="true" />',
+                "{test:tagBasedTest(registeredBooleanArgument: 'true')}",
+                [],
+                '<div registeredBooleanArgument="registeredBooleanArgument" />',
+            ],
+            'string false as registered bool attribute' => [
+                '<test:tagBasedTest registeredBooleanArgument="false" />',
+                "{test:tagBasedTest(registeredBooleanArgument: 'false')}",
+                [],
+                '<div />',
+            ],
+            'string null as registered bool attribute' => [
+                '<test:tagBasedTest registeredBooleanArgument="null" />',
+                "{test:tagBasedTest(registeredBooleanArgument: 'null')}",
+                [],
+                '<div registeredBooleanArgument="registeredBooleanArgument" />', // @todo this should probably behave differently
+            ],
+            'empty registered bool attribute' => [
+                '<test:tagBasedTest registeredBooleanArgument="" />',
+                "{test:tagBasedTest(registeredBooleanArgument: '')}",
                 [],
                 '<div />',
             ],
-            'true as registered tag attribute' => [
-                '<test:tagBasedTest registeredTagAttribute="{var}" />',
-                '{test:tagBasedTest(registeredTagAttribute: var)}',
+            'variable with true as registered bool attribute' => [
+                '<test:tagBasedTest registeredBooleanArgument="{var}" />',
+                '{test:tagBasedTest(registeredBooleanArgument: var)}',
                 ['var' => true],
-                '<div registeredTagAttribute="1" />',
+                '<div registeredBooleanArgument="registeredBooleanArgument" />',
             ],
-            'false as registered tag attribute' => [
-                '<test:tagBasedTest registeredTagAttribute="{var}" />',
-                '{test:tagBasedTest(registeredTagAttribute: var)}',
+            'variable with false as registered bool attribute' => [
+                '<test:tagBasedTest registeredBooleanArgument="{var}" />',
+                '{test:tagBasedTest(registeredBooleanArgument: var)}',
                 ['var' => false],
-                '<div registeredTagAttribute="" />',
+                '<div />',
             ],
-            'null as registered tag attribute' => [
-                '<test:tagBasedTest registeredTagAttribute="{var}" />',
-                '{test:tagBasedTest(registeredTagAttribute: var)}',
+            'variable with null as registered bool attribute' => [
+                '<test:tagBasedTest registeredBooleanArgument="{var}" />',
+                '{test:tagBasedTest(registeredBooleanArgument: var)}',
                 ['var' => null],
                 '<div />',
             ],
-            'undefined variable as registered tag attribute' => [
-                '<test:tagBasedTest registeredTagAttribute="{var}" />',
-                '{test:tagBasedTest(registeredTagAttribute: var)}',
+            'undefined variable as registered bool attribute' => [
+                '<test:tagBasedTest registeredBooleanArgument="{var}" />',
+                '{test:tagBasedTest(registeredBooleanArgument: var)}',
+                [],
+                '<div />',
+            ],
+            'casted variable as registered bool attribute' => [
+                '<test:tagBasedTest registeredBooleanArgument="{var as boolean}" />',
+                '{test:tagBasedTest(registeredBooleanArgument: \'{var as boolean}\')}',
+                ['var' => '0'],
+                '<div />',
+            ],
+            'boolean literal true as registered bool attribute' => [
+                '<test:tagBasedTest registeredBooleanArgument="{true}" />',
+                '{test:tagBasedTest(registeredBooleanArgument: true)}',
+                [],
+                '<div registeredBooleanArgument="registeredBooleanArgument" />',
+            ],
+            'boolean literal false as registered bool attribute' => [
+                '<test:tagBasedTest registeredBooleanArgument="{false}" />',
+                '{test:tagBasedTest(registeredBooleanArgument: false)}',
                 [],
                 '<div />',
             ],
-            'registered tag attribute' => [
-                '<test:tagBasedTest registeredTagAttribute="test" />',
-                "{test:tagBasedTest(registeredTagAttribute: 'test')}",
+            'null literal as registered bool attribute' => [
+                '<test:tagBasedTest registeredBooleanArgument="{null}" />',
+                '{test:tagBasedTest(registeredBooleanArgument: null)}',
                 [],
-                '<div registeredTagAttribute="test" />',
+                '<div />',
             ],
-            'unregistered argument' => [
+
+            // Unregistered ViewHelper arguments take strings as-is. To
+            // create a boolean argument, the passed value needs to have
+            // the correct type, either boolean or null
+            'string as unregistered argument' => [
                 '<test:tagBasedTest foo="bar" />',
                 "{test:tagBasedTest(foo: 'bar')}",
                 [],
                 '<div foo="bar" />',
             ],
+            'string true as unregistered argument' => [
+                '<test:tagBasedTest foo="true" />',
+                "{test:tagBasedTest(foo: 'true')}",
+                [],
+                '<div foo="true" />',
+            ],
+            'string false as unregistered argument' => [
+                '<test:tagBasedTest foo="false" />',
+                "{test:tagBasedTest(foo: 'false')}",
+                [],
+                '<div foo="false" />',
+            ],
+            'string null as unregistered argument' => [
+                '<test:tagBasedTest foo="null" />',
+                "{test:tagBasedTest(foo: 'null')}",
+                [],
+                '<div foo="null" />',
+            ],
             'empty unregistered argument' => [
                 '<test:tagBasedTest foo="" />',
                 "{test:tagBasedTest(foo: '')}",
                 [],
-                '<div />',
+                '<div />', // @todo this should render an empty attribute, however this would be a breaking change in templates
             ],
-            'true as unregistered argument' => [
+            'variable with true as unregistered argument' => [
                 '<test:tagBasedTest foo="{var}" />',
                 '{test:tagBasedTest(foo: var)}',
                 ['var' => true],
-                '<div foo="1" />',
+                '<div foo="foo" />',
             ],
-            'false as unregistered argument' => [
+            'variable with false as unregistered argument' => [
                 '<test:tagBasedTest foo="{var}" />',
                 '{test:tagBasedTest(foo: var)}',
                 ['var' => false],
-                '<div foo="" />',
+                '<div />',
             ],
-            'null as unregistered argument' => [
+            'variable with null as unregistered argument' => [
                 '<test:tagBasedTest foo="{var}" />',
                 '{test:tagBasedTest(foo: var)}',
                 ['var' => null],
@@ -97,6 +166,31 @@ public static function renderTagBasedViewHelperDataProvider(): array
                 [],
                 '<div />',
             ],
+            'casted variable as unregistered argument' => [
+                '<test:tagBasedTest foo="{var as boolean}" />',
+                '{test:tagBasedTest(foo: \'{var as boolean}\')}',
+                ['var' => '0'],
+                '<div />',
+            ],
+            'boolean literal true as unregistered argument' => [
+                '<test:tagBasedTest async="{true}" />',
+                '{test:tagBasedTest(async: true)}',
+                [],
+                '<div async="async" />',
+            ],
+            'boolean literal false as unregistered argument' => [
+                '<test:tagBasedTest async="{false}" />',
+                '{test:tagBasedTest(async: false)}',
+                [],
+                '<div />',
+            ],
+            'null literal as unregistered argument' => [
+                '<test:tagBasedTest async="{null}" />',
+                '{test:tagBasedTest(async: null)}',
+                [],
+                '<div />',
+            ],
+
             'data array' => [
                 '<test:tagBasedTest data="{foo: \'bar\', more: 1}" />',
                 '{test:tagBasedTest(data: {foo: \'bar\', more: 1})}',
@@ -176,7 +270,7 @@ public function renderTagBasedViewHelper(string $source, string $sourceInline, a
         $view->getRenderingContext()->getTemplatePaths()->setTemplateSource($source);
         $view->getRenderingContext()->getViewHelperResolver()->addNamespace('test', 'TYPO3Fluid\\Fluid\\Tests\\Functional\\Fixtures\\ViewHelpers');
         $output = $view->render();
-        self::assertEquals($expected, $output);
+        self::assertEquals($expected, $output, 'tag variant uncached');
 
         $view = new TemplateView();
         $view->assignMultiple($variables);
@@ -184,7 +278,7 @@ public function renderTagBasedViewHelper(string $source, string $sourceInline, a
         $view->getRenderingContext()->getTemplatePaths()->setTemplateSource($sourceInline);
         $view->getRenderingContext()->getViewHelperResolver()->addNamespace('test', 'TYPO3Fluid\\Fluid\\Tests\\Functional\\Fixtures\\ViewHelpers');
         $output = $view->render();
-        self::assertEquals($expected, $output);
+        self::assertEquals($expected, $output, 'inline variant uncached');
 
         // Second run to test cached template parsing
         $view = new TemplateView();
@@ -193,7 +287,7 @@ public function renderTagBasedViewHelper(string $source, string $sourceInline, a
         $view->getRenderingContext()->getTemplatePaths()->setTemplateSource($source);
         $view->getRenderingContext()->getViewHelperResolver()->addNamespace('test', 'TYPO3Fluid\\Fluid\\Tests\\Functional\\Fixtures\\ViewHelpers');
         $output = $view->render();
-        self::assertEquals($expected, $output);
+        self::assertEquals($expected, $output, 'tag variant cached');
 
         $view = new TemplateView();
         $view->assignMultiple($variables);
@@ -201,7 +295,7 @@ public function renderTagBasedViewHelper(string $source, string $sourceInline, a
         $view->getRenderingContext()->getTemplatePaths()->setTemplateSource($sourceInline);
         $view->getRenderingContext()->getViewHelperResolver()->addNamespace('test', 'TYPO3Fluid\\Fluid\\Tests\\Functional\\Fixtures\\ViewHelpers');
         $output = $view->render();
-        self::assertEquals($expected, $output);
+        self::assertEquals($expected, $output, 'inline variant cached');
     }
 
     public static function throwsErrorForInvalidArgumentTypesDatProvider(): array
diff --git a/tests/Functional/Fixtures/ViewHelpers/TagBasedTestViewHelper.php b/tests/Functional/Fixtures/ViewHelpers/TagBasedTestViewHelper.php
index d02351df8..1905095e0 100644
--- a/tests/Functional/Fixtures/ViewHelpers/TagBasedTestViewHelper.php
+++ b/tests/Functional/Fixtures/ViewHelpers/TagBasedTestViewHelper.php
@@ -17,5 +17,12 @@ public function initializeArguments(): void
     {
         parent::initializeArguments();
         $this->registerArgument('registeredArgument', 'string', 'test argument');
+        $this->registerArgument('registeredBooleanArgument', 'boolean', 'boolean argument', false, false);
+    }
+
+    public function render(): string
+    {
+        $this->tag->addAttribute('registeredBooleanArgument', $this->arguments['registeredBooleanArgument']);
+        return $this->tag->render();
     }
 }
diff --git a/tests/Unit/Core/ViewHelper/TagBuilderTest.php b/tests/Unit/Core/ViewHelper/TagBuilderTest.php
index a27915981..d692e8043 100644
--- a/tests/Unit/Core/ViewHelper/TagBuilderTest.php
+++ b/tests/Unit/Core/ViewHelper/TagBuilderTest.php
@@ -9,6 +9,7 @@
 
 namespace TYPO3Fluid\Fluid\Tests\Unit\Core\ViewHelper;
 
+use PHPUnit\Framework\Attributes\DataProvider;
 use PHPUnit\Framework\Attributes\Test;
 use PHPUnit\Framework\TestCase;
 use TYPO3Fluid\Fluid\Core\ViewHelper\TagBuilder;
@@ -242,4 +243,26 @@ public function tagIsNotRenderedIfTagNameIsEmpty(): void
         $tagBuilder->setTagName('');
         self::assertEquals('', $tagBuilder->render());
     }
+
+    public static function handlesBooleanAttributesCorrectlyDataProvider(): array
+    {
+        return [
+            'value false' => [false, '<foo />'],
+            'value true' => [true, '<foo async="async" />'],
+            'value null' => [null, '<foo async="" />'],
+            'string false' => ['false', '<foo async="false" />'],
+            'string true' => ['true', '<foo async="true" />'],
+            'string null' => ['null', '<foo async="null" />'],
+            'atttribute name' => ['async', '<foo async="async" />'],
+        ];
+    }
+
+    #[DataProvider('handlesBooleanAttributesCorrectlyDataProvider')]
+    #[Test]
+    public function handlesBooleanAttributesCorrectly(mixed $attributeValue, string $expected): void
+    {
+        $tagBuilder = new TagBuilder('foo');
+        $tagBuilder->addAttribute('async', $attributeValue);
+        self::assertEquals($expected, $tagBuilder->render());
+    }
 }