Consider using built-in types from phpstan/phpstan or vimeo/psalm directly?

[Ocramius]

The spec about how to declare annotated types changes constantly: this library seems to infer types with its own mechanisms, which is possibly why issues like #1 and #5 arise.

In order to reduce such issues, it may be a good idea to directly build on top of phpstan/phpstan or vimeo/psalm for their internal type definitions (value objects, practically):

• https://github.com/phpstan/phpstan-src/tree/447cd102d946a2d9883ac82793195275f54296a9/src/Type
• https://github.com/vimeo/psalm/tree/f1d47cc662500589e200a978e3af85488a5236cf/src/Psalm/Type

By leveraging these, and having a "default" case that throws ("I don't know this type yet"), it would potentially be clear what is and isn't supported yet, at the cost of introducing a runtime dependency.

[shulard]

Hello !

Good point, but this means having a direct dependency on one of those tools. Do you think it's relevant to depend on it "just" to get their internal type definition ?

[Ocramius]

Do you think it's relevant to depend on it "just" to get their internal type definition ?

It could help to know what upstream understands as types - the direct coupling would at least give an idea of version ranges supported.

The fact that we have an additional dependency is obviously problematic all the times, but right now you depend on the DSL of such packages, without stating it explicitly: vimeo/psalm:^5 and phpstan/phpstan:^2 may completely break the DSL in a new major release 🤔

[shulard]

Yeah absolutely, it's better to have an explicit dependency than depending on the syntax only. Thank you for the answer !

[Ocramius]

I wonder if require-dev would also suffice, but 🤷

Re-building the parsing was certainly an interesting experiment, but it is very wasteful, and will indeed lead to constant tiny incompatibilities over time.

Other libraries worth mentioning here are https://github.com/phpDocumentor/ReflectionDocBlock + https://github.com/phpDocumentor/TypeResolver , which do half of what you need:

https://github.com/phpDocumentor/ReflectionDocBlock/blob/622548b623e81ca6d78b721c5e029f4ce664f170/src/DocBlock/Tags/Param.php#L35

Only problem is that @phpDocumentor only supports PSR-5, so advanced types from vimeo/psalm (which usually has the most advanced stuff) are not necessarily working all the time.

[shulard]

This is an important point for that kind of library. Maybe, depending the context having a whole annotation set supported is not relevant.

For example, if I already depend on phpstan maybe I want to import the phpstan rules because my is code will be directly compliant. Same for psalm if my codebase is already using it. I'm not sure about introducing psalm specific syntax on a codebase that doesn't explicitely rely on it.

Why not having a kind of plugin that allows having an external resolver ? Only the relevant annotation will be interpreted. Of course this can lead to more maintenance work however all the codebases will not require the same type coverage. This will also help introducing project specific resolver that will be plugged directly in the resolution process.

Maybe it's too complicated but I'm sure that kind of validation tool can really change the way we work and there are two distinct parts here : object builder from input data, input data validation.

[Ocramius]

I'm not sure about introducing psalm specific syntax on a codebase that doesn't explicitely rely on it.

Dunno, I mostly use psalm's syntax because it allows the most precise types, which is what matters most.

Why not having a kind of plugin that allows having an external resolver ?

As in configuring the mapper with a specific annotation parser?

object builder from input data, input data validation.

IMO data validation is something for a later stage: what attracted me to this library is the ability to get type-safe instances of my objects, starting from mixed: the validation part is something that always cuts deep in the business domain of an application, and it is a massive can of worms to tackle.

[shulard]

Ah maybe I'm not clear enough when I talk about validation. I just mean that we need to validate that the input data can be used to build type safe instance. Not really the business domain rules that will be controlled after.

This library makes me think of the « Missing Middleware » that can allow type safe queries input before they enter the application logic ^^.

As in configuring the mapper with a specific annotation parser?

Regarding the external resolver, yes that's exactly what I mean ^^

Dunno, I mostly use psalm's syntax because it allows the most precise types, which is what matters most.

For sure having the most precise type is the goal here but if a team is familiar with some syntax, if that kind of library force them to learn new ones (or specific details) that can limit adoption. Of course it's also possible to only rely on basic behaviour annotation then move forward on more complex ones (generics for example).

[Ocramius]

For sure having the most precise type is the goal here but if a team is familiar with some syntax, if that kind of library force them to learn new ones (or specific details) that can limit adoption.

FWIW, phpstan/phpstan and vimeo/psalm try to stay aligned with each other, with some lag. The syntax is pretty much the same, when it comes to types.

I don't think that's the bottleneck for adoption: the first time people see a new type, they usually learn it in a matter of minutes (or less), and start adopting it.

For example, one type I use a lot these days is positive-int|0: takes a few seconds to understand what it means, and then a few minutes to grasp the value of it. From there on, adoption is not a question of learning, but rather deciding to be less/more strict about type constraints :)

[azjezz]

I would like to also note that PHPStan organization has a doc-parser that can help a lot with this: https://github.com/phpstan/phpdoc-parser, and it should also help avoiding having phpstan or psalm as a dependency.

example: https://github.com/phpstan/phpdoc-parser/blob/master/tests/PHPStan/Parser/TypeParserTest.php#L64

[Ocramius]

Absolute ref: https://github.com/phpstan/phpdoc-parser/blob/d639142cf83e91a07b4862217a6f8aa65ee10d08/tests/PHPStan/Parser/TypeParserTest.php#L64

[romm]

Hi, thanks for the discussion and the very interesting inputs on this topic.

First of all, just to allow you to understand why the lib doesn't use PHPStan/Psalm type parsers: well this project has been underground for a (very) long time — actually I just checked and the first commit on the private repo of this lib started three months only after the first commit of phpstan/phpdoc-parser 😅 — at that time I didn't find a good alternative so I started building by own parser.

To be honest, I'm not sure what to think about giving this responsibility to an external library, I understand the pros but there are some things to consider.

The internal type system is really inherent to the whole library, pretty logical because that's what this library is about; changing this would start a massive refactoring of pretty much everything (not impossible, but a lot of work). Considering the time I already put on the lib, I'm not sure I'd be ready (at least now) for such work.

While I guess it is already possible to cache types inhered by phpstan/phpdoc-parser, Valinor is an object mapper/hydrator, meaning performance is an important thing to consider — or, should I say, micro optimizations are more important for this kind of libraries than for static analyzers IMHO. Having full control of the whole system allowed me to have really good performance results, mostly because of a nice cache layer that fits perfectly the needs of the library. I'm not saying this is impossible to have the same results with an external dependency, but research work would be needed first.

@Ocramius: By leveraging these, and having a "default" case that throws ("I don't know this type yet"), it would potentially be clear what is and isn't supported yet, at the cost of introducing a runtime dependency.

I agree, and that is actually the current behavior: if a type is not handled it will be clear for the user. Also note that using an unknown type will throw an exception during the mapping only for used nodes that have unresolvable types , see below. It means that only the types from the object properties or from the constructor parameters could be an issue.

Valinor/src/Mapper/Tree/Shell.php

Lines 39 to 41 in ea842e2

	if ($type instanceof UnresolvableType) {
	throw new UnresolvableShellType($type);
	}

It's also still possible to use prefixed tags (@phpstan-param/var/… / @psalm-param/var/…) to make both libraries work alongside. We could even imagine handling @valinor-param/var/… if needed.

@Ocramius: The fact that we have an additional dependency is obviously problematic all the times, but right now you depend on the DSL of such packages, without stating it explicitly: vimeo/psalm:^5 and phpstan/phpstan:^2 may completely break the DSL in a new major release 🤔

I see your point, but let's see the other way around: if Valinor forces applications to be stuck on a specific version range of PHPStan/Psalm, that wouldn't be good either.

Also, even if the entirety of the annotations of these tools are not handled, the actual type parsing is not the main problem, it's rather how the type is used by the system — and mostly how raw data are accepted/casted to a specific type. So relying on an external library would not solve this issue: if a new type appears and is parsed, this library has to learn how to use it anyway.

@shulard: Why not having a kind of plugin that allows having an external resolver ? Only the relevant annotation will be interpreted. Of course this can lead to more maintenance work however all the codebases will not require the same type coverage. This will also help introducing project specific resolver that will be plugged directly in the resolution process.

While this could probably be doable, this would also mean opening this part of the API publicly, which also results in more maintenance burden overall — I think I'd prefer a well done pull request to implement a missing feature; let's see how this goes in the future.

@shulard: For sure having the most precise type is the goal here but if a team is familiar with some syntax, if that kind of library force them to learn new ones (or specific details) that can limit adoption.

I don't plan to implement new syntax standards at all, as stated in the README I rather plan to keep up handling what PHPStan/Psalm handle (more or less), so this should not be a problem.

[Ocramius]

The internal type system is really inherent to the whole library, pretty logical because that's what this library is about; changing this would start a massive refactoring of pretty much everything (not impossible, but a lot of work). Considering the time I already put on the lib, I'm not sure I'd be ready (at least now) for such work.

@romm beware: I've only thrown out the idea, but if you believe your architectural solution is sound, then roll with it.

All these issues are suggestions, and you already did the heavy lifting and legwork to bring this library out in the open, so don't feel obliged to follow any direction which just comes from some random dude's suggestion coming from the internet :D

I think the topic has been explored, discussed and understood: whether it is required to follow up on it, is mostly a question of how this will reflect in overall stability (see #1 , #5, #15, #18 ) long-term.

Closing here: the ideas have been seeded, and whether they are valid and/or should be adopted depends on how much work it is to maintain these abstractions, vs rewriting (for now, keeping them seems to be the correct choice)

[romm]

All these issues are suggestions, and you already did the heavy lifting and legwork to bring this library out in the open, so don't feel obliged to follow any direction which just comes from some random dude's suggestion coming from the internet :D

Don't get me wrong, I value these inputs a lot, especially coming from people who have proven to be really concerned about OSS overall 🙂 — I don't feel obliged at all but I really appreciate the suggestions!

I keep the idea somewhere in case this would be doable at some point, thanks!