twigphp / Twig

Twig, the flexible, fast, and secure template language for PHP
https://twig.symfony.com/
BSD 3-Clause "New" or "Revised" License
8.16k stars 1.25k forks source link

Add tag to declare variable types #4165

Closed drjayvee closed 1 month ago

drjayvee commented 3 months ago

See #4003 and #4009 for some context. In short, since the PHP language itself as well as its community are moving to (optional) stricter typing, it makes sense for Twig to support this as well by adding a types tag.

@fabpot said he'd like to bootstrap a spec. This is the first draft. Everything is up for debate, obviously. In addition, I've put in italics parts of the spec that I have doubts about myself.

Given the discussions below (thanks all for contributing), I think the spec is now crystallizing. Consider it a beta.

Syntax

The types tag contains a mapping of variable name to constant type string (no expression or interpolation):

{% types {variable: 'Type'} %}

The reason for quoting the type is that several characters (e.g., \) are currently not supported by the Lexer. It also allows analyzer implementations to freely specify the syntax.

The variable name must not be quoted. This is to ensure that only valid names can be used.

Types

The syntax and content of the types are not part of this specification. This provides complete freedom to extensions that inspect and analyze the tags. It also keeps the implementation in Twig very simple and robust.

(In practice, it's likely that implementations will rely on widely-used specifications like PHPDoc / PHPStan types.)

Examples

Here are two examples:

{% types {foo: 'bool'} %}

{% types {
  bar: "list<int>",
  baz: '?\Name\Space\ClassName'
} %}

Behavior

This specification does require any behavior other than validating the syntax for the tag itself. The tag does not lead to generated PHP code.

Usage

This section describes possible implementations to demonstrate the utility of this addition.

It is up to extensions developed by the community to implement any of this.

Development time

The types can be useful during development by enabling IDEs to auto-complete code, issue warnings, and more.

This is already implemented by the Symfony Support plugin for PHPStorm when using comments to declare variable types (e.g., {# @var id int #}).

These tags also provide precious documentation about the variables expected in a template's context.

Compile time

During compilation, a Twig extension can:

  1. Check the type and throw an appropriate Exception if it is not valid.
  2. For objects, check whether properties and more methods exist, and throw an appropriate Exception if there is none.
  3. Add run-time checks (see below).

Run time

During run time, behavior may depend on strict_variables environment option:

Possible extensions

JoshuaBehrens commented 3 months ago

Would you mind linking

Use property access or method call for objects instead of twig_get_attribute() to improve performance

with https://github.com/twigphp/Twig/pull/3928 so we can see, when this is unblocked by this issue? :)

ruudk commented 3 months ago

If I may suggest: instead of deciding the possible var type values, why not accept any valid PHPDoc type. Together with PHPStan's PHPDoc Parser we can parse them into PhpDocNode's that then can be transformed to PHPStan types. From there, it can be used to build compile time assertions. Doing it from scratch feels like reinventing the wheel 😅

We could still only accept a subset from the possible types, and let others improve it over time via PR's.

ericmorand commented 3 months ago

Give it enough time and Twig will end up being PHTML, the format it was supposed to replace.

drjayvee commented 3 months ago

If I may suggest: instead of deciding the possible var type values, why not accept any valid PHPDoc type

Thanks, that's a good suggestion. I was mindful of not reinventing the wheel and hoped that we could re-use parts of PHPStan to parse and evaluate the types. Should we perhaps go all the way and support any PHPStan type? I'll add a sentence in italics to that section for now.

I'll add support for nullable types, which I forgot. But the more gets added, the more sense it makes to just go all the way

willrowe commented 2 months ago

I'm not sure if this would be the best place to implement this, but a lot of times I have a flag in a template to allow certain markup to be turned on or off, but need to retain previous functionality for other templates that are already including it. I usually have to add a set tag to check if the variable exists and set it to a default value if not. I already include a comment at the top of the template denoting what variables are used in the template, but it would be nice to be able to provide a default value for any optional variables in the same place that I denote the type, replacing the need for a bunch of set tags.

ruudk commented 2 months ago
{% var variable Type %}

I think that the order should be the same as with inline PHPDocs:

{% var Type variable %}
willrowe commented 2 months ago

I agree with @ruudk on the order. It would also match how class properties and argument types are defined in PHP.

Also, going along with that and clarifying what I was proposing in https://github.com/twigphp/Twig/issues/4165#issuecomment-2265852945, this is what it would look like to specify a default value:

{% var Type variable = 'default_value' %}
{% var bool flag = false %}
drjayvee commented 2 months ago

I think that the order should be the same as with inline PHPDocs:

Agreed. I've updated the OP. (While I prefer the original order, we should follow existing conventions.)

We could also consider an optional description. phpDocumentor supports this, but PHPStan doesn't seem to.

{% var bool foo to foo or not to foo %}

Then again, this makes the tag less readable, plus there's nothing Twig itself would do with this. The alternative is to just add a comment (before or afterwards) for humans:

{% var bool foo %} {# to foo or not to foo #}
ruudk commented 2 months ago

We could also consider an optional description. phpDocumentor supports this, but PHPStan doesn't seem to.

What's the point of providing a description there? The reason why PHPStan doesn't support it at that position is that the @var is already inside a comment. So you can add a comment above the @var to explain the why.

Same can be done with Twig:

{# I need to explain this somehow #}
{% var 'bool' foo %}

PS: The Type should be a string (ConstantStringExpression).

The variable could be a NameExpression or ConstantStringExpression.

drjayvee commented 2 months ago

it would be nice to be able to provide a default value for any optional variables in the same place that I denote the type, replacing the need for a bunch of set tags.

I personally actually disagree on principle: declaring the type and any value should not be mixed. This does bring up two interesting questions, though.

First, should we add undefined as a possible type? Or should template developers ensure variables are always set (possibly using null)? Since var is all about stricter typing, I would prefer the latter.

Second, what do we do about variables defined using set? Should users add var afterwards? Or should Twig try to auto-detect the type? That would be very difficult (or outright infeasible) since set can use an expression (using literals and even function calls).

drjayvee commented 2 months ago

What's the point of providing a description there? The reason why PHPStan doesn't support it at that position is that the @var is already inside a comment. So you can add a comment above the @var to explain the why.

Exactly. I was just thinking out loud.

PS: The Type should be a string (ConstantStringExpression).

The variable could be a NameExpression or ConstantStringExpression.

Why should the type be quoted? It's not an expression (although I confess not to know the definition of "expression"). It also looks really weird to me and is inconsistent with PHPStan.

Do you think the implementation could be tricky if the type isn't quoted?

Update: the implementation does indeed run into issues. Therefore, I've amended the spec to quote types.

willrowe commented 2 months ago

I personally actually disagree on principle: declaring the type and any value should not be mixed.

@drjayvee can you explain more?

drjayvee commented 2 months ago

I personally actually disagree on principle: declaring the type and any value should not be mixed.

@drjayvee can you explain more?

@willrowe, absolutely! I should have done that straight away.

But first off, I want to point out that this is just my personal opinion. I'd love to hear what other people think.

One reason I dislike your proposal is that Twig already has three ways of setting values:

  1. injecting them into the template's context (in PHP)
  2. using the set tag
  3. defining a macro argument

I feel like adding a fourth way leads to bloat. (Or worse, conflicts.)

Macros arguments already have support for default values. (In fact, they're never even undefined, since they get an implicit default of null. I'm actually proposing to make this distinction clear in the AST: #4010)

The set tag already supports your use case just fine:

{% set foo = foo ?? 'bar' %}

{# or, in case null must be kept as-is, more strictly: #}
{% set foo = foo is defined ? foo : 'bar' %}

The second problem is that it feels like mixing concerns. I'm envisioning the var tag to be analogous to PHPStan's @var annotation, not PHP's native type declarations (e.g., for class properties and function arguments). Another way of looking at var is that it only affects template code (and therefore provide value) after the tag. You can read it like "from here on out, foo is guaranteed to be of this type".

However, I'm not super convinced by my own argument here, since both PHP and Typescript (as just two examples) do let you declare types and default values in one go in many places (e.g., function default values and class properties).

A third problem is that var is currently envisioned to have no effect on compiled templates. I think this is a sensible design goal is prevent compatibility issues and make the change more palatable to maintainers and users. If var does support default values, it will have to affect compilation output.

I hope I explained clearly. I'm sincerely curious as to you what you think now you know my reasoning.

ericmorand commented 2 months ago

@drjayvee

{% var ?\Name\Space\ClassName baz %}

What does it mean? What is \Name\Space\ClassName and how is such a type supposed to be declared?

willrowe commented 2 months ago

@drjayvee those are all great points and illustrate how you see this feature being utilized.

Where we differ is that I think about this as more of a dockblock/argument list at the top of the template file (similar to the extends tag) defining what variables should be available in the context of the template file. As if the file is a function and the var tags are its arguments. That's why it makes sense to me to have support for default value, since arguments can have default values.

Keeping the above in mind:

One reason I dislike your proposal is that Twig already has three ways of setting values:

  1. injecting them into the template's context (in PHP)

I see this as augmenting the template context specifically, so if a var is missing from the template context it will be initialized with the default value, if one has been specified.

  1. using the set tag

Since I am thinking of this as a single definition at the top, it would be different than set. The set tag can be used multiple times throughout the template to change a variable value at runtime.

  1. defining a macro argument

I think macros are separate from what I am talking about.

I feel like adding a fourth way leads to bloat.

I don't see it as an additional way to set variable values within the template if it were to only affect the context as it comes into the template. It would be overridden by any values provided by the user.

Macros arguments already have support for default values. (In fact, they're never even undefined, since they get an implicit default of null.

I'm not talking about macros, just template file context.

The set tag already supports your use case just fine

As I said in my original comment, I already do this, but am looking for a way to easily combine the documentation of variables within a template file and a default value.

The second problem is that it feels like mixing concerns. I'm envisioning the var tag to be analogous to PHPStan's @var annotation, not PHP's native type declarations (e.g., for class properties and function arguments). Another way of looking at var is that it only affects template code (and therefore provide value) after the tag. You can read it like "from here on out, foo is guaranteed to be of this type".

This is where we are thinking about the tag differently. It seems like adding the ability specify a type to the existing set tag would accomplish what you are describing here.

A third problem is that var is currently envisioned to have no effect on compiled templates. I think this is a sensible design goal is prevent compatibility issues and make the change more palatable to maintainers and users. If var does support default values, it will have to affect compilation output.

I think it would be much more powerful to have the variable type information available after compiling, since that would allow introspection of what variables a template file expects to be passed as context, among other things. Compiling the types could also possibly allow Twig to leverage built-in PHP type checks so that checks don't necessarily need to be run manually at runtime, but would rather be implicitly checked.

All this being said, I think it comes down to whether it would be used as a way to define variable types once for the whole file, or inline.

drjayvee commented 2 months ago

@drjayvee

{% var ?\Name\Space\ClassName baz %}

What does it mean? What is \Name\Space\ClassName and how is such a type supposed to be declared?

This tag declares that variable baz must be an object of class with FQN \Name\Space\ClassName or null (due to the ? prefix).

The PHP equivalent for a function argument would be

function bar(?\Name\Space\ClassName $baz) {}
drjayvee commented 2 months ago

@willrowe thanks for your detailed response. It does indeed appear we have slightly different use cases in mind.

Documenting / specifying the template's context variables similar to function arguments is an explicit goal of this proposal. I still maintain that setting default values would be supported just fine with a combination of set and var (to declare type).

The current proposal is more generic: you can use var in macros and anywhere inside a template.

Consider the following example:

{% if user.isLoggedIn() %}
  {% set userName = user.fullName %} {# Class property is ?string but guaranteed to be string if user is logged in #}
{% else %}
  {% set userName = 'Anonymous' %}
{% endif %}

{% var string userName %}

The var tag on the last line serves as documentation and safety net. It guarantees that, userName is always string, even though user.fullName itself is nullable.

That being said, I very much would like the community to weigh in here.

Speaking of: @fabpot, you mentioned you wanted to involve some interested parties, no? I'd also like to know your thoughts on the proposal so far.

ericmorand commented 2 months ago

@drjayvee thanks. How do you declare to the twig compiler that this class exist? Said differently, at compile-time, how is the compiler expected to check that this type exist?

drjayvee commented 2 months ago

@drjayvee thanks. How do you declare to the twig compiler that this class exist? Said differently, at compile-time, how is the compiler expected to check that this type exist?

Simply by using class_exists(), right? I guess the only potential issue is that autoloading must be enabled. But who doesn't have that.

Do you see any other problem? Or am I overlooking something here?

ericmorand commented 2 months ago

@drjayvee you are explaining how the php implementation would work. But how would the other implementations work? More generally, from a specification point of view, how would \Foo\Bar be resolved?

ericmorand commented 2 months ago

@drjayvee also it is fundamental to establish how type checking would work? Let's say the a variable x of type \Foo\Bar is used this way:

{{ x.y.z }}

What would be the algorithm that compilers are supposed to implement to check that x.y.z is a valid member access?

As I said earlier in this thread in an ironic manner, I'm concerned that twig would become a proper typed programming language, which means that it would also come with a complexity in both specification and implementation that the TwigPHP team may not be ready to face. I recommend that you have a look at how typed languages are specified and how their compilers are implemented before making a decision about this feature: this is very very complex topic.

willrowe commented 2 months ago

I still maintain that setting default values would be supported just fine with a combination of set and var (to declare type).

Documenting / specifying the template's context variables similar to function arguments is an explicit goal of this proposal. The current proposal is more generic: you can use var in macros and anywhere inside a template. The var tag on the last line serves as documentation and safety net. It guarantees that, userName is always string, even though user.fullName itself is nullable.

I think there are two different things we are talking about here in regards to a template's context variables:

  1. I was talking about the context variables that are passed in when rendering or including a template, so they are like "arguments".
  2. You are talking about the more general context variables which encompass both those that are initially passed (that I was talking about) and the context variables as they may change during runtime via set, etc.

I think it is important to distinguish between the two. Using your example to expand on what I mentioned in my last comment, I think it would be more clear to augment the existing set tag since you are creating a new context variable:

{% set string userName = (
    user.isLoggedIn()
        ?
    user.fullName
        :
    'Anonymous'
) %}

If that variable could be passed to the template instead, this could be done:

{% var string userName = 'Anonymous' %}
drjayvee commented 2 months ago

@drjayvee you are explaining how the php implementation would work. But how would the other implementations work? More generally, from a specification point of view, how would \Foo\Bar be resolved?

At compile time, Twig checks whether the type is valid. For classes, that would simply be done using class_exists(). For scalars, that's even simpler. Then again, we might let PHPStan do these checks.

For runtime (i.e., in the compiled template's PHP code), a check is added to assert the type.

For example:

{% var int number %}
{% var \User user %}
{{ user.name }} has {{ number }}

Would compile to something like the following, if (and only if) strict_variables is enabled:

if (!is_int($context['int'])) {
  trigger_error('Invalid type for variable "int"'); // or throw an Exception
}
if (! $context['user'] instanceof \User) {
  trigger_error('Invalid type for variable "user"'); // idem dito
}

Do you see any problem here so far? Or significant limitations which render this whole feature moot?

also it is fundamental to establish how type checking would work? Let's say the a variable x of type \Foo\Bar is used this way: {{ x.y.z }} What would be the algorithm that compilers are supposed to implement to check that x.y.z is a valid member access?

That's an interesting question. I'd hope to leverage PHPStan to do those chained checks. At the very least, though, we could check whether y is a valid property method of class x (if x's type is declared to be a class at all). That should be simple enough, and will already provide significant value.

However, even if properties/methods aren't checked at all, there's still value.

Consider this example:

{% var \Foo\Bar foo %}
{% set baz = foo.bar.getBaz() %}
{% var string baz %}

At compile time, we can ensure that the type is valid. The var tag also serves as (standardized) type documentation, as discussed in the OP.

At runtime, we can trigger warnings/errors or throw Exceptions if foo is not an instance of \Foo\Bar, and if baz is not a string. That alone will add a significant safety net to developers.

The goal of var is not to match PHPStan's level 9. Just adding what I'm proposing will add significant value. var is also entirely optional, so it doesn't have to support complicated use cases - certainly not in the first iteration.

drjayvee commented 2 months ago

@willrowe

I think there are two different things we are talking about here in regards to a template's context variables:

Oh, do I understand correctly that you'd want the default value in var to apply exclusively to injected variables, not ones created using set and macro arguments?

In that case, I think this would get too complicated, both in the implementation, as well as for users to understand.

I think it would be more clear to augment the existing set tag since you are creating a new context variable

I guess I wouldn't be opposed to adding optional types to set.

However, set can define (the documentation reads "assigned") multiple values, and be used "to 'capture' chunks of text". This might make it more complicated to implement and support.

{% set string foo, int bar = 'foo', 1337 %} {# should work fine #}

{% set string baz %} {# pointless, as captured chunks will always be guaranteed to be string (right?) #}
    Baz
{% endset %}

Moreover, set declarations have limited scope in loops (see documentation), so the implementation would need to take that into account.

I guess that could all work. But you'd need typing support in set and default values in var, right?

It's an interesting idea for sure.

My gut still says it's better separate those concerns. But that's just one opinion.

ericmorand commented 2 months ago

@drjayvee, what about other compiler implementations like twig.js or Twing? What would be the logic that they need to implement?

willrowe commented 2 months ago

@ericmorand what do you currently do for constant?

ericmorand commented 2 months ago

@willrowe since there was no specification, we made an arbitrary call:

https://twing.nightlycommit.com/specifics.html#constant

But constant is an easy case to handle since the value is picked from the data passed to the context.

Types are another story and need a proper specification to be supported by non-PHP implementation.

willrowe commented 2 months ago

@ericmorand do you have a specific question about how it should function when a template is rendered?

You keep asking about the specification and how it should be implemented outside of PHP, but it doesn't seem like that is something to be decided here in this issue or even in this project. You may need to decide for yourself how you are going to implement it in your own project so it matches TwigPHP in a way that is consistent with how you have implemented all the other features.

drjayvee commented 2 months ago

@ericmorand

@drjayvee, what about other compiler implementations like twig.js or Twing? What would be the logic that they need to implement?

I honestly wasn't aware of these! It's interesting that there are already two.

Not to be disrespectful, but I wouldn't want to limit the capability or complicate the spec to cater to implementations other than TwigPHP, especially ones in other languages. (Given the difficulties described here, this might already be the stance of the Twig maintainers.)

To answer your question directly: the easiest path is to simply ignore var. Alternatively, Twing (and any other implementation) could chose their own specification and implementation of the var tag.

Twing's documentation already reads:

Twig specification being intimately bound to the TwigPHP implementation, some parts of it don’t make sense outside of the context of a PHP runtime. Below is the list of those parts that Twing interprets differently from its TwigPHP counterpart.

var would be very similar to constant: either chose a different specification and implementation, or just don't support it.

willrowe commented 2 months ago

@drjayvee

Oh, do I understand correctly that you'd want the default value in var to apply exclusively to injected variables, not ones created using set and macro arguments?

Yes, that is correct.

In that case, I think this would get too complicated, both in the implementation, as well as for users to understand.

I do not think the implementation would be complicated, but I do agree that it would need to be done very carefully so that it is not ambiguous to the user what is happening. Naming a tag that does this var would probably be confusing.

I guess I wouldn't be opposed to adding optional types to set. However, set can define (the documentation reads "assigned") multiple values, and be used "to 'capture' chunks of text". This might make it more complicated to implement and support. Moreover, set declarations have limited scope in loops (see documentation), so the implementation would need to take that into account.

I think all of this could be addressed without it becoming insurmountably complex. Using a type on a set block would just be disallowed. I think augmenting set would be very clean from a template perspective and would be familiar and easy to understand for template authors. It might get confusing to have two separate tags (set and var) that seem similar, but don't do the same thing. If you always need to set the value of the variable anyways, combining it into one tag makes a lot of sense to me.

I guess that could all work. But you'd need typing support in set and default values in var, right? It's an interesting idea for sure. My gut still says it's better separate those concerns. But that's just one opinion.

I absolutely agree that these should be separate concerns. What I suggested as far as default values would need to be thought through very carefully and implemented as a completely separate tag that is well named so there is no confusion. I think what you have suggested here would be valuable to add. As I said above, in order to limit any confusion and make it as simple as possible, I think it should be added to the set tag instead of a new, separate tag.

fabpot commented 2 months ago

I've been thinking about this proposal for a bit now. I think I'm ready to share my thoughts now.

My take might sound a bit harsh, but this is mainly because I care a lot about Twig philosophy (more on that in a minute).

"In short, since the PHP language itself as well as its community are moving to stricter typing, it makes sense for Twig to support this as well by adding a var tag."

I strongly disagree with this first sentence. Twig is a templating language. Twig should not become PHP. And I certainly don't want Twig to become stricter (the reasons are explained below). But, I want to consider adding some support to ease the work of static analyzers, which is how this discussion started.

Outside of static analyzers, we might optionally use that information to optimize performance (think getAttribute() compile time resolution) via a NodeVisitor that would not be part of Twig core (can be part of extra though).

And of course, the usage of the new var tag must be optional.

So, let me share with you a bit of Twig's philosophy: The person writing Twig templates is not necessarily a developer (think templates updated by end users in a CMS backend). They might have a limited understanding of PHP/JavaScript/... and that's fine. So, they should not need to understand the difference between a method call, an object property, or an item of an array. That's the power of the . Twig operator. The . operator also allows to use a pure array in tests/staging/... and objects/arrays/... in production. I should also mention that the data passed to templates should be "prepared". Most data massaging should happen before they are sent to the template (that's the reason why, for instance, mutating an array is complicated in Twig (via the merge filter).

With that in mind, I don't think we will support types in a template outside the var tag (so no support in set for instance). I also think that the information from the var tag won't be used at compile time or runtime in core.

Starting small would be my advice here. I would focus on the var tag in the context of providing extra information for static analyzers, everything else can come later on.

willrowe commented 2 months ago

@fabpot so would this be more like front matter metadata to start with?

smnandre commented 2 months ago

I guess "PhpDoc" may be a good analogy or... even better for recent PHP versions: PHP attributes.

It allows to "mark" templates but would have no runtime effect.

And this would open the door for tools (static analysis beeing one) to plug-in and leverage this data.

fabpot commented 2 months ago

@fabpot so would this be more like front matter metadata to start with?

No, having a proper var tag is fine for me.

fabpot commented 2 months ago

I guess "PhpDoc" may be a good analogy or... even better for recent PHP versions: PHP attributes.

It allows to "mark" templates but would have no runtime effect.

And this would open the door for tools (static analysis beeing one) to plug-in and leverage this data.

That's indeed a good analogy.

The first idea (and the one I think implemented by VS Code) is to use comments. And this proposal is about having the proper infrastructure at Twig level so that everybody can share the same (like PHP attributes).

drjayvee commented 2 months ago

@fabpot thanks for your thoughtful response. I agree completely that typing should always be optional, and that Twig must not become PHP (let alone at max PHPStan).

I've had a chat with @ruudk today, and he showed the progress he made to leverage PHPStan to do inspection on compiled templates. I think there's great promise in that approach.

What is missing in Twig to enable both this or my humbler approach is a way to specify types. The PHPDoc route of using comments was rejected (see #4009) to keep them out of the AST. Therefore, a proper tag is the only way to go.

As I've pointed out before, the community is already using @var comments to declare types. Therefore, a proper var tag is an easy migration.

By the way, I'm working on adding the var tag to the TwigStan extension. However, I do think it should be an official part of Twig so that the community adopts it.

On a different note, using FQNs in var breaks the Lexer due to the leading \. This easily (though naively) solved by adding that character to Lexer::PUNCTUATION, but adding more complicated types (e.g., array{foobars: array<\Foo\Bar>}) will probably cause more breakage.

We could consider extending the Lexer, I'd very much prefer to keep the required changes to Twig core to a minimum.

Therefore, I propose types must always be quoted. I'll amend the OP.

fabpot commented 2 months ago

@drjayvee It looks like we are very much aligned :) Let's add a @var tag to make it the official "solution" (like PHP added attributes).

drjayvee commented 2 months ago

Awesome! I'll start working on an implementation and documentation.

I do think Twig should at least validate types to prevent mistakes. We can leave other inspections to extensions. Would you agree, @fabpot?

@ruudk's prototype had a way to specify multiple types in one go, which we could consider here as well:

{% var {
  foo: 'string',
  bar: '\Foo\Bar'
} %}

This would make the spec and implementation more complicated, plus the order of variable name and type is flipped in this notation. Therefore, I'd prefer to keep it simple and only support what's currently written in the OP.

fabpot commented 2 months ago

@ruudk's prototype had a way to specify multiple types in one go, which we could consider here as well:

{% var {
  foo: 'string',
  bar: '\Foo\Bar'
} %}

This would make the spec and implementation more complicated, plus the order of variable name and type is flipped in this notation. Therefore, I'd prefer to keep it simple and only support what's currently written in the OP.

The implementation is going to be trivial as this is just an expression. So, a call toParseExpression::parseMappingExpression() to enforce a mapping will do all the work.

drjayvee commented 2 months ago

So let's (try to) support formats:

{% var 'string' foo %}
{# and #}
{% var {
  bar: 'string',
} %}
fabpot commented 2 months ago

So let's (try to) support formats:

{% var 'string' foo %}
{# and #}
{% var {
  bar: 'string',
} %}

Why not support just 1 format?

drjayvee commented 2 months ago

Well, I don't love this format for single variables.

But then, most of the times (start of template and start of macro), multiple variables will need to be typed.

So sure, let's do the multi-type mapping expression exclusively then. I'll adapt the OP now and start working on an implementation soon.

stof commented 2 months ago

but then, is it an issue to have multiple {% var %} tags at the beginning of the template if we consider the single-variable format more readable ?

stof commented 2 months ago

Note that parseMappingExpression would allow complex expressions in values of the mapping, which we probably don't want to support in the {% var %} tag, so I don't think we can reuse it directly.

fabpot commented 2 months ago

Note that parseMappingExpression would allow complex expressions in values of the mapping, which we probably don't want to support in the {% var %} tag, so I don't think we can reuse it directly.

Correct. Let's copy/paste and simplify.

willrowe commented 2 months ago

I know everyone seems pretty locked into the name being var since it is taken directly from PHPDoc, but from the perspective of whoever is writing the templates (who may not be a PHP developer), could it be seen as ambiguous? For example, if I'm a JavaScript developer and I look at a Twig template and see the set and var tags, I'm going to think var is creating a variable and would not necessarily think set would do that.

I just want to make sure this is taken into account, I don't necessarily feel strongly that this would be a big issue, but might as well make sure the name is clear from the outset.

fabpot commented 2 months ago

I know everyone seems pretty locked into the name being var since it is taken directly from PHPDoc, but from the perspective of whoever is writing the templates (who may not be a PHP developer), could it be seen as ambiguous? For example, if I'm a JavaScript developer and I look at a Twig template and see the set and var tags, I'm going to think var is creating a variable and would not necessarily think set would do that.

I just want to make sure this is taken into account, I don't necessarily feel strongly that this would be a big issue, but might as well make sure the name is clear from the outset.

That's a very good point. Let's think about possible alternatives (that does not block the implementation anyway).

ruudk commented 2 months ago

I'm also working on a static analyzer for Twig. Next to supporting @var I came up with {% type_hint name 'string' %} as a way to hint type information to the analyzer. It's completely ignored on compilation/runtime.

stof commented 2 months ago

I would rather use {% type %} than {% type_hint %}

willrowe commented 2 months ago

I think {% type %} is very clear, no ambiguity there. If it will be the multi-variable syntax then plural types may be even better.

{% types {
  bar: 'string',
  foo: 'bool',
} %}
{% types {
  bar: 'string',
} %}