-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Fixed $model->countRelated() failing when the relation is reusable #14444
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
zsilbi
changed the title
[WIP] Fixed Model::countRelated() failing when the relation is reusable
[WIP] Fixed $model->countRelated() failing when the relation is reusable
Oct 3, 2019
sergeyklay
reviewed
Oct 11, 2019
sergeyklay
reviewed
Oct 11, 2019
@sergeyklay I'd also change What do you think? |
@zsilbi Let's use a separated PR to change Model::__callStatic() |
retrieve method (find, findFirst, count.. etc.)
New RobotsReusable model for testing reusable relation
null. Therefore it should return false, instead of null when the requested method doesn't exist.
_getRelatedRecords returns null. Therefore it should return false, instead of null when the requested relation doesn't exist.
Codecov Report
@@ Coverage Diff @@
## 4.0.x #14444 +/- ##
==========================================
+ Coverage 67.67% 67.68% +0.01%
==========================================
Files 482 482
Lines 111229 111235 +6
==========================================
+ Hits 75275 75294 +19
+ Misses 35954 35941 -13 |
zsilbi
force-pushed
the
model-reusable-records
branch
from
October 13, 2019 16:42
e44ca54
to
93ae076
Compare
zsilbi
changed the title
[WIP] Fixed $model->countRelated() failing when the relation is reusable
Fixed $model->countRelated() failing when the relation is reusable
Oct 13, 2019
sergeyklay
approved these changes
Oct 13, 2019
ruudboon
approved these changes
Oct 13, 2019
niden
added
documentation
Documentation required
and removed
wip
Work In Progress
labels
Oct 13, 2019
Thank you @zsilbi |
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.
Hello!
In raising this pull request, I confirm the following:
Small description of change:
The
uniqueKey
used to store reusable related records inModelsManager
is currently generated only from the conditions used to lookup the records.For example:
It uses the same key for
$robot->getParts(["some" = "thing"])
and$robot->countParts(["some" = "thing"])
.If the related records are accessed again using a different method, it will return the result of the first.
In this case, it will be a
Resultset
instead of aninteger
.I added the
retrieveMethod
variable to theuniqueKey
generation.Added some tests for
Model::__call()
Thanks,
zsilbi