Skip to content

Typehint relations and $user/$auditable properties for phpstan#1061

Open
KentarouTakeda wants to merge 1 commit into
owen-it:masterfrom
KentarouTakeda:typehint-relations-for-phpstan
Open

Typehint relations and $user/$auditable properties for phpstan#1061
KentarouTakeda wants to merge 1 commit into
owen-it:masterfrom
KentarouTakeda:typehint-relations-for-phpstan

Conversation

@KentarouTakeda

Copy link
Copy Markdown

This adds the Eloquent relation generics (Laravel 11.15+) to the relation methods and types the $user/$auditable morph properties. Docblocks and use statements only — there are no runtime behavior changes.

Motivation: in our project we record custom audit events (AuditCustom) and add helpers through a project trait layered on top of Auditable. The relation methods and these properties are effectively untyped for static analysis, so each consumer ends up re-asserting types locally. Narrowing them on the library side makes consuming implementations safer.

  • Auditable::audits(): @return MorphMany<Models\Audit, $this>
  • Audit::auditable() / Audit::user(): @return MorphTo<Model, $this>
  • Models\Audit: @property (Model&Auditable)|null $auditable and @property (Model&Authenticatable)|null $user — both were mixed, and the $auditable line had a stray trailing . that broke the annotation

Consumers running PHPStan/Larastan then get $post->audits() as MorphMany<Audit, Post>, $post->audits as Collection<int, Audit>, $audit->user as (Model&Authenticatable)|null, and so on.

PHPStan level 8 passes. The resolved types above were also verified with PHPStan\dumpType() against a sample consuming project (a local check, not part of this PR).

@willpower232 willpower232 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense but also I think the @inheritdoc 's are supposed to be coming from src/Contracts/Audit.php and src/Contracts/Auditable.php so it would be worth updating those rather than the traits.

I also don't think there should be any types at the top of the model, larastan should be sufficient these days so if you're able to confirm it works without any of them, that would be super swell.

@KentarouTakeda

Copy link
Copy Markdown
Author

Thanks for the review! I gave both suggestions a try — I couldn't make either of them work in the end, but here's what I ran into, in case I'm missing something.

Contracts vs. traits

You're right that the contracts are the natural home for these. I tried moving them, but the relation generics get in the way: TDeclaringModel is invariant, and the implementation ($this->morphTo()) returns MorphTo<Model, $this>, so it has to be the concrete model.

  • MorphTo<..., static> on the interface → Type static(Contracts\Audit) ... is not subtype of template type TDeclaringModel of Model. Adding @phpstan-require-extends Model to the interface doesn't change that.
  • MorphTo<..., Model> instead → Return type (MorphTo<Model, $this(Models\Audit)>) of method Models\Audit::auditable() is not covariant with return type (MorphTo<Model, Model>) of method Contracts\Audit::auditable() — with or without an annotation on the trait.

$this only resolves to the concrete model inside a trait, so that seems to be the only place it can live. (Contracts\Auditable::audits() does carry MorphMany<Audit, Model> today, but nothing in src/ uses that trait, so PHPStan never checks it against an implementation.) The one way to make the interface work would be to make Contracts\Audit generic, but that would force generics on every consumer type-hinting it — a bigger call than I wanted to make here.

The @property block

I checked, and our own PHPStan run fails without it:

 86: Access to an undefined property Models\Audit::$event.
 87: Access to an undefined property Models\Audit::$tags.
230: Parameter #1 $auditable of method Models\Audit::decodeAttributeValue()
     expects Contracts\Auditable, Illuminate\Database\Eloquent\Model given.
324: Access to an undefined property Models\Audit::$tags.

Two reasons, as far as I can tell:

  • The columns can't be derived from a migration. Ours ships as audits.stub, and even dropped in as a real .php migration it doesn't help, because the table name and morph prefix come from config() at runtime — so the same is true in a consumer app that published it. (I think this is also why Typehint created_at and updated_at values for phpstan #1023 landed this way.)
  • $user/$auditable can't come from the relations either: morphTo() is fixed at MorphTo<Model, $this> and TRelatedModel is invariant, so MorphTo<Model&Auditable, $this> gets rejected. Without the @property, $this->auditable falls back to Model — which is what breaks line 230.

What this changes for consumers

PHPStan dumpType() on a model using the Auditable trait. These all hold under plain PHPStan too, so projects that don't run Larastan benefit as well:

expression master this PR
$post->audits() MorphMany<Audit, Model> MorphMany<Audit, Post>
$audit->auditable() mixed MorphTo<Model, Audit>
$audit->auditable mixed (Model&Auditable)|null
$audit->user mixed (Authenticatable&Model)|null

If there's an angle I've missed here, I'm glad to rework it.


P.S. Out of scope here, but I noticed it while checking the above: @mixin Models\Audit on Contracts\Audit only propagates methods, not properties — so some types don't reach consumers, these ones included. In an Audited listener, for instance, $event->audit->tags is an "undefined property". @phpstan-require-extends Models\Audit resolves it, though unlike @mixin it's a checked constraint, so that's a call for you. It's the same on master — happy to raise it separately if it's of interest.

@parallels999

Copy link
Copy Markdown
Contributor

Seems great to me

happy to raise it separately if it's of interest.

Any help is welcome.

@willpower232

Copy link
Copy Markdown
Contributor

@KentarouTakeda thanks for your thorough efforts! would you mind getting rid of the @inheritdoc 's on the three you've changed since they don't seem to add any value?

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 this pull request may close these issues.

3 participants