[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
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

Description of relationships #2071

Open
choco-cat opened this issue Oct 16, 2024 · 22 comments
Open

Description of relationships #2071

choco-cat opened this issue Oct 16, 2024 · 22 comments
Labels
bug Something isn't working

Comments

@choco-cat
Copy link
choco-cat commented Oct 16, 2024
  • Larastan Version: 2.9.9
  • Laravel Version: 11.28.0

Description

Hi. How to describe relationships correctly?
To my description

    /**
     * @return BelongsTo<User, Rate>
     */
    public function user(): BelongsTo
    {
        return $this->belongsTo(User::class);
    }

an error occurs

 Method App\Rate\Models\Rate::user() should return Illuminate\Database\Eloquent\Relations\BelongsTo<App\Rate\Models\Rate<App\User\Models\User>> but returns
         Illuminate\Database\Eloquent\Relations\BelongsTo<App\User\Models\User, $this(App\Rate\Models\Rate)>.

Laravel code where the issue was found

    /**
     * @return BelongsTo<PariUser, Rate>
     */
    public function pariUser(): BelongsTo
    {
        return $this->belongsTo(PariUser::class);
    }
@choco-cat choco-cat added the bug Something isn't working label Oct 16, 2024
@calebdw
Copy link
Contributor
calebdw commented Oct 16, 2024

This should be:

    /**
     * @return BelongsTo<User, $this>
     */
    public function user(): BelongsTo
    {
        return $this->belongsTo(User::class);
    }

@calebdw
Copy link
Contributor
calebdw commented Oct 16, 2024

@canvural, I had new docs in #1990 that didn't get merged, can you cherry-pick that? 17bce5f

@szepeviktor
Copy link
Collaborator

Or just avoid the @return tag.

@calebdw
Copy link
Contributor
calebdw commented Oct 16, 2024

Or just avoid the @return tag.

The @return is useful for IDE autocompletion on the method call---and you can avoid a costly model file parse if you include them

@koraga
Copy link
Contributor
koraga commented Oct 16, 2024

This should be:

    /**
     * @return BelongsTo<User, $this>
     */
    public function user(): BelongsTo
    {
        return $this->belongsTo(User::class);
    }

Same error:

    /**
     * @return BelongsTo<User, $this>
     */
    public function third(): BelongsTo
    {
        return $this->belongsTo(User::class);
    }
Method App\Rate\Models\Rate::third() should return Illuminate\Database\Eloquent\Relations\BelongsTo<App\Admin\Models\User, App\Rate\Models\Rate> but   
         returns Illuminate\Database\Eloquent\Relations\BelongsTo<App\Admin\Models\User, $this(App\Rate\Models\Rate)>.                                          
         💡 Template type TDeclaringModel on class Illuminate\Database\Eloquent\Relations\BelongsTo is not covariant. Learn more:                               
            https://phpstan.org/blog/whats-up-with-template-covariant   

@calebdw
Copy link
Contributor
calebdw commented Oct 16, 2024

Make sure to clear any cache, phpstan clear-result-cache

@encodiaweb
Copy link

Make sure to clear any cache, phpstan clear-result-cache

Thanks for this tip, but unfortunately, it didn't solve. Same errors as @koraga

@koraga
Copy link
Contributor
koraga commented Oct 16, 2024
    /**
     * @return BelongsTo<User, covariant $this>
     */
    public function user(): BelongsTo
    {
        return $this->belongsTo(User::class);
    }

There is no error if I add covariant

@calebdw
Copy link
Contributor
calebdw commented Oct 16, 2024

Adding covariant is not the answer

This was working on #1990 (and it is working on my fork). Something likely was messed up when cherry-picking #1990

@calebdw
Copy link
Contributor
calebdw commented Oct 16, 2024

What does the full Model look like? By any chance are they final?

@koraga
Copy link
Contributor
koraga commented Oct 16, 2024

What does the full Model look like? By any chance are they final?

Yes, all my models are final

@calebdw
Copy link
Contributor
calebdw commented Oct 16, 2024

Yes, all my models are final

Ah, that's it---I can replicate it now. This looks like a PHPStan bug, I'll open an issue over there

@davideprevosto
Copy link
davideprevosto commented Oct 17, 2024

My Models are not final, but I am getting the same issue.

@calebdw
Copy link
Contributor
calebdw commented Oct 17, 2024

@davideprevosto, then there's also a bug in Larastan because non-final models work just fine on my fork

@NickSdot
Copy link
Contributor

Same issue here. Cache cleared etc. On my end it is not related to final/non-final. Caleb, I tried your fork and have the same result. Worth to mention that this is old code that used to work without reports.

/** @return BelongsTo<\App\Models\Company, \App\Models\MultisiteInquiry> */
public function company(): BelongsTo
{
    return $this->belongsTo(Company::class, 'company_id');
}
phpstan: Method App\Models\MultisiteInquiry::company() should return
Illuminate\Database\Eloquent\Relations\BelongsTo<App\Models\Company, App\Models\MultisiteInquiry> but returns
Illuminate\Database\Eloquent\Relations\BelongsTo<App\Models\Company, $this(App\Models\MultisiteInquiry)>.

Seems to me like it is a regression in Larastan 2.9.9.

Works? PHPStan Larastan
1.12.5 2.9.8
1.12.6 2.9.8
1.12.5 2.9.9
1.12.6 2.9.9

@calebdw
Copy link
Contributor
calebdw commented Oct 18, 2024

@NickSdot,

This error is correct, your generic should be:

/** @return BelongsTo<\App\Models\Company, $this> */

@NickSdot
Copy link
Contributor
NickSdot commented Oct 18, 2024

@NickSdot,

This error is correct, your generic should be:

/** @return BelongsTo<\App\Models\Company, $this> */

Thanks @calebdw. Not trying to argue, but please allow me to double confirm:

  1. Is it really intentional that a patch release makes my previouly working doc blocks to errors now? There is a ton of code on GitHub which is not using this/self, and it very likely worked before 2.9.9 (as it did in our projects).

  2. In a Laravel model, where we define the relation, $this === App\Models\MultisiteInquiry. Why would one work, and one not? Mind to elaborate on the technical difference for SA? I am having a hard time to see what is the added value of this breaking change.

  3. Using $this in a final class results in the below, which is what you reported here. So it definitely didn't work until now, which is indicating a new behaviour. However, the model FQN worked just fine until the last release.

phpstan: Method App\Models\MultisiteInquiry::company() should return
Illuminate\Database\Eloquent\Relations\BelongsTo<App\Models\Company, App\Models\MultisiteInquiry> but returns
Illuminate\Database\Eloquent\Relations\BelongsTo<App\Models\Company, $this(App\Models\MultisiteInquiry)>.

@calebdw
Copy link
Contributor
calebdw commented Oct 18, 2024

@NickSdot,

Yes it is intentional, and $this vs self matters when you have models that extend other models to insure the generics have the right model (the child and not the parent that the relation is defined on). This change actually fixed several bugs related to Model inheritance and as PHPStan has stated, sometimes fixing bugs / causing the code to be smarter causes new errors to be revealed.

Using $this is the most correct as the framework literally passes the $this instance into the Relation constructor:
https://github.com/laravel/framework/blob/d2895c733d738200964c6209714e37c20f568789/src/Illuminate/Database/Eloquent/Concerns/HasRelationships.php#L111

@NickSdot
Copy link
Contributor

@NickSdot,

Yes it is intentional, and $this vs self matters when you have models that extend other models to insure the generics have the right model (the child and not the parent that the relation is defined on). This change actually fixed several bugs related to Model inheritance and as PHPStan has stated, sometimes fixing bugs / causing the code to be smarter causes new errors to be revealed.

Using $this is the most correct as the framework literally passes the $this instance into the Relation constructor:

https://github.com/laravel/framework/blob/d2895c733d738200964c6209714e37c20f568789/src/Illuminate/Database/Eloquent/Concerns/HasRelationships.php#L111

Thank you for taking the time to explain! I get the intention.

Would have been nice if there would have been a fade-out phase until the next major were both the most correct $this, and the previous FQN approach would have worked in parallel. However, I know there is never a right time.

The above just as an aside. I appreciate all your hard work here, and in other repos. Thanks.🤝

@calebdw
Copy link
Contributor
calebdw commented Oct 18, 2024

There was talk about releasing this in v3, but we're going to release v3 when PHPStan is bumped to v2 so it was just decided to go ahead and release this to support Laravel >11.15

both the most correct $this, and the previous FQN approach would have worked in parallel.

This isn't as easy to do as it sounds. The dynamic return type extension has to return one value so it's kind of one or the other without a ton of complicated checks

he above just as an aside. I appreciate all your hard work here, and in other repos. Thanks.🤝

Thank you! You are too kind---I'm always happy to help 😄

@kayw-geek
Copy link
Contributor
 Line   app/Models/Support/FooTrait.php (in context of class App\Models\Bar)                                                              
 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------- 
  90     Method App\Models\Bar::getDate() should return Illuminate\Database\Eloquent\Collection<int, App\Models\Bar> but  
         returns Illuminate\Database\Eloquent\Collection<int, static(App\Models\Bar)>.  

The static method getDate() is declared in the FooTrait located in app/Models/Support/FooTrait. It returns a static::query(), and using this FooTrait in a final class will trigger the aforementioned error.

@calebdw
Copy link
Contributor
calebdw commented Oct 21, 2024

@kayw-geek,

There's an open issue at PHPStan. In the meantime you can either: 1) ignore the error, or 2) remove final from the model in question

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

8 participants