Skip to content
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

Model when use Carbon, auto generate docblock not pass by phpstan. #1004

Closed
bolechen opened this issue Aug 3, 2020 · 16 comments
Closed

Model when use Carbon, auto generate docblock not pass by phpstan. #1004

bolechen opened this issue Aug 3, 2020 · 16 comments

Comments

@bolechen
Copy link

bolechen commented Aug 3, 2020

this is my User Model diff after run php artisan ide-helper:models -WR

image

it will remove \Carbon\Carbon in docblock, would lead to result by phpstan: (https://github.com/nunomaduro/larastan)

 ------ -----------------------------------------------------------------------
  Line   User.php
 ------ -----------------------------------------------------------------------
  385    Property App\User::$vip_end (Illuminate\Support\Carbon|null) does not
         accept Carbon\Carbon.
  388    Property App\User::$vip_end (Illuminate\Support\Carbon|null) does not
         accept Carbon\Carbon.
  394    Property App\User::$vip_end (Illuminate\Support\Carbon|null) does not
         accept Carbon\Carbon.
 ------ -----------------------------------------------------------------------

Is there something I'm doing wrong? Please advise, thank you very much!

@bolechen
Copy link
Author

bolechen commented Aug 3, 2020

my php code sample below

$this->vip_end = $this->vip_end->addDays($vip_days);

@mfn
Copy link
Collaborator

mfn commented Aug 3, 2020

I wondered if my PR #859 is responsible but I'm not sure because it looks like when I made the PR, there was already no Carbon\Carbon referenced in the code I changed 🤔

@mfn
Copy link
Collaborator

mfn commented Aug 20, 2020

How did you end up with null|\Carbon\Carbon|Illuminate\Support\Carbon in the first place (from your screenshot)?

I've never seen it generated that way from ide-helper?

Even the null|\Illuminate\Support\Carbon is strange, because null is always appended…

Or did you simply have another tool reformatting that before you pasted it? 🤔

@mfn
Copy link
Collaborator

mfn commented Aug 20, 2020

null|\Carbon\Carbon|Illuminate\Support\Carbon

What I mean is: ide-helper never generated the union \Carbon\Carbon|Illuminate\Support\Carbon.

So it never was a union

@bolechen
Copy link
Author

Hello @mfn, \Carbon\Carbon is added manual by my self. Because if not add is, will not pass by phpstan. But when I run php artisan ide-helper:models -WR again, is always restore to null|\Illuminate\Support\Carbon.

I don't know if I made myself clear. Sorry for my poor english.

@mfn
Copy link
Collaborator

mfn commented Aug 22, 2020

@kylekatarnls phpstsan complains that when a model $model->some_carbon is set with a result of a Carbon method call, that it doesn't accept Carbon\Carbon:

Property App\User::$vip_end (Illuminate\Support\Carbon|null) does not
         accept Carbon\Carbon.

Thing is, it seems a mismatch of (php)doc vs. what really happens?

From Carbon.php:

 * @method        Carbon         addDays(int $value = 1)                                                              Add days (the $value count passed in) to the instance (using date interval).

But the resulting object is Illuminate\Support\Carbon:

$ ./artisan tinker
Psy Shell v0.10.4 (PHP 7.4.9 — cli) by Justin Hileman
>>> $c = new \Illuminate\Support\Carbon;
=> Illuminate\Support\Carbon @1598133330 {#4248
     date: 2020-08-22 21:55:30.588467 UTC (+00:00),
   }
>>> $c->addDays(2);
=> Illuminate\Support\Carbon @1598306130 {#4248
     date: 2020-08-24 21:55:30.588467 UTC (+00:00),
   }

But I see via briannesbitt/Carbon#1914 this was quite deliberate…

So it's either "IDE" or "static analyzers" at the moment?

@kylekatarnls
Copy link
Contributor

Maybe the helper should rather generate \Carbon\CarbonInterface which is the wider requirement while Carbon, CarbonImmutable, Illuminate\Carbon are possible specific implementations.

@mfn
Copy link
Collaborator

mfn commented Aug 23, 2020

Interesting idea.

However I'm not seeing myself pursuing this, as if will cause additional confusion when people see the result is \Illuminate\Support\Carbon.

Was my hunch right that?

So it's either "IDE" or "static analyzers" at the moment?

@kylekatarnls
Copy link
Contributor

Actually I would like to PHPDoc the Carbon method with some @return static but when set as a @method annotation, the static keyword is detected as a static-method marker instead of the return type. So I don't know how I could achieve the right typing and properly detected by the IDE too.

@kylekatarnls
Copy link
Contributor

A other possible fix is to copy-paste the \Carbon\Carbon PHPDoc over the \Illuminate\Support\Carbon PHPDoc. This would not be a good thing for the Laravel repository to hard-code this duplicate right in their source, so it should rather be some script to run to override it.

Maybe I can propose a fix for ide-helper:generate to generate the needed extra PHPDoc.

kylekatarnls added a commit to kylekatarnls/laravel-ide-helper that referenced this issue Aug 23, 2020
kylekatarnls added a commit to kylekatarnls/laravel-ide-helper that referenced this issue Aug 23, 2020
@kylekatarnls
Copy link
Contributor

I also could change Carbon with $this in the PHPDoc which make the typing of sub-classes correct but this would only be correct for Carbon\Carbon, not for Carbon\CarbonImmutable for which the same methods return new instances.

@mfn
Copy link
Collaborator

mfn commented Sep 5, 2020

but when set as a @method annotation, the static keyword is detected as a static-method marker instead of the return type. So I don't know how I could achieve the right typing and properly detected by the IDE too.

This may related to https://youtrack.jetbrains.com/issue/WI-54071

I also could change Carbon with $this in the PHPDoc

You in fact did, didn't you? => briannesbitt/Carbon@0a41ea7

Because I was about to test this "in code" in an up2date project and I couldn't reproduce it 😅 when I realized vendor/nesbot/carbon/src/Carbon/Carbon.php had $this everywhere already.

but this would only be correct for Carbon\Carbon, not for Carbon\CarbonImmutable

I guess it's fine, because:

  • \Illuminate\Support\Carbon does not extend Immutable
  • when using your Laravel customization Date::use(CarbonImmutable::class), ide-helper already detects and does generate the CarbonImmutable typehint which works correctly

The way I see it, with the release nestbot/carbon 2.39.0 everything is solved and we can close this issue and don't need #1014 ?

@kylekatarnls
Copy link
Contributor

Yes it should be 👌

@mfn
Copy link
Collaborator

mfn commented Sep 6, 2020

Solved it is then 😄

@mfn mfn closed this as completed Sep 6, 2020
@dennis-koster
Copy link

I am sorry, but I fail to see how this has been solved. I am still experiencing this exact issue, where the the generated docblocks state Illuminate\Support\Carbon|null and not Carbon\Carbon. I am on nesbot/carbon 2.40.1 and barryvdh/laravel-ide-helper 2.8.1.

@Sergiobop
Copy link

This is not solved @kylekatarnls @mfn. You can check larastan/larastan#1067. The packages are not compatible right now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants