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.

[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.

[nicolas-grekas]

git rebase FTW ;)

[GromNaN]

The state of my reflection is that we would want a different implement in Symfony TwigBundle that reads the attributes during container compilation and inject a pre-compiled instance of AttributeExtension.

This can be done in 2 ways:

• by serializing the AttributeExtension (I'm not sure for the performances),
• or by exposing an API to convert attributes to TwigCallable. Maybe creating a public method on the attribute classes AsTwigFilter::getTwigCallable(\ReflectionFunctionAbstract $reflection): TwigFilter

[GromNaN]

This is for cache invalidation when an extension is added/removed.

Instead of relying on this class check, I can introduce an interface: DelegatingExtensionInterface with the getClass method.

[fabpot]

Maybe we can make it optional and use the method name if no name is provided?

[GromNaN]

There is a conflict in name convention as method names are camelCase and Twig function names are snake_case.

This was discussed with @stof in #3916 (comment).

But I agree that this is duplicate information.

[fabpot]

Those are just conventions ;) And for single word names, that works well.

[GromNaN]

You're right, I made the name optional again.

[stof]

To me, the bigger argument is that Twig filter names are global, and so should be specific enough to avoid conflicts, while the method name is purely internal. For non-core extensions, filter names are generally namespaced with a prefix, but the method names in implementation don't have this prefix in the PHP code as the extension class already separates the PHP code.

[GromNaN]

Getting the method name simplifies the creation of function/filters in the scope of an app. For shared libraries, it's still possible and encouraged to set a prefixed name.

[stof]

Also, my previous comment (linked above) still applies: as the Twig filter name is part of the public API of packages providing an extension (it might even be their main public API if they are all about extending Twig), we won't ever be able to change the algorithm in Twig as changing the guessed name would break those packages (technically, we could by implementing both algorithm in parallel, keeping the old return value and triggering a deprecation in case the new one produces a different result, asking projects to stop relying on guessing, but this means that any change in the algorithm requires waiting for the next major version to be active).

[GromNaN]

Keeping the name of the method is the best that can be done. Trying to apply a transformation (prefix, snake case) would make it impossible to find the name when searching in the source code.

If I understand correctly, what you want is to force developers to think of a name? For me, that's the only good reason to make the name required.

[GromNaN]

I reverted the default name fallback. This can be added later.

[kbond]

Love this!

[kbond]

Does this automatically add the needsEnvironment option? What about needsContext? Is adding array $context enough?

[GromNaN]

The detection of the context is less obvious (no specific type).
And it's very rarely used: dump is the only that uses it in all the Twig extra, Symfony and UX packages. Leaving it explicit seems to me to be more safe.

[kbond]

Agreed, just auto-detecting needsEnvironment seems best for these reasons.

[GromNaN]

I did the detection initially: https://github.com/twigphp/Twig/blob/c5c00ed6c64ea2470f8a0dcb4c1fc18682f8fa5c/src/Extension/Extension.php

[GromNaN]

It would be possible to extend this class to create an extension without runtime dependency. This is interesting to simplify standalone usage only, as Symfony will abstract the runtime declaration part.

class CustomExtension extends AttributeExtension
{
    public function __construct()
    {
        // This can be set automatically for child classes
        parent::__construct(self::class);
    }

    #[AsTwigFunction('foo')]
    public function foo(): string
    {
        return 'foo';
    }
}

Reflection for a subsequent PR.