jsonmapper icon indicating copy to clipboard operation
jsonmapper copied to clipboard

Support shorter ?Type syntax for nullable types

Open RobinvanderVliet opened this issue 1 year ago • 3 comments

I used the following syntax in my PHP class: /** @var ?int */ Which gave me the following error: Class '?int' not found I changed it to the following, which does work: /** @var int|null */

It would be nice if JsonMapper supported both syntaxes. PhpStorm also supports this syntax. This merge request adds support for the shorter syntax.

RobinvanderVliet avatar Apr 08 '24 17:04 RobinvanderVliet

This change obviously needs test cases.

However I am not entirely convinced this change should be merged. First of all there is a documented method how nullable types should be used, and that is type|null in PHPDoc, not ?type, as this aligns with the really old legacy PHPDocs long before any serious typing was introduced into PHP itself, as it reproduces what other languages did. PHP 7.1 added the ?type ability, and PHP 8.0 added generic union types where type|null would only be one of many combinations. So in this regard, PHP clearly moved into the type|null direction, even though I do not expect them to phase out the ?type syntax any time soon.

Having said that, this PR is about adding syntactic sugar for parsing PHPDoc annotations. Any enforced type declarations are already supported, i.e. you could use either public Class|null $foo; or public ?Class $foo instead of relying on non-enforcing comment lines.

The suspected number of users that would benefit from this change might be quite low.

SvenRtbg avatar Apr 08 '24 21:04 SvenRtbg

This change obviously needs test cases.

I've added some test cases! Thank you for the suggestion. :+1:

However I am not entirely convinced this change should be merged. First of all there is a documented method how nullable types should be used, and that is type|null in PHPDoc, not ?type, as this aligns with the really old legacy PHPDocs long before any serious typing was introduced into PHP itself, as it reproduces what other languages did. PHP 7.1 added the ?type ability, and PHP 8.0 added generic union types where type|null would only be one of many combinations. So in this regard, PHP clearly moved into the type|null direction, even though I do not expect them to phase out the ?type syntax any time soon.

I've searched for @var ? in all GitHub repositories, and I have found 30.8k results for PHP, so this syntax is clearly used and quite common.

Having said that, this PR is about adding syntactic sugar for parsing PHPDoc annotations. Any enforced type declarations are already supported, i.e. you could use either public Class|null $foo; or public ?Class $foo instead of relying on non-enforcing comment lines.

This is only possible on the newest PHP versions, not everyone is using those already.

The suspected number of users that would benefit from this change might be quite low.

I've created this pull request because I was personally impacted. I am sure there are more people impacted.

RobinvanderVliet avatar Apr 09 '24 17:04 RobinvanderVliet

I see your argument as being valid. ?type syntax is to be used for explicit types in PHP 7.x, which this library supports, and indeed it feels a bit weird why the same syntax isn't supported as @var docblock (even though technically these two are completely different, the type declaration is parsed using Reflection based on PHP's syntax parsing, the docblock still is a string).

I wonder if the nullability would behave differently for the two more complex things: ?array and ?SomeObject, or even ?SomeObject[] (array where content can be null or SomeObject) and ?SomeObject[]|null (same as before, but instead of the array it can also be null - as absurd as that may sound). This library has some inconsistent checks for null values that are technically not allowed (see #233) that I like to wipe out, hence I am closely watching anything related to NULL getting changed.

SvenRtbg avatar Apr 13 '24 10:04 SvenRtbg

I resolved the conflicts and renamed the test methods a bit to make them clearer.

Thank you for the patch!

cweiske avatar May 17 '24 16:05 cweiske

Released with v4.5.0.

cweiske avatar Sep 08 '24 10:09 cweiske