RFC: TagBasedViewHelpers accept undeclared arguments as tag attributes

[kitsunet]

Hey,

we had the request and I created some code to support that any arguments given to a tag based viewhelper that was not formally declared will simply be cast to string (or throw an exception if impossible) and added as attribute to the tag. This would handling much easier for new attributes in various ways.

What's your opinion, good idea or bad idea and if so, why?

[NamelessCoder]

Sounds like a perfect idea to me. The only objection I could come up with is it makes it ever so slightly harder to debug issues with typos in argument names (result output as attribute in tag, no warning, need to see output source and know about this passthrough to determine problems). But the benefit is significant improvement and universal tag supports so I would happily sacrifice an informative exception.

For your consideration, I'm working on this (and a lot of other improvements) currently:

<?php
declare(strict_types=1);
namespace TYPO3Fluid\Fluid\Core\ViewHelper\Traits;

use TYPO3Fluid\Fluid\Core\Rendering\RenderingContextInterface;
use TYPO3Fluid\Fluid\Core\ViewHelper\ArgumentDefinition;
use TYPO3Fluid\Fluid\Core\ViewHelper\TagBuilder;

/**
 * Tag Generating ViewHelper Trait
 *
 * Implement in ViewHelpers which should generate tags.
 */
trait TagGenerating
{
    /**
     * @return ArgumentDefinition[]
     */
    abstract public function prepareArguments();

    /**
     * @param array $attributes
     * @param RenderingContextInterface $renderingContext
     * @return TagBuilder
     */
    public static function createTag(array $attributes, RenderingContextInterface $renderingContext): TagBuilder {
        $tag = new TagBuilder('div');
        $tag->forceClosingTag(true);
        $tag->ignoreEmptyAttributes();
        if (isset($attributes['data'])) {
            foreach ((array) $attributes['data'] as $name => $value) {
                $attributes['data-' . $name] = $value;
            }
            unset($attributes['data']);
        }
        $tag->addAttributes($attributes);
        return $tag;
    }

    /**
     * @param array $arguments
     * @param TagBuilder $tagBuilder
     * @param \Closure $renderChildrenClosure
     * @param RenderingContextInterface $renderingContext
     * @return string
     */
    public static function renderTag(
        array $arguments,
        TagBuilder $tagBuilder,
        \Closure $renderChildrenClosure,
        RenderingContextInterface $renderingContext
    ): string {
        $tagBuilder->setContent($renderChildrenClosure());
        return $tagBuilder->render();
    }

    /**
     * @param array $arguments
     * @param \Closure $renderChildrenClosure
     * @param RenderingContextInterface $renderingContext
     * @return string
     */
    public static function renderStatic(
        array $arguments,
        \Closure $renderChildrenClosure,
        RenderingContextInterface $renderingContext
    ) {
        return static::renderTag(
            $arguments,
            static::createTag(
                array_diff_key($arguments, (new static)->prepareArguments()),
                $renderingContext
            ),
            $renderChildrenClosure,
            $renderingContext
        );
    }

    /**
     * @param array $arguments
     * @return void
     */
    public function handleAdditionalArguments(array $arguments)
    {
    }

    /**
     * @param array $arguments
     * @return void
     */
    public function validateAdditionalArguments(array $arguments)
    {
    }
}

Combined with:

diff --git a/src/Core/ViewHelper/TagBuilder.php b/src/Core/ViewHelper/TagBuilder.php
index b0ff3b7f..69b62864 100644
--- a/src/Core/ViewHelper/TagBuilder.php
+++ b/src/Core/ViewHelper/TagBuilder.php
@@ -44,6 +44,11 @@ class TagBuilder
     protected $forceClosingTag = false;

     /**
+     * @var bool
+     */
+    protected $removeEmptyAttributes = false;
+
+    /**
      * Constructor
      *
      * @param string $tagName name of the tag to be rendered
@@ -110,10 +115,7 @@ class TagBuilder
      */
     public function hasContent()
     {
-        if ($this->content === null) {
-            return false;
-        }
-        return $this->content !== '';
+        return empty(trim((string) $this->content));
     }

     /**
@@ -167,6 +169,15 @@ class TagBuilder
     }

     /**
+     * @return void
+     */
+    public function ignoreEmptyAttributes()
+    {
+        $this->removeEmptyAttributes = true;
+        $this->attributes = array_filter($this->attributes, function ($item) { return trim((string) $item) !== ''; });
+    }
+
+    /**
      * Adds an attribute to the $attributes-collection
      *
      * @param string $attributeName name of the attribute to be added to the tag
@@ -177,6 +188,9 @@ class TagBuilder
      */
     public function addAttribute($attributeName, $attributeValue, $escapeSpecialCharacters = true)
     {
+        if (trim((string) $attributeValue) === '' && $this->removeEmptyAttributes) {
+            return;
+        }
         if ($escapeSpecialCharacters) {
             $attributeValue = htmlspecialchars($attributeValue);
         }

The Trait can be pretty much used as-is, but in this iteration it must be combined with CompileWithRenderStatic in order to apply render() method.

I will be adding this as a pull request to suggest, as a FEATURE, then follow that up with deprecation of the AbstractTagBasedViewHelper in favor of the trait.

[mneuhaus]

we're talking about attributes that do not start with data- right? because those are already passed through in https://github.com/TYPO3/Fluid/blob/master/src/Core/ViewHelper/AbstractTagBasedViewHelper.php#L161

I'm not sure, if we should blindly pass through any argument. i agree with claus, that this could make debugging a bit harder, because spelling errors would not raise exceptions anymore. imho any non data- attribute should be properly declared in the viewHelper for exactly that reason. Any "ad-hoc" attribute that might need to be added should be prefixed with data- anyway for validity, right?

[NamelessCoder]

Full passthrough of also un-prefixed attributes is added in https://github.com/TYPO3/Fluid/pull/306/files#diff-a45384f75aed9dcc5b22b7a54cdbf66dR49 which is open as PR https://github.com/TYPO3/Fluid/pull/306.

My thinking about this one is that when you write HTML and make a typo, you'd have to view source to find it - and this would be exactly the same. Attempts to use a non-string-compatible value as attribute will still throw an error, though it's different than an "argument not defined" error.

If we allow arbitrary attributes we no longer have to maintain those "universal tag attributes" and standard HTML attributes no longer have to be part of the documentation.

Re: the open PR we might need to add a little checking if an attribute value is an invalid type and then throw a normal parsing error. But apart from that, I don't see any problems allowing arbitrary tags regardless of prefix.

[mneuhaus]

i'd actually vote for not allowing arbitrary stuff. i think this "strictness" of fluid is actually a good thing, because it helps developers to write correct attributes and prevent spelling errors. before we allowed the "data-" attributes it was a bit nasty to add custom attributes, but now that works out of the box. aside from removing the attributes form the documentation i can't think of a reason to allow this. What would an actual use-case be?

[NamelessCoder]

how about a flag property to allow all attributes if someone wants to use it for new viewHelpers?

This could make good sense. I'll work that into the patch somehow so it becomes opt-in to accept arbitrary tags.

[NamelessCoder]

aside from removing the attributes form the documentation i can't think of a reason to allow this. What would an actual use-case be?

I guess the top-three would be:

• Writing XML-fluid and needing arbitrary attributes for otherwise HTML-outputting ViewHelpers
• Ability to add attributes even for third party ViewHelpers also if they do not declare those attributes
• Improved performance due to fewer arguments to be validated in parse step

The more I think about this, the more I lean towards making arbitrary tag support the default or perhaps even the only way...

EDIT: on the point of "don't make it possible to do it wrong" - if you have an arbitrary-tag-accepting ViewHelper and generate docs, any attribute you declared that needs a special type (for example an object like DateTime) will specially show up in the docs and be auto-completed/validated for type. And you'd still declare such arguments specially if you need to accept non-scalar types ;)

[mneuhaus]

XML-Fluid: already works right now, only if you add a specific custom XML ViewHelper it could clash, because you can really assign any attribute to non viewHelper tags, so a combination of for/if viewHelpers with nonViewHelper tags would already work without a problem right now

Third Party ViewHelpers: yea, adding the stuff would mean a bit of manual labour registering the common attributes, you're right. Kind of an crazy idea: we could put a general registry of allowed HTML Tags in the TagViewHelper or some place else, in relation to Tags, that way, if you create a TagViewHelper with the Tag img for example all valid attributes for that tag could be allowed without the need to register them. that would make it "easier" for the developer, remove the "boilerplate registrations" and still have it kind of strict.

Performance: sure, that could make an impact, but i have no idea if it would have an actual impact.

[lolli42]

decision:

• allow arbitrary attributes
• explicitely defined attributes take precedence in processing
• phase out additionalAttributes?!
• look at data="{...}" and aria="{...}" and verify we can easily keep it
• detail: what happens with an attribute that has (or resolves) to empty string? look at existing solutinons ...
• update docs

[kitsunet]

Awesome, I actually looked at this a few hours ago, what a coincidence

[s2b]

This has been implemented in #859