Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible legal values for parameters #151

Open
ulrichmathes opened this issue Sep 19, 2023 · 7 comments
Open

Possible legal values for parameters #151

ulrichmathes opened this issue Sep 19, 2023 · 7 comments
Labels
enhancement New feature or request question Further information is requested

Comments

@ulrichmathes
Copy link
Member

ulrichmathes commented Sep 19, 2023

It would be nice to define possible legal values for an parameter. Without having to introduce a custom data structure.

<fc:param name="align" type="string" cases="{'left', 'right'}" />

// or more slimline with cases fromString:
<fc:param name="align" type="string" cases="left,right" />
// or ?
<fc:param name="align" type="string" cases="left|right" />

I think an implementation would not a big deal.

I thought about to enable enum as parameter type but we would not be able to pass an enum case in an fluid template. So the value of the parameter would be a string anyway. To then pass the string to the tryFrom function of the enum does not feel right and overdone.
Maybe enum will have usecases but for many cases I can think of now, a simple list, defined within the parameter tag would be very handy.

[written on smartphone, pull request will follow]

@ulrichmathes ulrichmathes added the enhancement New feature or request label Sep 19, 2023
@ulrichmathes
Copy link
Member Author

Does such a feature makes sense?

@ulrichmathes ulrichmathes added the question Further information is requested label Sep 20, 2023
@sascha-egerer
Copy link

IMO it does but it can't be implemented like this as far as I could see. The parameters are converted to native fluid parameters and there it is not possible to add an additional attribute like yours.
Our workaround was to implement an additional viewhelper that does take care of value validation.
Enum would also be something that should be supported but this does also not solve many uses cases (like if something should be in a range of numbers or something like that).
I think adding validators like in extbase would be the best way to solve this but i think this would require a bigger change due to the reason mentioned in the beginning.

@sascha-egerer
Copy link

This is the viewhelper I've used for such cases


<?php
declare(strict_types = 1);

namespace xxx\xxx\ViewHelper;

use SMS\FluidComponents\Domain\Model\ComponentInfo;
use SMS\FluidComponents\Exception\InvalidArgumentException;
use TYPO3Fluid\Fluid\Core\Rendering\RenderingContextInterface;
use TYPO3Fluid\Fluid\Core\ViewHelper\AbstractViewHelper;
use TYPO3Fluid\Fluid\Core\ViewHelper\ArgumentDefinition;
use TYPO3Fluid\Fluid\Core\ViewHelper\Traits\CompileWithRenderStatic;

class ParamValidatorViewHelper extends AbstractViewHelper
{
    use CompileWithRenderStatic;

    /**
     * @var bool
     */
    protected $escapeOutput = false;

    public function initializeArguments(): void
    {
        $this->registerArgument('name', 'string', 'Name of variable to validate', true);
        $this->registerArgument('allowed', 'array', 'List of values to validate against', true);
        $this->registerArgument('inList', 'bool', 'Validate if the given values are in the given list', false, false);
    }

    /**
     * @param array<mixed> $arguments
     */
    public static function renderStatic(array $arguments, \Closure $renderChildrenClosure, RenderingContextInterface $renderingContext): string
    {
        $propertyValue = $renderingContext->getVariableProvider()->get($arguments['name']);
        $componentInfo = $renderingContext->getVariableProvider()->get('component');
        $allowedValues = $arguments['allowed'];
        $componentName = $renderingContext->getControllerName() . '::' . $renderingContext->getControllerAction();
        if ($componentInfo instanceof ComponentInfo) {
            $componentName = $componentInfo->namespace ?? $componentName;
            $argumentDefinition = $componentInfo->argumentDefinitions[$arguments['name']] ?? null;
            if (!$argumentDefinition instanceof ArgumentDefinition) {
                throw new \LogicException(sprintf('Can not validate values for param "%s", because it is not registered in component "%s"', $arguments['name'], $componentName), 1670863756);
            }
            if (!$argumentDefinition->isRequired()) {
                // If an optional property is not set on the tag, Fluid will assign the default value to the argument
                // Hence we allow default values as allowed values
                // This also means if no default is set (default is null),
                // that null values are allowed to be passed to the tag property (e.g. through a variable)
                $allowedValues[] = $argumentDefinition->getDefaultValue();
                // If a default value is set for a component prop and the property on the tag is set as well,
                // but a null value is explicitly passed (e.g. <s:component.headline type="1" style="{does.not.exist}" />),
                // Fluid Components assigns this null value to the prop instead of the default.
                // To avoid to handle this case with a condition in the templates, we modify the value
                // in the variable provider and set it to the default value gracefully.
                if ($propertyValue === null && $argumentDefinition->getDefaultValue() !== null) {
                    $propertyValue = $argumentDefinition->getDefaultValue();
                    // This is a side effect, which is debatable, I'd argue this is a workaround
                    // for an inconsistent behaviour in Fluid/ Fluid Components
                    $renderingContext->getVariableProvider()->add($arguments['name'], $propertyValue);
                }
            }
        }

        $throwException = static function ($value, $allowedValues, string $argumentName, string $componentName): void {
            throw new InvalidArgumentException(sprintf(
                'The given value "%s" for option "%s" is invalid in "%s". Possible values are: %s',
                var_export($value, true),
                $argumentName,
                $componentName,
                implode(
                    ', ',
                    array_map(static fn ($item): string => var_export($item, true), $allowedValues)
                )
            ), 1667555651);
        };

        if (is_array($propertyValue)) {
            if (!self::arrayIsAllowedValue($allowedValues, $propertyValue, $arguments['inList'] ?? false)) {
                $throwException($propertyValue, $allowedValues, $arguments['name'], $componentName);
            }

            return '';
        }

        if (!in_array($propertyValue, $allowedValues, true)) {
            $throwException($propertyValue, $allowedValues, $arguments['name'], $componentName);
        }

        return '';
    }

    /**
     * @param array<mixed> $allowedValues
     * @param array<mixed> $value
     */
    private static function arrayIsAllowedValue(array $allowedValues, array $value, bool $inList = false): bool
    {
        if ($inList) {
            $isAllowed = false;
            foreach ($value as $singleValue) {
                $singleValueResult = self::arrayIsAllowedValue(
                    array_map(fn ($item): array => [$item], $allowedValues),
                    [$singleValue]
                );
                if (!$singleValueResult) {
                    return false;
                }
                $isAllowed = true;
            }
            return $isAllowed;
        }
        foreach ($allowedValues as $allowedValue) {
            if (!is_array($allowedValue)) {
                continue;
            }

            if ($value === $allowedValue) {
                return true;
            }
        }

        return false;
    }
}

@s2b
Copy link
Collaborator

s2b commented Sep 20, 2023

Another possibility just for the "in list" case could be to add literal and union types, like TypeScript has:

https://www.typescriptlang.org/docs/handbook/2/everyday-types.html#literal-types

But I'm not sure if it's worth the trouble implementing this.

@ulrichmathes ulrichmathes changed the title Cases for string (and int?) parameters Possible legal values for parameters Sep 20, 2023
@ulrichmathes
Copy link
Member Author

Something like this?

<fc:param name="align" type="literal" cases="{'left', 'right'}" />
<fc:param name="prime" type="literal" cases="{2, 3, 5, 7}" />

// not valid
<fc:param name="prime" type="literal" cases="{'left', 2, 3, 5, 7}" />

@s2b
Copy link
Collaborator

s2b commented Sep 20, 2023

More like:

<fc:param name="align" type="'left' | 'right'" />

@fgeierst
Copy link

fgeierst commented Oct 2, 2023

+1 for this feature!

I found this example for a Button component with three variants. If both params are false, then it's the default variant.

    <fc:param name="isPrimary" type="boolean" optional="1" default="0" />
    <fc:param name="isSecondary" type="boolean" optional="1" default="0" />

In my view this syntax would be much better:

<fc:param name="variant" type="'default' | 'primary' | 'secondary'" />

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants