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

Fix issue where \Eloquent is not included when using write_mixin #1352

Merged
merged 4 commits into from
Aug 11, 2022

Conversation

Jefemy
Copy link
Contributor

@Jefemy Jefemy commented May 22, 2022

Summary

If you write your models using the --write-mixin option the included docblock generated on the model will include a mixin. Previously the code replaced here would ignore the @mixin \Eloquent addition if any mixin existed on the model. This would break the code suggestions as on first write the Eloquent mixin would be added but after that it wouldn't be generated as its detected as 'already existing'. Info including the lines and commits where I found this are discussed in #1299

This fix changes it to a loop which removes any mixins that matches the Eloquent class before appending it.

Also fixes #1342, fixes #1343, fixes #1299

Type of change

  • Bug fix (non-breaking change which fixes an issue)

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

@Jefemy Jefemy changed the title Check for exact mixin name for Eloquent when writing model docblock Fix issue where \Eloquent is not included when using write_mixin May 22, 2022
@FatBoyXPC
Copy link

Will this end up writing @mixin \Eloquent to the model class, or to the generated _ide_helper_models.php classes?

@Jefemy
Copy link
Contributor Author

Jefemy commented May 25, 2022

Will this end up writing @mixin \Eloquent to the model class, or to the generated _ide_helper_models.php classes?

It will put it in the generated _ide_helper_models class. This pull request changes no base functionality in the package, it just fixes an issue where on a second call of the ide-helper:models --write-mixin it will create a different output.

To temporarily work around the issue while this isn't merged you can delete the docblock written to your main models that contains the @mixin IdeHelperModel and rerun the generate command.
You will see that @mixin \Eloquent is properly included in _ide_helper_models for those models.

@FatBoyXPC
Copy link

For the temporary work around, do you mean delete the @mixin IdeHelperFoo from the Foo model?

@Jefemy
Copy link
Contributor Author

Jefemy commented May 25, 2022

For the temporary work around, do you mean delete the @mixin IdeHelperFoo from the Foo model?

Yes. In the current version any mixins existing on the original model will cause \Eloquent to be excluded from the generated class.

@FatBoyXPC
Copy link

Wow - so this also doesn't add it when you add a brand new model (it'll add the mixin to the model but not to the generated class). Certainly frustrating. Thanks for this PR!

@dnmarques
Copy link

@barryvdh any plans to merge this pull request?

@barryvdh barryvdh merged commit c8f0f91 into barryvdh:master Aug 11, 2022
ppmathis pushed a commit to ppmathis/laravel-ide-helper that referenced this pull request Nov 12, 2022
…ryvdh#1352)

* Check for exact mixin name when writing

* changelog

* add mixin to test class

Co-authored-by: Barry vd. Heuvel <[email protected]>
d3v2a pushed a commit to d3v2a/laravel-ide-helper that referenced this pull request Feb 16, 2024
…ryvdh#1352)

* Check for exact mixin name when writing

* changelog

* add mixin to test class

Co-authored-by: Barry vd. Heuvel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants