-
-
Notifications
You must be signed in to change notification settings - Fork 394
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
canvural
requested changes
Oct 25, 2021
Thanks for the feedback @canvural. Should I rebase the commits, or do you just want to Squash & Merge? |
canvural
changed the title
Add new rule NoModelMake to prevent inefficient instantiation
feat: add new rule Oct 25, 2021
NoModelMake
to prevent inefficient instantiation
Squash and merge is fine. Thank you! |
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 instantiatinga 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.