Create attributes AsTwigFilter, AsTwigFunction and AsTwigTest to ease extension development

[GromNaN]

One drawback to writing extensions at present is that the declaration of functions/filters/tests is not directly adjacent to the methods. It's worse for runtime extensions because they need to be in 2 different classes. See SerializerExtension and SerializerRuntime as an example.

By using attributes for filters, functions and tests definition, we can make writing extensions more expressive, and use reflection to detect particular options (needs_environment, needs_context, is_variadic).

Example if we implemented the formatDate filter:

Twig/extra/intl-extra/IntlExtension.php

Lines 392 to 395 in aeeec9a

	public function formatDate(Environment $env, $date, ?string $dateFormat = 'medium', string $pattern = '', $timezone = null, string $calendar = 'gregorian', string $locale = null): string
	{
	return $this->formatDateTime($env, $date, $dateFormat, 'none', $pattern, $timezone, $calendar, $locale);
	}

By using the AsTwigFilter attribute, it is not necessary to create the getFilters() method. The needs_environment option is detected from method signature. The name is still required as the method naming convention (camelCase) doesn't match with Twig naming convention (snake_case).

use Twig\Extension\Attribute\AsTwigFilter;

class IntlExtension
{
    #[AsTwigFilter(name: 'format_date')]
    public function formatDate(Environment $env, $date, ?string $dateFormat = 'medium', string $pattern = '', $timezone = null, string $calendar = 'gregorian', string $locale = null): string
    {
        return $this->formatDateTime($env, $date, $dateFormat, 'none', $pattern, $timezone, $calendar, $locale);
    }
}

This approach does not totally replace the current definition of extensions, which is still necessary for advanced needs. It does, however, make for more pleasant reading and writing.

This makes writing lazy-loaded runtime extension the easiest way to create Twig extension in Symfony: symfony/symfony#52748

Related to symfony/symfony#50016

Is there any need to cache the parsing of method attributes? They are only read at compile time, but that can have a performance impact during development or when using dynamic templates.

[GromNaN]

I'll rework the implementation after reading discussions on symfony/symfony#50016

[GromNaN]

Ready for review @fabpot.

[GromNaN]

In all honesty, I'm not totally convinced by this implementation. The fact that extensions have to be registered in 2 places (as a runtime extension and for definition in AttributeExtension) makes them difficult to use with Twig standalone, this is totally hidden for Symfony users with the TwigBundle.

I would like to add a new Environment::add(object $extension) method that would accept any class with attributes. Invocable classes could be used to define filters and functions.

[nicolas-grekas]

What is a bit strange is having one single instance of AttributeExtension be responsible for registering many runtimes.
Maybe we should instead have one AttributeExtension per runtime?
Then it might be easier to use this standalone : addExtension(new AttributeExtension(new AttributeBasedRuntime))) (and such runtimes could have a static factory to make this even easier to create).
Note that this suggestion might be wrong as I didn't think of how runtimes should be made lazy-instantiated.

[kbond]

For reference, this is my attempt/experiment to solve the problem of apps having to create a bunch of twig extensions for things: https://github.com/zenstruck/twig-service-bundle

[GromNaN]

What is a bit strange is having one single instance of AttributeExtension be responsible for registering many runtimes. Maybe we should instead have one AttributeExtension per runtime? Then it might be easier to use this standalone : addExtension(new AttributeExtension(new AttributeBasedRuntime))) (and such runtimes could have a static factory to make this even easier to create). Note that this suggestion might be wrong as I didn't think of how runtimes should be made lazy-instantiated.

In the ExtensionSet, extensions are identified by their class name. The same extension class cannot be registered multiple times with distinct configuration.

I updated this PR so that extension can be registered directly using any class name or object that have the #[AsTwigExtension] attribute (the attribute becomes required).

    $twig = new \Twig\Environment($loader);
-    $twig->addExtension(new \Twig\Extension\AttributeExtension([
-        ProjectExtension::class,
-    ]));
+    $twig->addExtension(ProjectExtension::class);
    $twig->addRuntimeLoader(new \Twig\RuntimeLoader\FactoryLoader([
        ProjectExtension::class => function () use ($lipsumProvider) {
            return new ProjectExtension($lipsumProvider);
        },
    ]));

To use standalone (non-lazy-loaded):

$twig->addExtension(new ProjectExtension($lipsumProvider));

[stof]

thinking about that, I'm wondering whether those asTwigFilter, AsTwigFunction and AsTwigTest attributes should be read by that extension at all.

An alternative solution would be to have an extension in which you inject this list of test, filter and function metadata, and letting TwigBundle perform all this attribute reading at build-time instead.
This would even removed the need for the AsTwigExtension attribute entirely (which would be quite confusing as the class having this attribute is not a twig extension but a twig runtime) because the autoconfiguration system is able to apply autoconfiguration based on method-level attributes already.

[GromNaN]

I think that the logic for reading attributes should be in Twig,and the extension could receive the configuration already extracted. And Symfony TwigBundle will do the job of serializing and caching the configuration.

The serializing may be problematic in the future if closures are used in attributes for safeCallback (RFC for PHP 8.5)

[GromNaN]

thinking about that, I'm wondering whether those asTwigFilter, AsTwigFunction and AsTwigTest attributes should be read by that extension at all.

Here is an other approach where attribute class can create the twig callable themself.
https://github.com/twigphp/Twig/blob/36a0732ee66bf405c2164282626fbdc8c23ae81a/src/Attribute/AsTwigFilter.php

[stof]

making $this->extensions contain a mix of extensions and of runtimes (but not all of them) looks weird to me

[stof]

if this is a way to solve the case of getLastModified, I'd rather add a way to make ExtensionSet aware of a separate list of runtimes needing to be checked (which would allow solving the case where we don't invalidate the cache today if you change the argument names in a runtime while it actually impacts the template compilation if you use named parameters in Twig, by making TwigBundle inject the name of all runtimes in the list).

[fabpot]

Not an extensive review, just things I've spotted while reading the code quickly.

[GromNaN]

What is a bit strange is having one single instance of AttributeExtension be responsible for registering many runtimes. Maybe we should instead have one AttributeExtension per runtime? Then it might be easier to use this standalone : addExtension(new AttributeExtension(new AttributeBasedRuntime))) (and such runtimes could have a static factory to make this even easier to create). Note that this suggestion might be wrong as I didn't think of how runtimes should be made lazy-instantiated.

I refactored once again to get 1 instance of AttributeExtension for each class. This allows to list the encapsulated class in the ExtensionSet::getSignature to invalidate the cache when an extension class is removed.

Also, I removed the ability to pass an instance of an object to AttributeExtension, this would require proxying calls to ExtensionSet::getExtension('MyClass')->functionCall() to the wrapped object. This can be implemented in a follow-up PR.

The doc is up-to-date on how to use it standalone. The Symfony implementation need to be updated.