Add support for functions/filters/tests stubs

[fabpot]

Quick PoC for #3655

@javiereguiluz Would that help?

Usage:

$twig->registerFunctionStub('csrf_token', fn (string $tokenId) => '');
$twig->registerFunctionStub('importmap', fn (string|array $entryPoint = 'app', array $attributes = []): string => '');
$twig->registerTestStub('random', fn (string $value) => false);

[javiereguiluz]

I think this could work to solve the linked issue. But, let me add some comments.

Imagine that your bundle template uses importmap() function without knowing if AssetMapper is installed in the user app or not:

(1) If it's available, the correct Twig function is used
(2) If it's NOT available, an exception occurs

The (2) problem is solved by this PR, but there are some sub-cases when defining a stub:

(2.1) You define an empty/simple stub (e.g. return ''). User doesn't see an exception, but the app doesn't work as expected either and this can be confusing;
(2.2) You add some logic to the stub. The problem is duplicating code and e.g. providing unsafe alternatives. E.g. 'csrf_token', fn (string $tokenId) => md5($tokenId));
(2.3) You throw an exception on purpose when using the filter/function/test. E.g. 'importmap', fn (...) => throw new \RuntimeException('... Did you forgot to install symfony/asset-mapper?')

My use-case would be to always use (2.3). This way:

• If the app doesn't have AssetMapper installed, no exception occurs for having importmap() in some Twig templates ...
• ... but if you try to use that feature actively, you'll see an exception telling you that you must install symfony/asset-mapper.

So, if for other folks the use case is always (2.3) or similar, we could do something like Symfony's UnefinedCallableHandler and define the stubs like this (registerFunctionStub(string $functionName, string $nameOfPackageThatDefinesIt)) to always thro an exception when trying to use those:

$twig->registerFunctionStub('csrf_token', 'symfony/security-csrf');
$twig->registerFunctionStub('importmap', 'symfony/asset-mapper');
$twig->registerTestStub('random', '...');

[fabpot]

$twig->registerFunctionStub('csrf_token', 'symfony/security-csrf');
$twig->registerFunctionStub('importmap', 'symfony/asset-mapper');
$twig->registerTestStub('random', '...');

We need the function because we need its signature. But I like this idea. I'm not sure it covers that many use cases though.

[stof]

In its current form, it looks like that this is just syntactic sugar over what undefined callable callbacks can do already (the implementation could correspond to registering an undefined callable callback that return false when the name does not match).
And if we allow that stub to produce a non-throwing behavior, the function signature is not enough. We also need the callable options (otherwise, auto-escaping might be off), making this even closer to the undefined callable callbacks.

I'm not sure we need this shortcut implementation as a separate API in the Environment itself.

[fabpot]

Maybe this is just a recipe in the documentation then?

[smnandre]

For the importmap scenario i wonder if something like this could do the work, without the negative impact

 {% if function('importmap') is defined %}
    {{ importmap('app', ....) }}
 {% endif %}

or maybe

 {% if 'importmap' is function %}
    {{ importmap('app', ....) }}
 {% endif %}

As you said @javiereguiluz, either

• you do need this function => it looks like a requirement problem, you can add a undefined callback like symfony bridge does
• you don't need it => maybe the final app needs it in other places, so hiding the exception / message does not feel like a good idea (as it would impact the whole Environment right?)

As i see it, stubs could be really usefull in functional tests, but i imagine this is not the same need

[fabpot]

For the importmap scenario i wonder if something like this could do the work, without the negative impact

 {% if function('importmap') is defined %}
    {{ importmap('app', ....) }}
 {% endif %}

or maybe

 {% if 'importmap' is function %}
    {{ importmap('app', ....) }}
 {% endif %}

No, it's not possible as the function must be defined at compilation time.

[fabpot]

I've pushed Javier's suggestion: $twig->registerFunctionStub('importmap', 'symfony/asset-mapper');

Is it worth it? Would a recipe in the docs be enough?

[javiereguiluz]

I like this, and I think this would work in most cases. Thanks. But, looking at some real example, I think we might need something more.

E.g. I have this in the Twig template:

<form class="d-none" method="post" id="delete-form">
    <input type="hidden" name="token" value="{{ ea_csrf_token('ea-delete') }}" />
</form>

Thanks to this PR, I'd change ea_csrf_token('ea-delete') by just csrf_token('ea-delete') and folks not having symfony/security-csrf will see this error message when loading a backend page:

Cannot use the "csrf_token" function because the "symfony/security-csrf" composer package is not installed. Try running "composer require symfony/security-csrf".

But, this could be confusing because the CSRF token is like an indirect feature not related to what the user is doing at this moment.

So, maybe the stub should allow to define an optional error message to display to the user?

public function registerFunctionStub(string $name, string $package, ?string $customErrorMessage): void

And I'd use it like this:

registerFunctionStub(
    'csrf_token',
    'symfony/security-csrf',
    'When using the delete action on any backend page, EasyAdmin generates a CSRF token to protect your application. This uses the "csrf_token()" Twig function provided by the "symfony/security-csrf" package. So, you must install that package in your app by running "composer require symfony/security-csrf"'
);

[javiereguiluz]

Just a random thought. This feature is needed by a small niche of apps like backend bundles that don't have control over the app where they are installed.

So, maybe we could simplify this by creating a try ... catch ... endtry tag. I know this feels like programming 🙏 but this already exists in Jinja as a plugin: https://pypi.org/project/jinja-try-catch/

So, my original Twig template:

<form class="d-none" method="post" id="delete-form">
    <input type="hidden" name="token" value="{{ ea_csrf_token('ea-delete') }}" />
</form>

Would become:

<form class="d-none" method="post" id="delete-form">
    {% try %}
        <input type="hidden" name="token" value="{{ csrf_token('ea-delete') }}" />
    {% catch %}
        When using the delete action on any backend page, EasyAdmin generates a CSRF token to protect your application.
        This uses the "csrf_token()" Twig function provided by the "symfony/security-csrf" package. So, you must install
        that package in your app by running "composer require symfony/security-csrf"
    {% endtry %}
</form>

[smnandre]

I may not have understood all this correctly, so please correct me if i missread something :)

DX (before)

I took an open project (no security, no easyadmin) and added {{ csrf_token() }} on a page.

An exception has been thrown during the rendering of a template ("CSRF tokens can only be generated if a CsrfTokenManagerInterface is injected in FormRenderer::__construct(). Try running "composer require symfony/security-csrf".").

Here, i understand the situation, the "problem" and the solution.

DX (after) ?

With this stub registration, if easyadmin is installed in the project, i'll read this:

When using the delete action on any backend page, EasyAdmin generates a CSRF [...]

It would feel very confusing to me, as nothing here is related to what i did 🤷‍♂️

[javiereguiluz]

@smnandre for this csrf_token() example it might be better to just do nothing and let users see the exception (we did this in case some folks didn't use security with the bundle).

But here's a more realistic use-case. We added support for AssetMapper in EA very early because AssetMapper is great. BUT, many people still use WebpackEncore or nothing at all. So, we have to use this in the template:

{% block head_javascript %}
    {# ... #}

    {% block importmap %}
        {{ ea_importmap(assets|map(asset => asset.value)) }}
    {% endblock %}
{% endblock head_javascript %}

We cannot use importmap() directly because not having AssetMapper installed is a common and legit scenario.

The ideal solution is what you proposed earlier:

 {% if function('importmap') is defined %}
    {{ importmap('app', ....) }}
 {% endif %}

or

 {% if 'importmap' is function %}
    {{ importmap('app', ....) }}
 {% endif %}

but apparently is not possible technically speaking.

[smnandre]

Thank you for the example !

I imagine you have something similar for webpack ? So the two of them must be "detected" ?

---

Regardless of the stubs.... i may have something that could help you

In your bundle recipe, do you know you can target / differenciate assetmapper and webpack users ?

Exemple with the StimulusBundle recipe

      {
          "file": "assets/bootstrap.js",
          "content": "import { startStimulusApp } from '@symfony/stimulus-bridge';\n\n// Registers Stimulus controllers from controllers.json and in the controllers/ directory\nexport const app = startStimulusApp(require.context(\n    '@symfony/stimulus-bridge/lazy-controller-loader!./controllers',\n    true,\n    /\\.[jt]sx?$/\n));",
          "position": "top",
          "requires": "symfony/webpack-encore-bundle"
      },
      {
          "file": "assets/bootstrap.js",
          "content": "import { startStimulusApp } from '@symfony/stimulus-bundle';\n\nconst app = startStimulusApp();",
          "position": "top",
          "requires": "symfony/asset-mapper"
      }
                    

This is how the bridge/bundle is installed with two ways, the "good one" being selected wether the packages are installed and the precence of key files :)

So maybe you could do something similar to change/comment a file (ex: a twig path?) or a line of your configuration ?

Not a silver bullet but it's always good to know (if you did not already 😅 )

[fabpot]

As we need to execute the test at compilation time, here are two proposals:

{% if when compiling 'importmap' is function %}
    {{ importmap('app') }}
{% else %}
    importmap does not exist
{% endif %}

I've implemented this one and it works well. Basically, it only executes the branch for which the condition is true, avoiding the branch for which we are using the non-existing function.

or:

## if 'importmap' is function ##
    {{ importmap('app') }}
## else ##
    importmap does not exist
## endif ##

[smnandre]

👏

I love both syntaxes!

Also, this could open the door for nice things .. what are the limits / constraints ?

[javiereguiluz]

The proposed syntaxes look a bit rough to me. Even if we're talking about an advanced feature here, we should keep Twig somewhat friendly to designers.

Another random proposal: introduce function_exists(), filter_exists() and test_exists() functions? (they would be evaluated at compile time too)

{% if function_exists('importmap') %}
    {{ importmap('app', ....) }}
{% endif %}

{% if filter_exists('u') %}
    {{ user.biography|u.truncate(120, '…', false) }}
{% else %}
    {{ user.biography[:120] }}
{% endif %}

{% set is_foo = test_exists('foo') ? (variable is foo) : false %}

And don't throw exceptions or output any messages to users:

• If some filter/function/test is really needed, add it as a dependency in your bundle/app
• If they are optional, then use these functions to use alternative (but fully functional) code when they are not available

[fabpot]

The proposed syntaxes look a bit rough to me. Even if we're talking about an advanced feature here, we should keep Twig somewhat friendly to designers.

Another random proposal: introduce function_exists(), filter_exists() and test_exists() functions? (they would be evaluated at compile time too)

{% if function_exists('importmap') %}
    {{ importmap('app', ....) }}
{% endif %}

{% if filter_exists('u') %}
    {{ user.biography|u.truncate(120, '…', false) }}
{% else %}
    {{ user.biography[:120] }}
{% endif %}

{% set is_foo = test_exists('foo') ? (variable is foo) : false %}

And don't throw exceptions or output any messages to users:

* If some filter/function/test is really needed, add it as a dependency in your bundle/app

* If they are optional, then use these functions to use alternative (but fully functional) code when they are not available

The main "problem" is that the test expression (whatever the syntax) must be evaluated at compile time to avoid parsing the branch that won't work. So, we need a way to signal that to the parser.

[fabpot]

Another syntax (whatever we end up with) is probably the way to go (like a pre-processor).

[javiereguiluz]

I thought the *_exists() functions were "simple" because in my mind, this is how it would work:

(1) Original template code:

{% if function_exists('importmap') %}
    {{ importmap('app', ....) }}
{% endif %}

{% if filter_exists('u') %}
    {{ user.biography|u.truncate(120, '…', false) }}
{% else %}
    {{ user.biography[:120] }}
{% endif %}

{% set is_foo = test_exists('foo') ? (variable is foo) : false %}

(2) A new compiler pass that evaluates the functions at compiling time and replaces them by true or false:

Let's consider that importmap() exists, |u and foo don't exist:

{% if true %}
    {{ importmap('app', ....) }}
{% endif %}

{% if false %}
    {{ user.biography|u.truncate(120, '…', false) }}
{% else %}
    {{ user.biography[:120] }}
{% endif %}

{% set is_foo = false ? (variable is foo) : false %}

About this concern --> "So, we need a way to signal that to the parser." Just using these *_exists() functions would be that signal. In the docs we'd mention that they are evaluated at compile time.

(3) A DCE (dead code elimination) compiler pass simplifies the code like this:

{{ importmap('app', ....) }}

{{ user.biography[:120] }}

{% set is_foo = false %}

[stof]

@javiereguiluz the issue is not the function_exists that needs to be replaced. It is the body of the {% if %} or {% else %} that must be parsed in relaxed mode instead of the normal parsing, so that it does not fail at compile-time for unknown functions (we cannot skip parsing entirely if we reuse the {% if %} and {% else %} tags for that, as that would break for nested tags)