twigphp / Twig

Twig, the flexible, fast, and secure template language for PHP
https://twig.symfony.com/
BSD 3-Clause "New" or "Revised" License
8.16k stars 1.25k forks source link

Helper to render "data-testid" attribute, conditionally #4417

Open Kocal opened 3 days ago

Kocal commented 3 days ago

Hey :)

Some days ago I've suggested a new feature to the UX team, but we think it would be better if this was part of Twig or twig/extra-test, something like that.

When you write E2E tests, and do assertions on HTML elements, texts, etc... it's a common thing to use dedicated attributes like data-test or data-testid, see :

On several Symfony projects I've worked on, I implemented a dumb Twig function like testid() which conditionally (usually depending of kernel environment/debug mode) returns data-testid="..." or not (I've simplified an existing Twig extension, IDK if this one works):

class TestingExtension extends AbstractExtension
{
    public function __construct(
        private bool $renderAttribute,
    ) {
    }

    public function getFunctions(): array
    {
        return [
            new TwigFunction(
                'testid',
                fn(string $identifier) => $this->renderAttribute ? 'data-testid="'.$identifier.'"' : '',
                ['is_safe' => ['html']]
            ),
        ];
    }
}

And use it like that:

<button type="button" {{ testid('btn-delete') }}>
    Delete
</button>

It's far from perfect, each time you call testid(), you have to check if the attribute must be rendered or not.

Ofc, this can be improved with two extension/runtime instances that will either:

But, we still have useless functions calls in production.

Ideally, we'd like this function call to be removed from the compiled Twig code. This would have no impact on production performance.

Nice to have, it would be great if testid() can works with array, it might make it easier to use with Twig components:

<div{{ attributes.defaults({
    class: 'foo bar',
    ...testid('my-identifier')
}) }}
    ...
</div>

WDYT? Thanks :)

stof commented 3 days ago

Twig callables (i.e. filters, functions and tests) already support customizing the node class used to compile them, allowing to provide custom compilation of the callable call. See the implementation of enum_cases in #4177 for an example.

Nice to have, it would be great if testid() can works with array, it might make it easier to use with Twig components:

having different return types for your function depending on where it is called looks weird to me (and probably impossible to document in a reliable way). You should rather have 2 different functions.

stof commented 3 days ago

Side note: if you care about micro-optimizing the function calls at runtime to avoid calling the function in prod to get an empty string as output, you should first optimize your implementation by defining a callable that does not require triggering initialization of extensions (loading the available filters, functions, etc...) at runtime in order to get the actual callable in the compiled template

Optimizable callables:

Non-optimizable callables:

Second side note: your code snippet marks testid as safe for the html strategy but it is wrong because it does not apply escaping on its argument (if your actual implementation does the same, you have a potential XSS security when rendering attributes)