Generics annotations support #1298

tanerkay · 2022-01-17T08:53:35Z

Summary

Add option use_generics_annotations for collection type hints

Before:

 * @property-read \Illuminate\Database\Eloquent\Collection|Simple[] $regularHasMany

After:

 * @property-read \Illuminate\Database\Eloquent\Collection<int, Simple> $regularHasMany

I feel this is is a useful transition given what has evolved since #942 (comment). Namely:

the upcoming release of Laravel 9 this month
most PhpStorm users should be on version >= 2021.3 (addition of generics support and support for future Laravel collections).

Note: for some reason, specific support for @property and @property-read didn't land until PhpStorm version 2022.3. Testing now to make sure things work

Type of change

New feature (non-breaking change which adds functionality)

Checklist

Existing tests have been adapted and/or new tests have been added
Add a CHANGELOG.md entry
Update the README.md
Code style has been fixed via composer fix-style

mfn · 2022-01-17T09:53:09Z

config/ide-helper.php

+    | e.g. `Collection<User>` instead of `Collection|User[]`.
+    |
+    */
+    'use_generics_annotations' => false,


I have not yet looked into detail of this PR and will do it later, but if it works really good I'm also tempted to suggesting to make it even default to true. But that was just a quick showerthought :)

That would be my preference too, but I thought this was a decision best left to you. I originally had the fallback value as version_compare($this->laravel['version'], '9', '>=') (i.e. true if Laravel version is at least version 9), but was in two minds about it. In the end, I removed it. Open to suggestions here.

Another option is to set the default to true, then do the inverse check for Laravel version, thereby falling back to false if the config option is not set and the Laravel version is less than 9.

LastDragon-ru · 2022-03-15T16:13:09Z

phpstan with checkGenericClassInNonGenericObjectType: true wants

@property-read \Illuminate\Database\Eloquent\Collection<int, Simple> $regularHasMany

Legion112 · 2022-03-15T17:35:19Z

How the progress is going on this one?
Will it support previous versions of laravel?

Legion112 · 2022-03-15T17:37:45Z

In pull request, I don't see generation for \Illuminate\Database\Eloquent\Collection method such get, add, etc

Legion112 · 2022-03-15T17:40:31Z

Each method here should have and template annotation to define return time base on template type

tanerkay · 2022-03-30T10:35:32Z

@Legion112

Will it support previous versions of laravel?

Yes

In pull request, I don't see generation for \Illuminate\Database\Eloquent\Collection method such get, add, etc

The idea is that the collection class is type hinted, so the IDE can infer those class methods. Right?

Each method here should have and template annotation to define return time base on template type

That didn't make sense to me. Rephrase?

Legion112 · 2022-03-30T15:51:59Z

@tanerkay

The idea is that the collection class is type hinted, so the IDE can infer those class methods. Right?

This is wrong. You need to have a defined template that ide would be able to know what will be return then you call a method on the collection.

Look here https://psalm.dev/docs/annotating_code/templated_annotations/

Without such a template, IDE cannot determine the type of any method of the collection It would alwasy think mixed.

/**
 * @template T
 */
class MyContainer {
  /** @var T */
  private $value;

  /** @param T $value */
  public function __construct($value) {
    $this->value = $value;
  }

  /** @return T */
  public function getValue() {
    return $this->value;
  }
}

tanerkay · 2022-04-23T16:08:04Z

@Legion112

This is wrong. You need to have a defined template that ide would be able to know what will be return then you call a method on the collection.

Sorry, still don't understand. The templates are defined in the Laravel collection classes: \Illuminate\Database\Eloquent\Collection and \Illuminate\Support\Collection. Are you looking at Laravel 8 code?

tanerkay · 2022-04-23T16:10:23Z

Haven't really been following this closely or thinking too much about it as PhpStorm still has incomplete support for this, see https://youtrack.jetbrains.com/issue/WI-64963/@property-does-not-support-@template-declaration

Legion112 · 2022-04-25T19:27:13Z

I didn't have much sleep lately 😪 . I will try my best to read the last link to understand the problem.
@tanerkay

barryvdh · 2022-06-20T08:21:02Z

So what is the status of this, is this 100% done or not?

mfn · 2022-06-20T13:45:55Z

It definitely slipped my radar to go through the PR, whoopsie. Hope to do it in the next days!

bbprojectnet · 2022-08-06T11:17:43Z

Any news on this? ;)

tanerkay · 2022-08-08T16:32:18Z

Any news on this? ;)

Still not supported by PhpStorm at time of writing (version 2022.2)

Feel free to +1 the linked issue on Jetbrains' issue tracker or comment, as it would be awesome if this were implemented sooner rather than later: https://youtrack.jetbrains.com/issue/WI-64963/@property-does-not-support-@template-declaration

shahruslan · 2022-09-07T07:24:36Z

Is this PR waiting for a bug in PhpStorm to be fixed?

tanerkay · 2022-09-07T09:12:35Z

Is this PR waiting for a bug in PhpStorm to be fixed?

yes, I've lost track of the actual issue in their bug tracker, as they've marked them as duplicates of unrelated issues

tanerkay · 2022-09-07T10:39:46Z

Apparently fixed in upcoming PhpStorm version, 2022.3 🎉

jnoordsij · 2022-12-06T10:12:10Z

Any update on this? Would be great to have!

I opened larastan/larastan#1470 in an effort to create a workaround on their side, but enabling this package to allow for adding generics would be much better.

grEvenX · 2023-01-25T15:11:00Z

@barryvdh Support for this has been out for a while in PhpStorm now and this PR should be ready for review.
I've tested it on our codebase and it works perfectly 👍

I guess a decision should be made if this should now in 2023 be set as the default, or if it's needed to set use_generics_annotations to true in the ide-helper config file.

# Conflicts: # CHANGELOG.md

tanerkay · 2023-01-25T16:07:28Z

I guess a decision should be made if this should now in 2023 be set as the default, or if it's needed to set use_generics_annotations to true in the ide-helper config file.

I think it makes sense to, I just have the following concerns:

it ties this package more with PhpStorm. might be undesired for developers who don't use it. Nevertheless, they have the option to change the config value.
users using Laravel 8 and running vendor:publish for the first time will have the option set to true*

I guess the benefits from having the collection type hinted using generics outweighs the limited benefit from the alternative syntax.

In any case, I've changed the default in this PR to true. Can leave the decision to @barryvdh . The change is in 28775bd in case it is decided to revert it.

*An alternative middle of the road option is to block the use of generics annotations if the Laravel version is not version 9 or greater, e.g.

diff --git a/src/Console/ModelsCommand.php b/src/Console/ModelsCommand.php
--- a/src/Console/ModelsCommand.php	(revision 28775bd73779318b764f91ca28b2e7f3ae312478)
+++ b/src/Console/ModelsCommand.php	(date 1674662581752)
@@ -1091,7 +1091,7 @@
     protected function getCollectionTypeHint(string $collectionClassNameInModel, string $relatedModel): string
     {
         $useGenericsSyntax = $this->laravel['config']->get('ide-helper.use_generics_annotations', false);
-        if ($useGenericsSyntax) {
+        if ($useGenericsSyntax && version_compare($this->laravel['version'], '9', '>=')) {
             return $collectionClassNameInModel . '<int, ' . $relatedModel . '>';
         } else {
             return $collectionClassNameInModel . '|' . $relatedModel . '[]';

mfn

it ties this package more with PhpStorm. might be undesired for developers who don't use it. Nevertheless, they have the option to change the config value.

I would not kid ~~ourselves~~ myself over this: PhpStorm is on the forefront and their endeavors are part of why the community thrives, too.

Some tests are failing, but I'd willing to approve this, makes sense (now that's 2023).

tanerkay · 2023-01-27T08:14:22Z

I would not kid ~~ourselves~~ myself over this: PhpStorm is on the forefront and their endeavors are part of why the community thrives, too.

Fair call, I wanted to bring up the topic but acknowledge the significance of PhpStorm at the same time.

Some tests are failing, but I'd willing to approve this, makes sense (now that's 2023).

Awesome. I'll sort out tests today :)

…` by default

tanerkay · 2023-01-27T18:00:23Z

tests updated

src/Console/ModelsCommand.php

mfn

Is there a test now where it does not use the generics annotation? I see the test setting it to false, but I failed to spot a diff in the PR where this is the case (or I'm blind).

tanerkay · 2023-01-28T12:07:22Z

Is there a test now where it does not use the generics annotation?

Yes, in the namespace Barryvdh\LaravelIdeHelper\Tests\Console\ModelsCommand\GenericsSyntaxDisabled

https://github.com/barryvdh/laravel-ide-helper/pull/1298/files#diff-7f2f9319ecfc1e0b601eeb87d07266d79901707f53c161979a89918ce2e4d6b4

tanerkay · 2023-01-28T12:09:52Z

I know that creates a regression for users who just updated the package and not the config, but this is easily fixed if they care/it poses a problem for them and, in the end, this is a change for the better so we want to promote this.

That would be my preference too, but was wary that it's a regression as you mentioned. If the idea is to promote it (which I'm completely in agreeance), then let's change it :)

…nfig Co-authored-by: Markus Podar <[email protected]>

mfn

👍🏼

@barryvdh after merging and releasing this, existing annotations will be changed to use the new generics annotation but it an be disabled in the config

Sergiobop · 2023-02-07T15:04:17Z

I don't know if it is just me, but after this is released, every time the model command is ran, some generic annotations are being duplicated even if they are already in the phpdoc.

grEvenX · 2023-02-07T17:20:55Z

@Sergiobop I see I get the same experience when running:

php artisan ide-helper:models "App\MyModel" --write --reset
php artisan ide-helper:models "App\MyModel" --write

Should probably open a new issue about it.

* option to use generics syntax, default to yes in laravel 9 * rename `use_generics_syntax` to `use_generics_annotations` * update readme and changelog * remove laravel 9 default override * include key type in generics annotations * readme rewording * set `use_generics_annotations` to `true` by default * update tests to reflect `use_generics_annotations` being set to `true` by default * default `use_generics_annotations` setting to true when not set in config Co-authored-by: Markus Podar <[email protected]> --------- Co-authored-by: Markus Podar <[email protected]>

tanerkay added 5 commits January 17, 2022 09:28

option to use generics syntax, default to yes in laravel 9

968af20

rename use_generics_syntax to use_generics_annotations

86abe34

update readme and changelog

add6f63

remove laravel 9 default override

04cc0ae

include key type in generics annotations

5cb9ded

mfn reviewed Jan 17, 2022

View reviewed changes

mfn self-requested a review January 17, 2022 09:53

Legion112 mentioned this pull request Mar 15, 2022

Collection template support psalm/psalm-plugin-laravel#225

Closed

bbprojectnet mentioned this pull request Jan 25, 2023

Attribute with collection with generic annotation return type generates incorrect PHPDoc #1382

Closed

tanerkay added 2 commits January 25, 2023 16:30

Merge branch 'master' into generics-syntax

213c0d9

# Conflicts: # CHANGELOG.md

readme rewording

3e9fe04

tanerkay force-pushed the generics-syntax branch from 490b8a1 to 3e9fe04 Compare January 25, 2023 15:55

set use_generics_annotations to true by default

28775bd

mfn reviewed Jan 27, 2023

View reviewed changes

update tests to reflect use_generics_annotations being set to `true…

59c16b8

…` by default

mfn reviewed Jan 28, 2023

View reviewed changes

src/Console/ModelsCommand.php Outdated Show resolved Hide resolved

mfn reviewed Jan 28, 2023

View reviewed changes

default use_generics_annotations setting to true when not set in co…

adf9d06

…nfig Co-authored-by: Markus Podar <[email protected]>

mfn approved these changes Jan 28, 2023

View reviewed changes

mfn assigned barryvdh Jan 28, 2023

barryvdh merged commit f0e4cf5 into barryvdh:master Jan 28, 2023

This was referenced Jan 31, 2023

Option for generating Collection|ElementType[] types as Collection<int, ElementType> instead #942

Closed

Feature request: allow transformation of generic array-syntax in IDE helper docblocks larastan/larastan#1470

Closed

Sergiobop mentioned this pull request Feb 8, 2023

Property issues with 2.13.0 #1419

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generics annotations support #1298

Generics annotations support #1298

tanerkay commented Jan 17, 2022 •

edited

Loading

mfn Jan 17, 2022

tanerkay Jan 17, 2022

tanerkay Jan 17, 2022

LastDragon-ru commented Mar 15, 2022

Legion112 commented Mar 15, 2022

Legion112 commented Mar 15, 2022

Legion112 commented Mar 15, 2022

tanerkay commented Mar 30, 2022

Legion112 commented Mar 30, 2022 •

edited

Loading

tanerkay commented Apr 23, 2022

tanerkay commented Apr 23, 2022

Legion112 commented Apr 25, 2022

barryvdh commented Jun 20, 2022

mfn commented Jun 20, 2022

bbprojectnet commented Aug 6, 2022

tanerkay commented Aug 8, 2022 •

edited

Loading

shahruslan commented Sep 7, 2022

tanerkay commented Sep 7, 2022 •

edited

Loading

tanerkay commented Sep 7, 2022 •

edited

Loading

jnoordsij commented Dec 6, 2022

grEvenX commented Jan 25, 2023

tanerkay commented Jan 25, 2023

mfn left a comment

tanerkay commented Jan 27, 2023

tanerkay commented Jan 27, 2023

mfn left a comment

tanerkay commented Jan 28, 2023

tanerkay commented Jan 28, 2023

mfn left a comment

Sergiobop commented Feb 7, 2023 •

edited

Loading

grEvenX commented Feb 7, 2023

Generics annotations support #1298

Generics annotations support #1298

Conversation

tanerkay commented Jan 17, 2022 • edited Loading

Summary

Type of change

Checklist

mfn Jan 17, 2022

Choose a reason for hiding this comment

tanerkay Jan 17, 2022

Choose a reason for hiding this comment

tanerkay Jan 17, 2022

Choose a reason for hiding this comment

LastDragon-ru commented Mar 15, 2022

Legion112 commented Mar 15, 2022

Legion112 commented Mar 15, 2022

Legion112 commented Mar 15, 2022

tanerkay commented Mar 30, 2022

Legion112 commented Mar 30, 2022 • edited Loading

tanerkay commented Apr 23, 2022

tanerkay commented Apr 23, 2022

Legion112 commented Apr 25, 2022

barryvdh commented Jun 20, 2022

mfn commented Jun 20, 2022

bbprojectnet commented Aug 6, 2022

tanerkay commented Aug 8, 2022 • edited Loading

shahruslan commented Sep 7, 2022

tanerkay commented Sep 7, 2022 • edited Loading

tanerkay commented Sep 7, 2022 • edited Loading

jnoordsij commented Dec 6, 2022

grEvenX commented Jan 25, 2023

tanerkay commented Jan 25, 2023

mfn left a comment

Choose a reason for hiding this comment

tanerkay commented Jan 27, 2023

tanerkay commented Jan 27, 2023

mfn left a comment

Choose a reason for hiding this comment

tanerkay commented Jan 28, 2023

tanerkay commented Jan 28, 2023

mfn left a comment

Choose a reason for hiding this comment

Sergiobop commented Feb 7, 2023 • edited Loading

grEvenX commented Feb 7, 2023

tanerkay commented Jan 17, 2022 •

edited

Loading

Legion112 commented Mar 30, 2022 •

edited

Loading

tanerkay commented Aug 8, 2022 •

edited

Loading

tanerkay commented Sep 7, 2022 •

edited

Loading

tanerkay commented Sep 7, 2022 •

edited

Loading

Sergiobop commented Feb 7, 2023 •

edited

Loading