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

feat: add new rule NoModelMake to prevent inefficient instantiation #976

Merged
merged 13 commits into from
Oct 25, 2021
Merged

feat: add new rule NoModelMake to prevent inefficient instantiation #976

merged 13 commits into from
Oct 25, 2021

Conversation

spawnia
Copy link
Contributor

@spawnia spawnia commented Oct 22, 2021

  • Added or updated tests
  • Documented user facing changes
  • Updated CHANGELOG.md

Changes

Due to laravel-ide-helper generating a stub containing static methods for all proxied builder methods,
it appeared to some developers in our team that calling Model::make() is a sensible way of instantiating
a model. The symmetry with Model::create() and matching methods in the model factory might suggest it is.

However, this method is not supposed to be used like this - its purpose is rather served in relations or when
hydrating models from a collection. Performance measurements showed me that it is 40x slower then simply calling new Model. In fact, Model::make() performs a whole bunch of unnecessary work, such as instantiating the model twice (!).

Because this is a very simple mistake to make, I decided to go here and try to fix it once and for all, for everybody. While this rule does not necessarily uncover bugs in the code, it is similar to the NoUnnecessaryCollectionCallRule and improves performance when followed.

Breaking changes

No, but analysis will become more strict.

src/ReturnTypes/ModelExtension.php Outdated Show resolved Hide resolved
src/Rules/NoModelMakeRule.php Outdated Show resolved Hide resolved
tests/Features/Methods/ModelExtension.php Outdated Show resolved Hide resolved
@spawnia
Copy link
Contributor Author

spawnia commented Oct 25, 2021

Thanks for the feedback @canvural. Should I rebase the commits, or do you just want to Squash & Merge?

@canvural canvural changed the title Add new rule NoModelMake to prevent inefficient instantiation feat: add new rule NoModelMake to prevent inefficient instantiation Oct 25, 2021
@canvural canvural merged commit c7e41a8 into larastan:master Oct 25, 2021
@canvural
Copy link
Collaborator

Squash and merge is fine.

Thank you!

@spawnia spawnia deleted the no-model-make-rule branch October 25, 2021 12:11
@spawnia
Copy link
Contributor Author

spawnia commented Oct 25, 2021

Thanks for including this pull request. I am glad to have this feature in Larastan, makes it easier to adopt it across many projects. I hope it will help others as well 👍

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.

None yet

2 participants