-
Notifications
You must be signed in to change notification settings - Fork 47
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 traversal when parent model is loaded #853
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I don't think this is correct approach.
Loading record shouldn't add condition to model because that will limit how getTitles()
, export()
and other dataset-wise methods will work when model is or is not loaded. That shouldn't happen.
f519577
to
14a1a34
Compare
All changes reverted and addressed differently, |
// add entity ID to scope to allow easy traversal | ||
if ($model->id_field && $model->getId() !== null) { | ||
$query->group($model->getField($model->id_field)); | ||
$query->having($model->getField($model->id_field), $model->getId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: having
with group by
is like where
, but it does not apply after aggregate functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved in advance. Need to fix this approach for MS SQL
d23156b
to
83077c2
Compare
8c2ab17
to
02df81d
Compare
5588b33
to
6834cd4
Compare
cee8d38
to
bf48e54
Compare
3781e5f
to
85c1345
Compare
a121105
to
1836826
Compare
1836826
to
1b4dc57
Compare
finish after #855, #860, #861
discussion here: c8cbce3#r49718142
fixes https://github.com/mvorisek/atk4-hintable-mirror/blob/1.2.1/tests/Data/HintableModelTest.php#L120
must traverse from model with ID = 2 only (before this PR, ID = 2 condition was not applied and it was required to be added manually, now with entity concept we can assume locked ID)
note: this works also with
$model->load{One, Any}()->simpleMany
and also withModel::duplicate()
which will unset the ID correctly.