Skip to content

Generate cleaner model mixin classes #1268

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

Merged
merged 2 commits into from
Oct 25, 2021
Merged

Conversation

georgeboot
Copy link
Contributor

@georgeboot georgeboot commented Oct 14, 2021

Summary

1. removed @mixin from mixin itself when using mixin mode

The generated mixin class, contained a @mixin phpdoc to itself. This breaks phpstan as it's a recursive reference.

2. don't add extends and implements to generated class when using mixin mode

If you are using mixin mode, the mixin only needs to contain the properties and methods. The mixin does not need to extend the models classes, as the model already does that and the helper annotations are mixed in.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Misc. change (internal, infrastructure, maintenance, etc.)

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

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

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

Not 110% sure about the effect but happy to give it a try 😄

@georgeboot
Copy link
Contributor Author

Not 110% sure about the effect but happy to give it a try 😄

We're using it atm and works like charm :)

@barryvdh barryvdh merged commit dd12b7c into barryvdh:master Oct 25, 2021
@georgeboot georgeboot deleted the patch-1 branch October 25, 2021 11:47
@edcoreweb
Copy link
Contributor

@mfn @georgeboot I added the write_mixin feature initially, and unfortunately this breaks auto-complete on builder methods such as firstOrFail, find, etc.

To fix this, you need to add extends \Eloquent back or replace it with @mixin \Eloquent

$output .= "extends \Eloquent ";

Or to add the @mixin \Eloquent this needs to be modified.

if ($this->write && !$phpdoc->getTagsByName('mixin')) {

@georgeboot
Copy link
Contributor Author

Are you sure about that?

A static analyser should combine the data from the mixing with the data inferred from the class itself (hence the name mixin). Practically this means that this package should only min-in attributes, relations and other magic methods into the existing definition, which should already contain findOrFail etc.

@edcoreweb
Copy link
Contributor

@georgeboot I am pretty sure.

findOrFail is a method on the Illuminate\Database\Eloquent\Builder not on Illuminate\Database\Eloquent\Model, so it's not available unless you extend \Eloquent or mix that in.

The package already mixes in Illuminate\Database\Eloquent\Builder and Illuminate\Database\Query\Builder into a new facade, \Eloquent

See the config file.
image

@georgeboot
Copy link
Contributor Author

Ah now I see what you mean. I personally always use MyModel::query()..... so haven't noticed this.

Maybe related: didn't Laravel 9 change things related to the multiple types of query builders and their relation to model classes?

@edcoreweb
Copy link
Contributor

edcoreweb commented Mar 17, 2022

Not that I am aware of, they added some dummy interfaces for the Builder and QueryBuilder classes to assist with code completion, but those only come into play if you do Model::query()->findOrFail and not Model::findOrFail

IMHO this introduced a breaking change, and it needs to be fixed.

d3v2a pushed a commit to d3v2a/laravel-ide-helper that referenced this pull request Feb 16, 2024
* Generate cleaner model mixin classes

* Fix test
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.

4 participants