-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Determine which types to support for types
tag
#4256
Comments
It's also related to the work done here: https://github.com/twigphp/Twig/pull/4252/files#diff-6fd6db196be65abd4dcc8132c2bc70a2e0b232c4a5732f29ea26d0a09676ff74R79-R90 |
the PhpStorm Symfony integration now supports Basically most projects needs a class type hint with array. Thats what is now supported here for know:
I am also not sure about the type value itself. imho should be as open as possible but guidlines would be helpful.
|
Can we start small with all the types from https://github.com/twigphp/Twig/pull/4362/files some more precise ones like the above example? |
I'm working on a First off, I've come around to preferring the higher-level Twig types However, I really being able to tell the distinction between lists (e.g., 💡 What if we support:
One can use any of these for iterable objects, depending on what the actual keys are. As for more complicated types, I don't think union types are sensible for Twig. What's a template designer supposed to do with Surely, though, we need at least support for classes and iterables of classes. Therefore, I'd propose:
|
I don't think we should restrict the types in a strict way. If the backend code passes an array containing multiple kind of values, the analysis with twigstan will be much more useful if we describe a union type than if we fallback to
|
Can you elaborate? Do you mean Twig itself should remain completely agnostic and the community should come to an understanding? Or just that Twig should never
Which one are you referring to? 😊 @ruudk's project, or mine (which was formerly called TwigStan as well)? I'm trying to keep TwigQI simple and in line with @Haehnchen's PHPStorm plugin. That probably means not supporting the full monty PHPStan/PSalm syntax.
Well, the documentation now distinguishes between "iterable (mapping)" and "iterable (sequence)". The question is how do we make that distinction in Your thoughts?
Thanks for pointing that out. I've updated my previous comment to avoid confusion. |
Adding support for nullable types is very important (to me, at least). There are two ways:
I favor the 2nd option because the first leads naturally to full union support (e.g., There is a third option as well, of course: agnosticism. |
@drjayvee Twig itself has already made the choice of being agnostic of the types being valid or invalid (Twig itself does not validate the content of the string at all), which is what would allow twigstan to leverage the full syntax of phpstan for instance
Given that PHPStorm supports union types in its type system, are you sure the PHPStorm plugin does not support them in its current
when talking about twigstan, I'm talking about the project that still has this name today.
To me, places expecting only a sequence should use Btw, using And note that |
Unfortunately, my proposal re:
It looks to me like we need to decide whether to "break" compatibility with PHP. Consider @fabpot's vision:
I fully agree. Twig is for front end developers, who might not be PHP devs at all. While I see the value of using PHPStan's enormous capabilities, I think types at least should be kept simple. (Validating logic is another matter, and here, PHPStan might still have a role to play!) One last thing is whether to support I'd love to hear your opinion and plans, @Haehnchen. |
@stof thanks for your detailed response. I'll reply tomorrow! |
@drjayvee |
I fully agree on this principle. However, I'd personally like if the Twig documentation included a basic set of types to establish a community convention. I can only speak for our (AlisQI's) team, but our Twig templates are mostly written and maintained by developers who are fluent in TypeScript, not PHP. Therefore, I actually don't want to use PHPstan's complicated type system, nor will I try to implement checks in TwigQI. My vision is for templates to use simple types. Instead of array shapes, union/intersection types, etc, use a class (e.g., By the way, I'm not arguing Twig's types, whatever they may be, should therefore be closer to TypeScript. But I do like Twig having its own (simple) types as opposed to relying on PHP's. Fabian agrees (see quote further up). Now, I do understand the appeal of TwigStan, don't get me wrong! However, it looks to me like we're at a crossroads, and have to decide between:
The first option is obviously the most flexible, but it looks like it may split the community's extensions and IDE integration(s). That would be a a bit of a bummer. Are the 2nd and 3rd really in conflict? Let's explore, in the hope of finding a subset that makes us all happy. Let me think out loud here. TwigStan already needs to map Twig syntax and semantics (e.g., macro arguments are always optional) to PHP code that PHPStan can inspect. Transpiling But what about So if Twig "officially supported"
Sure, that would be still be possible. Fortunately, So it looks like this should fly, right? @ruudk, I'd love to hear your thoughts on all this.
The plugin simply uses the first type. In other words: it does not support unions. |
Twig has 3 types for those: iterable, sequences and mappings. That's not about having either iterable on one hand or sequences and mappings on the other hand.
The funny thing about that statement is that Typescript actually supports types that are way more complex that what phpstan supports. Note that Fabien never said he wants to forbid using more complex types in the
it actually does (that's what it uses when using a PHP native
And that's my point. We have a bunch of places requiring a sequence, not any type of iterable. Documenting those as |
I meant to distinguish between sequence (a subset of iterable where we don't really care about keys) on the one hand and mapping/iterable (where we do) on the other hand. Obviously, we could use PHPStan's types and semantics. I'm merely trying to come up with a simpler subset that matches the Twig documentation, with maximum PHPStan compatibility.
I'm literally saying Twig's types don't have to be like TypeScript. (I'm not a huge fan myself.) I'm trying to explain why it makes sense for Twig not to use PHP's types, and especially not PHPStan's.
I'm not claiming he did, plus I'm explicitly saying that I'm not in favor of that either. This is not a productive conversation, so I'm leaving it at that. |
I have been documenting variable types at the top of Twig files for years using a simple comment. In that time I have refined the types I use into the following most used:
These follow the built-in PHP types mostly and then Python types (based on Jinja2) to distinguish between iterables.
|
Thanks for your feedback, @willrowe I agree with most of your points!
I also thought the difference was really important, but I came around somewhat. I now strongly favor a simple and PHP-agnostic type system, even if that means not every type can be documented in detail. I now think wrapping dictionaries/mappings in However, I have a feeling the community won't come to a consensus on this topic.
TwigQI supports nullable types using a Would you agree this is sufficient? |
|
Following discussions in #4165, we want to determine which types (if any), Twig wants to suggest or even mandate extensions support.
There seem to be four options:
string
,bool
, etcstringable
,iterable
The text was updated successfully, but these errors were encountered: