Problem/Motivation
entity_load() should load a single entity, entity_load_multiple() should load multiple entities in order to be consistent with other API functions like entity_delete() and entity_delete_multiple().
Remaining tasks
Use a consistent/clean pattern for using $reset or drupal_static() discusses creating a consistent reset pattern.
Standardizing [entity_type]_load() is a followup issue discussing [entity_type]_load().
User interface changes
none.
API changes
"entity_load_multiple() introduced. entity_load() is now just a wrapper for entity_load_multiple(), so it's still okay to use it if one wants to load only a *single* entity.
Original report by Dave Reid
Let's fix it.
Comment | File | Size | Author |
---|---|---|---|
#76 | drupal8.entity-load.75.patch | 22.5 KB | sun |
#73 | 1160566-no-reset.patch | 22.49 KB | Berdir |
#73 | 1160566-interdiff.patch | 2.25 KB | Berdir |
#66 | 1160566_66_user_entity.patch | 22.4 KB | cosmicdreams |
#64 | 1160566_64_user_entity.patch | 22.41 KB | cosmicdreams |
Comments
Comment #1
Dave ReidFYI I wanted to abstract this from a larger entity issue so that I could just focus on renaming the function and adding a real entity_load().
Comment #3
catchIf we're going to do this at all, it makes sense to do it early-ish during D8.
Patch can't work ;)
Comment #4
Dave Reiddoh!
Comment #5
Dave ReidDid not like that $id had a default parameter.
Comment #7
sun+1, this inconsistency annoyed me right from the start.
Comment #8
Dave ReidFound the bug...was with user_load_by_name().
Comment #10
Dave ReidOk, actually tried this one. Fixed bug with node_load() passing an array rather than a singular ID.
Comment #11
Dave ReidExcellent, #10 is now fully ready for review.
Comment #12
sunThanks. Adding essential tags.
Comment #13
catchThis is a task, no functional bug.
Comment #14
Dries CreditAttribution: Dries commentedThis patch is RTBC.
Given that this is an API change we shouldn't backport it to Drupal 7.
Let's wait with this patch until we fixed more of Drupal 7?
Comment #15
catchRight, if we go with something like http://drupal.org/node/1050616#comment-4471480, then we'd want to hold off committing this patch until D7 is in better shape, so it depends what's agreed there.
Comment #16
aspilicious CreditAttribution: aspilicious commentedI reuploaded this patch because I made some small doc changes.
It would be nice if you guys (great core coders) could use the same coding standards for D8.
Why?
There will be a documentation gate, probably that gate will ensure there will be api documentation but I guess it also will check doc standards of the newly written or rewritten parts. It would be sad if we have to redo all the docs in the end :).
Some notes:
1) Please try to use 3th person verbs for functions, even if the other functions in the file aren't using it yet.
2) There is a new not yet documented "standard" in drupal land that says you should type hint arrays, the original patch did that alrdy I tried to do it on some more arrays. We will see if I broke something ;)
3) If you're rewritting a function with no space between @param and @return don't hesitate to place the space yourself.
If my patch fails for some reason you can use the one in #10
Comment #18
aspilicious CreditAttribution: aspilicious commentedInteresting...
This should fix the tests...
Comment #19
Dave Reid@aspilicious: Thanks for the improvements. I just copied/pasted the help from the previous entity_load() fyi. Could you also upload a patch that doesn't completely change the git credit? :)
Comment #20
aspilicious CreditAttribution: aspilicious commentedReviewing my own patch. If we are going to fix everything at once, we probably should do the following before commiting.
Would like a review before I do some more (maybe unneeded) work.
Should this not be:
"The entity ID to load"
Should $ids be type hinted to?
Should probably be type hinted too
Powered by Dreditor.
Comment #21
aspilicious CreditAttribution: aspilicious commentedHmm davereid did I change the credit that is totally not intentional. I just use the same process as with my contrib work. I'm going to investigate how to get your credit back...
PS: I'm using egit for eclipse
Comment #22
Dave Reid$ids parameter can always be FALSE, so that should not be type-hinted unless we also change all invocations to use array().
Comment #23
aspilicious CreditAttribution: aspilicious commentedThis should work.
Credit back to you, now it should be good to go.
Comment #25
aspilicious CreditAttribution: aspilicious commented#23: 1160566-entity-load-multiple_23.patch queued for re-testing.
Comment #27
aspilicious CreditAttribution: aspilicious commentedLets try this one...
Comment #28
fagoIs there any cause to support $conditions in the single entity_load() ?
>The entity ID to load, or FALSE to be able to select the first all entities.
I find it a bit confusing to select multiple entities if I load only one.
Furthermore I don't think we need that $conditions support in a single-entity load as
- the results are probably multiple
- multiple results are loaded even if a single one is returned
- it is already marked as deprecated anyway
- I doubt this is an often used feature. If required, calling entity_load_multiple + reset still does it + makes it clear in reality multiple entities are loaded.
Comment #29
aspilicious CreditAttribution: aspilicious commented@fago
" @todo Remove $conditions in Drupal 8."
Thats a seperate issue I think.
Comment #30
fagoI don't think so, as this patch is in fact introducing this feature for the *new* single entity_load() function. I agree though that removing it from entity_load_multiple() is a separate issue.
Comment #31
tstoecklerI agree, it doesn't make sense to add a parameter with the comment that it is deprecated.
Comment #32
Dave ReidMake sure to also adjust the user_load_by_name() and user_load_by_mail() as those used the $conditions variable in entity_load().
Comment #34
fagook, I've fixed that + at some other places like node_load() and user_load(). Beside that, there were 2 strict warnings due to off-topic function signature changes in entity-load-controllers. Removed that too.
@node_load()
What about introducing a $revision_id parameter to entity_load() as we support for node_load()? Well, we could do that in a follow-up too.
Comment #35
Dave Reidfollow-up for node_revision_load() or entity_revision_load()
Comment #37
fagoComment #38
catchI'd like to take the revision handling completely out of entity_load() and have entity_revision_load() (and associated other functions specifically for revisions) but that's definitely a followup.
Comment #39
catchComment #40
xjmTagging issues not yet using summary template.
Comment #41
Dave ReidThis needs to be re-rolled since entity.inc was moved to entity.module.
Comment #42
NealB-1 CreditAttribution: NealB-1 commentedWhy have two functions? entity_load() can figure out what it was passed and do the right thing. See http://drupal.org/node/824246#comment-5610334
Comment #43
xjm#824246: Better parameter handling for entity_load has been marked as a duplicate of this issue.
Comment #44
fagoOverloading PHP parameters isn't good practice, so let's better separate those. Also this ensures consistency with other API functions like entity_delete() and entity_delete_multiple()
Note: I've added an issue summary.
Comment #45
fagoAs mentioned in #41, this one needs a reroll as code got moved around. Adding tags.
Comment #46
bdlangton CreditAttribution: bdlangton commentedHere is a reroll of the patch.
Comment #47
bdlangton CreditAttribution: bdlangton commentedNeeds review.
Comment #49
fagobdlangton: thanks!
In the meantime some tests got added (see EntityAPITestCase), so we'll have to convert entity_load() calls to entity_load_multiple() there too. That should fix the test bot errors.
Comment #50
bdlangton CreditAttribution: bdlangton commentedTests fixed.
Comment #51
fagoThanks! Here a closer review:
Maybe we should re-word this sentence to say "if attaching the during the entity loading phase is not appropriate, ..".
We should use FALSE instead of array() here. FALSE means load all entities, but array() means load entities by ID with no IDs given - thus load no entities ;)
Same here.
This sentence could need an improvement as there not two loading phases, e.g. just re-word it to say "if attaching the data during the entity loading phase is not .." ?
array() should be FALSE here. FALSE means load all entities, but array() means load entities by ID with no IDs given.
Same here.
Hm, that's a lot of documentation that is duplicated. Let's better just keep the first line summary and the parameter documentation + point to the docs of entity_load_multiple(). Also, in the meantime the coding standards changed so we should add data types to all parameter and return documentation statements. But let's fix this for entity_load_multiple() too so we stay consistent with that.
Comment #52
bdlangton CreditAttribution: bdlangton commentedThere are actually many other places where array() is used other than FALSE. Should I be changing that in all of these other places?
Almost none of the documentation lists data types with the parameters and return values. Am I supposed to just update entity_load() and entity_load_multiple() for now, or go through and update more? Where would I stop?
Thanks for your help.
Comment #53
fagoBetter not. We should try to keep this issue focused. So let's leave fixing that to another issue then.
True. New code should comply with our recent guidelines, old code might be converted with the time and/or dedicated issues. So let's just make sure the functions we touch are alright.
Comment #54
bdlangton CreditAttribution: bdlangton commentedDid the above suggestions to code that this particular issue is touching.
Comment #55
bdlangton CreditAttribution: bdlangton commentedComment #56
cosmicdreams CreditAttribution: cosmicdreams commentedAlthought the changing of comments for non-entity related things seems to be beside the point, I'm glad you did it. It needed to be clarified anyway.
Otherwise the patch looks good to me. Recommend RTBC.
Comment #57
amateescu CreditAttribution: amateescu commented$reset has been removed here but it remained in the doxygen block. I think it's removal was not intentional and should be brought back :)
Comment #58
bdlangton CreditAttribution: bdlangton commentedOk, I put $reset back in the comment_load_multiple function.
Comment #60
bdlangton CreditAttribution: bdlangton commentedFixed patch so it could apply.
Comment #61
tim.plunkettThis was broken by #1361232: Make the taxonomy entities classed objects, rerolled.
Comment #62
tim.plunkettSorry for the cross post, the one in #61 keeps the type-hinted @return docs for taxonomy_term_load_multiple.
Comment #63
xjmThese @see should be at the bottom of the docblock after the @return.
Comment #64
cosmicdreams CreditAttribution: cosmicdreams commentedfixed #63
Comment #65
xjmOops, left an extra blank line though. :)
Comment #66
cosmicdreams CreditAttribution: cosmicdreams commentedGood catch, corrected
Comment #67
xjmDoes the default for
$nid
need to be changed toFALSE
here? In either case we should improve the parameter documentation to explain what happens if you passNULL
or whatever the default is. (I also think the datatype should beint|null
given the current code, right? Same for $vid.)The behavior of
node_load()
is also totally inconsistent with other functions, e.g.taxonomy_term_load()
. I'm thinking if thefoo_load()
are all wrappers forentity_load_multiple()
in the end, we should standardize them as well. Thoughts?Comment #68
Berdirin the end, we should probably throw away the $reset params to that function, then we can make $id a required argument. But probably not here and I think there already is a major task open to create a consistent reset pattern..
Comment #69
xjmAlright, let's get that issue about resetting linked here, and open a followup for standardizing
foo_load()
, and then add both to the summary here as remaining tasks? (The summary is also a bit sparse so let's flesh it out.) At that point I'd be comfortable RTBCing this, I think, unless there are any other concerns?Comment #70
ryan.gibson CreditAttribution: ryan.gibson commentedHere's the issue discussing creating a consistent reset pattern. Use a consistent/clean pattern for using $reset or drupal_static()
Comment #70.0
ryan.gibson CreditAttribution: ryan.gibson commentedUpdated issue summary.
Comment #70.1
ryan.gibson CreditAttribution: ryan.gibson commentedUpdated issue summary.
Comment #70.2
ryan.gibson CreditAttribution: ryan.gibson commentedUpdated issue summary.
Comment #70.3
ryan.gibson CreditAttribution: ryan.gibson commentedUpdated issue summary.
Comment #71
xjmThanks @ryanissamson!
Comment #72
sunLove this patch.
I'd prefer to use the more explicit array index access (as in the old comment_load() above), as it's not only an additional validation to ensure that the returned entry is the requested one, but it also clarifies the negative return value of FALSE -- which is totally not obvious with reset().
Comment #73
BerdirHm, I think that reset() is quite a common pattern. However, I don't care about that, but I care about this patch getting commited, so here is a re-roll.
Also had to fix a bunch of conflicts, mostly due to moved test files (tell me again why we did that ;)). Attaching an interdiff, not sure what's the deal with the second part of the interdiff, they didn't apply anymore so I re-did the changes manually. Looks weird now in the interdiff..
Comment #75
BerdirComment #76
sunoh, sorry, I actually meant this.
Comment #77
xjmThe interdiff in #73 is a bit suspect, so I checked locally with the patch applied and confirmed there are no stray
taxonomy_term_load_multiple(NULL, whatever)
intaxonomy.test
. Looking closely, it appears that those changes are relative to each other? Weird. Silly non-git interdiffs. :)In any case, waiting for the bot.
Comment #78
xjmThat looks fine.
Comment #79
BerdirVery awesome :) Not related to this patch, so I opened a follow-up for this: #1542674: Wrong count assertion in taxonomy.test.
Comment #80
catchLooks good. Committed/pushed to 8.x, thanks!
Comment #81
BerdirChange record: http://drupal.org/node/1553180
Comment #82
aspilicious CreditAttribution: aspilicious commentedLooks great!
Comment #83
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #84
xjmComment #86
jyee CreditAttribution: jyee commentedJust noting that #708730: For consistancy, add taxonomy_term_view_multiple was closed as a duplicate of this issue as taxonomy_term_view_multiple() was added in work above.
Comment #87
jyee CreditAttribution: jyee commentedoops. didn't mean to remove tags. adding them back now.
Comment #87.0
jyee CreditAttribution: jyee commentedUpdated issue summary.