-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[DDC-1398] Extra-lazy get for indexed associations #706
Conversation
If an association is EXTRA_LAZY and has an indexBy, then you can call get() without loading the entire collection.
<3 |
throw new \BadMethodCallException("Selecting a collection by index is only supported on indexed collections."); | ||
} | ||
|
||
return current($persister->load(array($mapping['indexBy'] => $index), null, null, array(), 0, 1)); |
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.
you need to handle the case of non-existent keys properly: the Collection interface wants to get null
in such case, not false
(which is what current
returns when the array is empty)
you could also implement |
|
||
$queryCount = $this->getCurrentQueryCount(); | ||
|
||
$user->articles->get($this->articleId); |
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.
can you test for the actual data being returned as well? in the second test as well
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.
and please also add a test for non-existent keys
The load() function already returns just one entity or NULL, so the current() is not needed and the result can be returned directly.
I think I fixed all the things you pointed out. I'm looking at implementing |
I am not sure what the best way is to handle
So, what do you suggest? |
@sandermarechal Lets keep containsKey for another time, we have something very useful here already. One thing I would like to see is the Persister Collections to use find() instead of findBy() to use the identity map, if the index-by is the identifier field. Otherwise its already awesome |
@beberlei: I'm sorry, I don't quite understand what you mean. I'm not using find() in this PR but I am using load() directly. Do you mean fixing the TODO at BasicEntityPersister->load() on line 742? |
@sandermarechal yes, please use the |
@sandermarechal regarding your propsal above, I don't think |
@sandermarechal i am afraid you need to add more tests now ;) the article stuff uses articleIds, so its never triggering the collection persister get calls anymore |
@beberlei Yup. Done. |
[DDC-1398] Extra-lazy get for indexed associations
If an association is EXTRA_LAZY and has an indexBy, then
you can call get() without loading the entire collection.