entity_load() is actually entity_load_multiple()

If we're going to do this at all, it makes sense to do it early-ish during D8.

mulitple

Patch can't work ;)

+1, this inconsistency annoyed me right from the start.

Thanks. Adding essential tags.

This is a task, no functional bug.

This 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?

Right, 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.

I 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

Interesting...
This should fix the tests...

Reviewing 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.

+++ b/includes/common.incundefined
@@ -7400,7 +7400,51 @@
+ *   The entity IDs to load, or FALSE to be able to select the first all entities.

Should this not be:
"The entity ID to load"

+++ b/modules/comment/comment.moduleundefined
@@ -1684,7 +1683,7 @@
+  protected function buildQuery($ids, array $conditions = array(), $revision_id = FALSE) {

Should $ids be type hinted to?

+++ b/modules/taxonomy/taxonomy.moduleundefined
@@ -1118,7 +1118,7 @@
 function taxonomy_vocabulary_load_multiple($vids = array(), $conditions = array()) {

Should probably be type hinted too

Powered by Dreditor.

Hmm 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

This should work.
Credit back to you, now it should be good to go.

#23: 1160566-entity-load-multiple_23.patch queued for re-testing.

Lets try this one...

Is 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.

@fago

" @todo Remove $conditions in Drupal 8."

Thats a seperate issue I think.

I 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.

I agree, it doesn't make sense to add a parameter with the comment that it is deprecated.

ok, 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.

I'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.

Tagging issues not yet using summary template.

Why 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

#824246: Better parameter handling for entity_load has been marked as a duplicate of this issue.

Why 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

Overloading 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.

As mentioned in #41, this one needs a reroll as code got moved around. Adding tags.

Here is a reroll of the patch.

Needs review.

bdlangton: 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.

Tests fixed.

Thanks! Here a closer review:

+++ b/core/modules/entity/entity.api.php
@@ -427,8 +428,9 @@ function hook_entity_view_alter(&$build, $type) {
+ * view. Only use this if attaching the data during the entity_load() or
+ * entity_load_multiple() phase is not appropriate, for example when attaching
+ * other 'entity' style objects.

Maybe we should re-word this sentence to say "if attaching the during the entity loading phase is not appropriate, ..".

+++ b/core/modules/taxonomy/taxonomy.test
@@ -1347,7 +1347,7 @@ class TaxonomyLoadMultipleUnitTest extends TaxonomyWebTestCase {
-    $terms = taxonomy_term_load_multiple(NULL, array('vid' => $vocabulary->vid));
+    $terms = taxonomy_term_load_multiple(array(), array('vid' => $vocabulary->vid));

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 ;)

+++ b/core/modules/taxonomy/taxonomy.test
@@ -1367,7 +1367,7 @@ class TaxonomyLoadMultipleUnitTest extends TaxonomyWebTestCase {
-    $terms4 = taxonomy_term_load_multiple(NULL, array('vid' => $vocabulary->vid));
+    $terms4 = taxonomy_term_load_multiple(array(), array('vid' => $vocabulary->vid));

Same here.

+++ b/core/modules/entity/entity.api.php
@@ -427,8 +428,9 @@ function hook_entity_view_alter(&$build, $type) {
+ * view. Only use this if attaching the data during the entity_load() or
+ * entity_load_multiple() phase is not appropriate, for example when attaching
+ * other 'entity' style objects.

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 .." ?

+++ b/core/modules/taxonomy/taxonomy.test
@@ -1347,7 +1347,7 @@ class TaxonomyLoadMultipleUnitTest extends TaxonomyWebTestCase {
-    $terms = taxonomy_term_load_multiple(NULL, array('vid' => $vocabulary->vid));
+    $terms = taxonomy_term_load_multiple(array(), array('vid' => $vocabulary->vid));

array() should be FALSE here. FALSE means load all entities, but array() means load entities by ID with no IDs given.

+++ b/core/modules/taxonomy/taxonomy.test
@@ -1367,7 +1367,7 @@ class TaxonomyLoadMultipleUnitTest extends TaxonomyWebTestCase {
-    $terms4 = taxonomy_term_load_multiple(NULL, array('vid' => $vocabulary->vid));
+    $terms4 = taxonomy_term_load_multiple(array(), array('vid' => $vocabulary->vid));

Same here.

+++ b/core/modules/entity/entity.module
@@ -191,7 +191,43 @@ function entity_create_stub_entity($entity_type, $ids) {
 /**
- * Loads entities from the database.
+ * Loads an entity from the database.
+ *
+ * The entity is stored in a static memory cache, and will not require
+ * database access if loaded again during the same page request.
+ *
+ * The actual loading is done through a class that has to implement the
....

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.

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 ;)

There are actually many other places where array() is used other than FALSE. Should I be changing that in all of these other places?

core/includes/file.inc:845:      $existing_files = file_load_multiple(array(), array('uri' => $uri));
core/includes/file.inc:1094:      $existing_files = file_load_multiple(array(), array('uri' => $uri));
core/includes/file.inc:1633:    $existing_files = file_load_multiple(array(), array('uri' => $file->uri));
core/includes/file.inc:1931:      $existing_files = file_load_multiple(array(), array('uri' => $uri));
core/modules/comment/comment.module:1418:      $comments = comment_load_multiple(array(), array('uid' => $account->uid));
core/modules/comment/comment.module:1426:      $comments = comment_load_multiple(array(), array('uid' => $account->uid));
core/modules/file/file.module:127:  $files = file_load_multiple(array(), array('uri' => $uri));
core/modules/image/image.module:299:  $files = file_load_multiple(array(), array('uri' => $uri));
core/modules/node/node.test:63:    $nodes = node_load_multiple(array(), array('promote' => 0));
core/modules/node/node.test:136:    $nodes = node_load_multiple(array(), array('status' => NODE_PUBLISHED));
core/modules/node/node.test:144:    $nodes = node_load_multiple(array(), array('status' => NODE_NOT_PUBLISHED));
core/modules/simpletest/drupal_web_test_case.php:908:    $nodes = node_load_multiple(array(), array('title' => $title), $reset);
core/modules/simpletest/tests/file.test:1931:    $files = file_load_multiple(array(), array('uri' => 'foobar://misc/druplicon.png'));
core/modules/simpletest/tests/file.test:1940:    $files = file_load_multiple(array(), array('status' => -99));
core/modules/simpletest/tests/file.test:1972:    $by_path_files = file_load_multiple(array(), array('uri' => $file->uri));
core/modules/simpletest/tests/menu.test:1324:      $terms = taxonomy_term_load_multiple(array(), array('name' => $name));
core/modules/taxonomy/taxonomy.module:1170:  return taxonomy_term_load_multiple(array(), $conditions);
core/modules/taxonomy/taxonomy.module:1701:      if ($possibilities = taxonomy_term_load_multiple(array(), array('name' => trim($typed_term), 'vid' => array_keys($vocabularies)))) {
core/modules/user/user.pages.inc:59:  $users = user_load_multiple(array(), array('mail' => $name, 'status' => '1'));
core/modules/user/user.pages.inc:63:    $users = user_load_multiple(array(), array('name' => $name, 'status' => '1'));
core/modules/user/user.test:37:    $accounts = user_load_multiple(array(), array('name' => $name, 'mail' => $mail));
core/modules/user/user.test:47:    $accounts = user_load_multiple(array(), array('name' => $name, 'mail' => $mail));
core/modules/user/user.test:72:    $accounts = user_load_multiple(array(), array('name' => $name, 'mail' => $mail));
core/modules/user/user.test:96:    $accounts = user_load_multiple(array(), array('name' => $name, 'mail' => $mail));
core/modules/user/user.test:160:    $accounts = user_load_multiple(array(), array('name' => $name, 'mail' => $mail));
core/modules/user/user.test:224:    $accounts = user_load_multiple(array(), array('name' => $name, 'mail' => $mail));
core/modules/user/user.test:252:      $accounts = user_load_multiple(array(), array('name' => $name, 'mail' => $mail));

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.

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.

There are actually many other places where array() is used other than FALSE. Should I be changing that in all of these other places?

Better not. We should try to keep this issue focused. So let's leave fixing that to another issue then.

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?

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.

Did the above suggestions to code that this particular issue is touching.

Althought 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.

+++ b/core/modules/comment/comment.module
@@ -1520,16 +1520,16 @@ function comment_delete_multiple($cids) {
-function comment_load_multiple($cids = array(), $conditions = array(), $reset = FALSE) {
-  return entity_load('comment', $cids, $conditions, $reset);
+function comment_load_multiple($cids = array(), array $conditions = array()) {
+  return entity_load_multiple('comment', $cids, $conditions);

$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 :)

Ok, I put $reset back in the comment_load_multiple function.

Fixed patch so it could apply.

This was broken by #1361232: Make the taxonomy entities classed objects, rerolled.

Sorry for the cross post, the one in #61 keeps the type-hinted @return docs for taxonomy_term_load_multiple.

+++ b/core/modules/entity/entity.moduleundefined
@@ -191,7 +191,31 @@ function entity_create_stub_entity($entity_type, $ids) {
+ *
+ * @see hook_entity_info()
+ * @see entity_load_multiple()
+ * @see DrupalEntityControllerInterface
+ * @see DrupalDefaultEntityController
+ * @see EntityFieldQuery

These @see should be at the bottom of the docblock after the @return.

fixed #63

+++ b/core/modules/entity/entity.moduleundefined
@@ -191,7 +191,32 @@ function entity_create_stub_entity($entity_type, $ids) {
+ * Loads an entity from the database.
+ *
+ *
+ * @param string $entity_type

Oops, left an extra blank line though. :)

Good catch, corrected

+++ b/core/modules/node/node.moduleundefined
@@ -913,40 +913,40 @@ function node_invoke($node, $hook, $a2 = NULL, $a3 = NULL, $a4 = NULL) {
- * @param $nid
+ * @param int $nid
  *   (optional) The node ID.
...
 function node_load($nid = NULL, $vid = NULL, $reset = FALSE) {

Does the default for $nid need to be changed to FALSE here? In either case we should improve the parameter documentation to explain what happens if you pass NULL or whatever the default is. (I also think the datatype should be int|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 the foo_load() are all wrappers for entity_load_multiple() in the end, we should standardize them as well. Thoughts?

in 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..

Alright, 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?

Here's the issue discussing creating a consistent reset pattern. Use a consistent/clean pattern for using $reset or drupal_static()

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Thanks @ryanissamson!

Love this patch.

+++ b/core/modules/comment/comment.module
@@ -1537,8 +1537,7 @@ function comment_load_multiple($cids = array(), $conditions = array(), $reset =
-  return $comment ? $comment[$cid] : FALSE;

+++ b/core/modules/entity/entity.module
@@ -191,7 +191,31 @@ function entity_create_stub_entity($entity_type, $ids) {
+  return reset($entities);

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().

Hm, 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..

oh, sorry, I actually meant this.

The 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) in taxonomy.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.

That looks fine.

+++ b/core/modules/taxonomy/taxonomy.testundefined
@@ -1382,7 +1382,7 @@ class TaxonomyLoadMultipleUnitTest extends TaxonomyWebTestCase {
     $this->assertTrue(count($terms4 == 4), 'Correct number of terms were loaded.');

Very awesome :) Not related to this patch, so I opened a follow-up for this: #1542674: Wrong count assertion in taxonomy.test.

Looks good. Committed/pushed to 8.x, thanks!

Change record: http://drupal.org/node/1553180

Looks great!

Just 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.

oops. didn't mean to remove tags. adding them back now.

Updated issue summary.