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

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

Open
Kocal opened this issue Oct 25, 2024 · 2 comments
Open

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

Kocal opened this issue Oct 25, 2024 · 2 comments

Comments

@Kocal
Copy link
Contributor

Kocal commented Oct 25, 2024

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:

  • always render the attribute
  • always NOT render the attribute
    and depending of your environment, you register on of the instance.

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
Copy link
Member

stof commented Oct 25, 2024

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
Copy link
Member

stof commented Oct 25, 2024

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:

  • PHP functions
  • PHP static methods
  • public method of the Twig extension itself
  • public method on a Twig runtime object

Non-optimizable callables:

  • closures
  • callables like $this->myProp->someMethod(...) (whether they use the first-class callable or an old-style syntax does not matter)

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants