Create attributes `AsTwigFilter`, `AsTwigFunction` and `AsTwigTest` to ease extension development #3916

GromNaN · 2023-11-26T00:15:57Z

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 serialize 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;
use Twig\Extension\Attribute\AsTwigExtension;

#[AsTwigExtension]
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.

src/Extension/Extension.php

src/Extension/Attribute/AsTwigFilter.php

src/Extension/Attribute/AsTwigTest.php

src/Extension/Attribute/AsTwigFilter.php

src/Extension/Extension.php

GromNaN · 2023-11-26T09:36:45Z

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

stof · 2023-11-27T10:17:41Z

src/Extension/AttributeExtension.php

+                    $parameters = $method->getParameters();
+                    $needsEnvironment = isset($parameters[0]) && Environment::class === $parameters[0]->getType()?->getName();
+                    $firstParam = $needsEnvironment ? 1 : 0;
+                    $needsContext = isset($parameters[$firstParam]) && 'context' === $parameters[$firstParam]->getName() && 'array' === $parameters[$firstParam]->getType()?->getName();


this special behavior of parameter named $context needs to be documented on the attributes.

I added the doc and example in the attribute class header.

The Symfony serialize filter accepts an array $context as 3rd argument. This will not conflict but this means developers could have an unexpected behavior if they don't know about this special argument handling.

GromNaN · 2023-11-30T08:51:41Z

src/Extension/WithLastModified.php

+
+namespace Twig\Extension;
+
+interface WithLastModified


This is also a new feature required to detect when the class that defines attributes have been updated.

Alternatively, we can make passing a class name to the ExtensionSet, that will be scanned by it. This has slightly more impact on existing classes.

@stof I can't replace the modification date by a hash, the Environment::isFresh relies on a date comparison between the ExtensionSet and the cache file mtime.

Twig/src/Environment.php

Lines 416 to 419 in aeeec9a

public function isTemplateFresh(string $name, int $time): bool

{

return $this->extensionSet->getLastModified() <= $time && $this->getLoader()->isFresh($name, $time);

}

LastModifiedAwareInterface? What's the existing convention in Twig?

Renamed to ModificationAwareInterface. The convention if to suffix with Interface.

With the last change, I removed this interface. ExtensionSet is now aware of the extension classes that use attributes, and check their modification date as well.

src/Extension/Attribute/AsTwigExtension.php

src/Extension/AttributeExtension.php

smnandre · 2023-12-02T04:45:25Z

src/Extension/Attribute/AsTwigExtension.php

+ * Identifies a class that uses PHP attributes to define filters, functions, or tests.
+ */
+#[\Attribute(\Attribute::TARGET_CLASS)]
+class AsTwigExtension


What is the purpose of this attribute ? I don't see (yet) how it is / will be used

It's similar to Twig\Extension\RuntimeExtensionInterface, it's a marker for classes that provide extension features, to be automatically tagged by Symfony dependency injection.

Given that extensions must implement the ExtensionInterface anyway, there is no need for a marker attribute. The Symfony dependency injection component is also able to perform auto-configuration based on implemented interfaces (and TwigBundle already does it for the Twig ExtensionInterface

It's more a question of DX. It's more meaningful to use an attribute than an interface as a marker as the developer will use attributes in the class. Implementing an interface without method is a deviation from the interface system.

I've made this attribute mandatory to prevent any object from being passed by mistake. In practice, it will be mandatory anyway for Symfony autoconfiguration.

tests/Extension/Fixtures/ExtensionWithAttributes.php

GromNaN

Ready for review @fabpot.

GromNaN · 2023-12-10T21:41:43Z

doc/advanced.rst

+
+.. note::
+
+    The ``\Twig\Extension\AttributeExtension`` can be added only once to an environment.


There is an exception if we try to register the extension a second time.

GromNaN · 2024-01-03T09:04:27Z

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 · 2024-01-03T09:21:43Z

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 · 2024-01-03T16:13:53Z

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

…as attribute properties

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));

GromNaN · 2024-04-05T14:39:33Z

src/Environment.php

+    /**
+     * @param ExtensionInterface|object|class-string $extension
+     */
+    public function addExtension(object|string $extension)


This is a breaking change.

…extension to be registered

stof · 2024-04-05T14:41:27Z

src/Extension/AttributeExtension.php

+        }
+
+        // Assign all at the end to avoid inconsistent state in case of exception
+        $this->filters = $filters;


you should do array_values on those arrays instead of returning names as keys

stof · 2024-04-05T14:43:04Z

src/Attribute/AsTwigTest.php

+ *
+ * If the first argument of the method has Twig\Environment type-hint, the test will receive the current environment.
+ * If the next argument of the method is named $context and has array type-hint, the test will receive the current context.
+ * The last argument of the method is the value to be tested, if any.


this is wrong. It is not the last argument. It is the following argument. A test can have as many arguments as you want after the value being tested, which become arguments in the Twig signature.

stof · 2024-04-05T14:44:43Z

src/Attribute/AsTwigFilter.php

+        public ?array $preservesSafety = null,
+
+        /**
+         * Set to true if the filter is deprecated.


what is the string type about then ?

stof · 2024-04-05T14:45:14Z

src/Attribute/AsTwigFilter.php

+        public ?string $preEscape = null,
+
+        /**
+         * Preserves the safety of the value that the filter is applied to.


this misses the doc for the type of array values

stof · 2024-04-05T14:50:01Z

src/Extension/AttributeExtension.php

+                throw new \LogicException(sprintf('Extension class "%s" must have the attribute "%s" in order to use attributes', is_string($objectOrClass) ? $objectOrClass : get_debug_type($objectOrClass), AsTwigExtension::class));
+            }
+
+            foreach ($reflectionClass->getMethods() as $method) {


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.

stof · 2024-04-05T14:52:06Z

src/ExtensionSet.php

+            if ($extension instanceof ExtensionInterface) {
+                $this->initExtension($extension);
+            } else {
+                $classes[] = $extension;


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

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

GromNaN force-pushed the attribute branch from 6bde02d to 20cf159 Compare November 26, 2023 00:21

GromNaN mentioned this pull request Nov 26, 2023

[RFC][TwigBundle] Register custom functions & filters using an attribute symfony/symfony#50016

Open

GromNaN changed the title ~~Allows registration of filters, functions and tests with an attribute~~ Create attributes AsTwigFilter, AsTwigFunction and AsTwigTest to improve extension development Nov 26, 2023

GromNaN commented Nov 26, 2023

View reviewed changes

GromNaN changed the title ~~Create attributes AsTwigFilter, AsTwigFunction and AsTwigTest to improve extension development~~ Create attributes AsTwigFilter, AsTwigFunction and AsTwigTest to ease extension development Nov 26, 2023

GromNaN force-pushed the attribute branch from 20cf159 to c432044 Compare November 26, 2023 00:51

derrabus reviewed Nov 26, 2023

View reviewed changes

src/Extension/Attribute/AsTwigFilter.php Outdated Show resolved Hide resolved

src/Extension/Extension.php Outdated Show resolved Hide resolved

GromNaN mentioned this pull request Nov 26, 2023

[TwigBundle] Enable #[AsTwigFilter], #[AsTwigFunction] and #[AsTwigTest] attributes to configure runtime extensions symfony/symfony#52748

Open

GromNaN force-pushed the attribute branch 3 times, most recently from d505321 to e64a54b Compare November 27, 2023 08:53

stof reviewed Nov 27, 2023

View reviewed changes

GromNaN force-pushed the attribute branch from 9d3dd5b to 99999e9 Compare November 30, 2023 08:49

GromNaN commented Nov 30, 2023

View reviewed changes

src/Extension/Attribute/AsTwigExtension.php Outdated Show resolved Hide resolved

smnandre reviewed Dec 2, 2023

View reviewed changes

src/Extension/AttributeExtension.php Outdated Show resolved Hide resolved

smnandre reviewed Dec 2, 2023

View reviewed changes

src/Extension/AttributeExtension.php Show resolved Hide resolved

smnandre reviewed Dec 2, 2023

View reviewed changes

tests/Extension/Fixtures/ExtensionWithAttributes.php Show resolved Hide resolved

GromNaN force-pushed the attribute branch 4 times, most recently from c627a38 to 4c1adc1 Compare December 10, 2023 21:38

GromNaN commented Dec 10, 2023

View reviewed changes

GromNaN requested review from stof, derrabus and smnandre December 10, 2023 21:42

smnandre approved these changes Dec 12, 2023

View reviewed changes

GromNaN force-pushed the attribute branch from fb3ca3d to 5bfb738 Compare December 26, 2023 20:03

stof mentioned this pull request Jan 4, 2024

[Twig-Bridge] [RFC] Allow to declare twig extensions via config ? symfony/symfony#53403

Open

stof mentioned this pull request Feb 13, 2024

Add #[YieldReady] to allow extensions to tell when they're ready for yielding #3999

Merged

GromNaN mentioned this pull request Feb 14, 2024

What would you like to see as major changes that would lead to a BC break for the next major release of Twig? #3951

Open

GromNaN added 16 commits April 5, 2024 16:34

Allows registration of filter/function/test with an attribute

c8c2f57

Undocument detected options

4d9ae3f

Test repeated attributes

22aeb4f

Use attributes in standalone classes and add flatten promote options …

3f84792

…as attribute properties

Accept a class name to leverage runtime lazy-loading

d441d0a

Fix CS

153e028

Track changes in runtime extensions

7b88138

Fix types

76b2830

Add AsTwigExtension attribute and make function name required

200c0ba

Update phpdoc

29f3b3e

Move attributes to Twig\Attribute namespace

5dc5e40

Add pattern filter test case

365d102

Remove name unicity constraint, add phpdoc, refactor time computation

2fd1210

Add docs

4f9e0f8

Mark attributes as final

43bec2f

Rename ModificationAwareInterface

36476c3

GromNaN force-pushed the attribute branch from 5bfb738 to d517ef1 Compare April 5, 2024 14:35

GromNaN commented Apr 5, 2024

View reviewed changes

Make AsTwigExtension extension first party instead of relying on the …

57cb0f4

…extension to be registered

GromNaN force-pushed the attribute branch from d517ef1 to 57cb0f4 Compare April 5, 2024 14:47

stof reviewed Apr 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create attributes `AsTwigFilter`, `AsTwigFunction` and `AsTwigTest` to ease extension development #3916

Create attributes `AsTwigFilter`, `AsTwigFunction` and `AsTwigTest` to ease extension development #3916

GromNaN commented Nov 26, 2023 •

edited

GromNaN commented Nov 26, 2023

stof Nov 27, 2023

GromNaN Nov 30, 2023

GromNaN Nov 30, 2023

GromNaN Nov 30, 2023

GromNaN Dec 1, 2023

GromNaN Dec 8, 2023 •

edited

nicolas-grekas Dec 26, 2023

GromNaN Dec 26, 2023

GromNaN Apr 5, 2024

smnandre Dec 2, 2023

GromNaN Dec 2, 2023

stof Dec 15, 2023

GromNaN Dec 15, 2023 •

edited

GromNaN Apr 5, 2024

GromNaN left a comment

GromNaN Dec 10, 2023

GromNaN commented Jan 3, 2024

nicolas-grekas commented Jan 3, 2024

kbond commented Jan 3, 2024

GromNaN left a comment •

edited

GromNaN Apr 5, 2024

stof Apr 5, 2024

stof Apr 5, 2024

stof Apr 5, 2024

stof Apr 5, 2024

stof Apr 5, 2024

stof Apr 5, 2024

stof Apr 5, 2024

	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);
	}

	public function isTemplateFresh(string $name, int $time): bool
	{
	return $this->extensionSet->getLastModified() <= $time && $this->getLoader()->isFresh($name, $time);
	}


		.. note::

		The ``\Twig\Extension\AttributeExtension`` can be added only once to an environment.

Create attributes AsTwigFilter, AsTwigFunction and AsTwigTest to ease extension development #3916

Are you sure you want to change the base?

Create attributes AsTwigFilter, AsTwigFunction and AsTwigTest to ease extension development #3916

Conversation

GromNaN commented Nov 26, 2023 • edited

GromNaN commented Nov 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GromNaN Dec 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GromNaN Dec 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GromNaN left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GromNaN commented Jan 3, 2024

nicolas-grekas commented Jan 3, 2024

kbond commented Jan 3, 2024

GromNaN left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Create attributes `AsTwigFilter`, `AsTwigFunction` and `AsTwigTest` to ease extension development #3916

Create attributes `AsTwigFilter`, `AsTwigFunction` and `AsTwigTest` to ease extension development #3916

GromNaN commented Nov 26, 2023 •

edited

GromNaN Dec 8, 2023 •

edited

GromNaN Dec 15, 2023 •

edited

GromNaN left a comment •

edited