Add parameter-dependent psalm return type hint to temporal accessors #1700
Conversation
Please refer to the Psalm documentation for conditional types[^0] for more information. Although the docs specify that template parameters are required, the reference-a-variable shorthand for templates also work, see the [example]. Usually this project seems to prefer `@phpstan-`-prefixes, but this construct is currently not available to PHPstan[^1]. [^0]: https://psalm.dev/docs/annotating_code/type_syntax/conditional_types/ [^1]: phpstan/phpstan#3853 [example]: https://psalm.dev/r/11cdedd7b3
| @@ -894,6 +894,8 @@ public function addTemporalAccessorComment(&$script, Column $column) | |||
| * @return string|{$dateTimeClass}{$orNull} Formatted date/time value as string or $dateTimeClass object (if format is NULL), NULL if column is NULL" . ($handleMysqlDate ? ', and 0 if column value is ' . $mysqlInvalidDateString : '') . " | |||
| * | |||
| * @throws PropelException - if unable to parse/validate the date/time value. | |||
| * | |||
| * @psalm-return (\$format is null ? {$dateTimeClass}{$orNull} : string{$orNull}) | |||
There was a problem hiding this comment.
The linked doc says conditional types only work with template parameters, which would need more setup, but it says here that this is a short hand syntax which works too, so I guess it is ok.
Since this only affects generated classes, I don't think it can cause problems in Propel anyway. Not sure what happens, if someone uses an outdated version of Psalm, where the feature is not available. But if they manage to update their Propel , they can update their Psalm, too.
Propel itself requires Psalm 4.2.1, where conditional types are available (which doesn't even matter according to above). I wouldn't agree to using third-party extensions in user code, but since Propel uses Psalm itself, I guess this is a mild recommendation from the team (which I cannot speak for at all).
But unless I am misunderstanding something (am I?), this looks fine.
There was a problem hiding this comment.
Hey @mringler, somewhere else you wrote:
When updating my project, I realized that in your other psalm PR, where you added
@psalm-return ($format is null ? DateTime : string)to DateTime getters, I did not realize that these function also returnnull. You might want to fix that.
Correct type is probably
@psalm-return null|($format is null ? DateTime : string)
As you see, the returned type is {$dateTimeClass}{$orNull} or string{$orNull}, so if the field you're referencing is nullable, the null typehint should already be there!
There was a problem hiding this comment.
Oh, you are right. The columns I saw were all required. Should have checked again. Sorry!
Please refer to the Psalm documentation for conditional types1 for
more information. Although the docs specify that template parameters are
required, the reference-a-variable shorthand for templates also work,
see the [example].
Usually this project seems to prefer
@phpstan--prefixes, but thisconstruct is currently not available to PHPstan2.
Footnotes
https://psalm.dev/docs/annotating_code/type_syntax/conditional_types/ ↩
https://github.com/phpstan/phpstan/issues/3853
[example]: https://psalm.dev/r/11cdedd7b3 ↩