entity_load() has a $conditions parameter that allows you to pass in entity properties instead of IDs. This was marked deprecated towards the end of the Drupal 7 cycle when EntityFieldQuery was added, it's time to remove it now.

There are some places in core that use this feature, mainly some helper functions in user module and tests, so they'll need updating as well as the controllers.

CommentFileSizeAuthor
#82 1184272-81-interdiff.txt2.55 KBcorvus_ch
#81 1184272-81.patch118.08 KBcorvus_ch
#80 1184272-80.patch118.08 KBcorvus_ch
#79 1184272-79.patch1.31 KBcorvus_ch
#77 1184272-77.patch118.06 KBcorvus_ch
#77 1184272-77-interdiff.txt2.28 KBcorvus_ch
#68 entity-load-conditions-begone-1184272-68.patch118.13 KBBerdir
#68 entity-load-conditions-begone-1184272-68-interdiff.txt2.07 KBBerdir
#61 entity-load-conditions-begone-1184272-61.patch118.1 KBBerdir
#61 entity-load-conditions-begone-1184272-61-interdiff.txt788 bytesBerdir
#59 entity-conditions.59.patch116.05 KBsun
#59 interdiff.txt9.37 KBsun
#55 entity-load-conditions-begone-1184272-55.patch114.79 KBBerdir
#55 entity-load-conditions-begone-1184272-55-interdiff.txt3.09 KBBerdir
#52 entity-load-conditions-begone-1184272-52.patch114.8 KBBerdir
#52 entity-load-conditions-begone-1184272-52-interdiff.txt35.97 KBBerdir
#43 entity-load-conditions-begone-1184272-43.patch108.07 KBBerdir
#43 entity-load-conditions-begone-1184272-43-interdiff.txt1.57 KBBerdir
#41 entity-load-conditions-begone-1184272-41.patch106.84 KBBerdir
#41 entity-load-conditions-begone-1184272-41-interdiff.txt10.4 KBBerdir
#38 entity-load-conditions-begone-1184272-37-interdiff.txt1.03 KBBerdir
#37 entity-load-conditions-begone-1184272-37.patch102.68 KBBerdir
#37 entity-load-conditions-begone-1184272-37.patch102.68 KBBerdir
#35 entity-load-conditions-begone-1184272-35.patch105.83 KBBerdir
#35 entity-load-conditions-begone-1184272-35-interdiff.txt1.11 KBBerdir
#32 entity-load-conditions-begone-1184272-32.patch104.82 KBBerdir
#32 entity-load-conditions-begone-1184272-32-interdiff.txt12.49 KBBerdir
#30 entity-load-conditions-begone-1184272-30.patch96.57 KBBerdir
#30 entity-load-conditions-begone-1184272-30-interdiff.txt13.79 KBBerdir
#28 entity-load-conditions-begone-1184272-27.patch93.78 KBBerdir
#25 entity-load-conditions-begone-1184272-25.patch75.39 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Alternatively, we could just remove the feature from the controller, but re-implement it as a EFQ inside the entity_load() wrapper.

catch’s picture

Title: Remove deprecated $conditions support from entity_load() » Remove deprecated $conditions support from entity controller

That might be a better idea, it's a fairly limited feature but people do use it.

I've had vague plans for trying to approximate that in entitycache for D7, but never got around to it (and it'd still be in the controller, just using EFQ in that case).

fago’s picture

One thing the conditions parameter allows us to do, what EFQ can't is evaluating conditions on cached entities, thus returning the results without any query.

Anyway, re-implementing various conditions in PHP doesn't fly, but still it could be useful to have conditions part of loading operation, so controllers could cache frequently used conditions. Use-case example: load a profile entity for a user.

>Alternatively, we could just remove the feature from the controller, but re-implement it as a EFQ inside the entity_load() wrapper.

Makes sense! So what about re-implementing it the same way inside the controller and off-load condition evaluation to a separate method that runs before entities are loaded by id?
That way entity-types could plugin there and customize condition evaluation to cache some of those.

catch’s picture

One thing the conditions parameter allows us to do, what EFQ can't is evaluating conditions on cached entities, thus returning the results without any query.

It only allows for this if you pass both $ids and $conditions iirc (which is not a very useful feature IMO), if you pass $conditions only, there is no way to know if all possible items are in the cache already, so it falls through to the database whatever happens.

For the latter case, the EFQ would return IDs for stuff that are already cached, but this is the only real change in behaviour and very negligible I think.

Also as the person who wrote a lot of the 'check conditions for cached entities' code, it is pretty ugly in there, even if it seemed cool at the time, and it has some wonky assumptions about case insenstive handling of usernames etc. (can't remember if it tries it's best in PHP or punts, but either way is not ideal).

Makes sense! So what about re-implementing it the same way inside the controller and off-load condition evaluation to a separate method that runs before entities are loaded by id?
That way entity-types could plugin there and customize condition evaluation to cache some of those.

That sounds good, as long as we can separate loading from IDs and conditions I'll be pretty happy with how this issue turned out, and other details we can always resolve later when refactoring the controller system more extensively.

But I'm still not sure about having this in entity_load() - entity_load($type, array(), array('condition' => 'value')); isn't pretty and people complained about it when we introduced it.

At the moment I'd go for this:

entity_load($entity_type, $ids);

entity_load_revision($entity_type, $id, $vid);

entity_load_conditions($entity_type, $conditions);

Controller gets a ->resolveConditions() method.

Add a EntityRevisionController() - hopefully we can clean this all up sufficiently that it could usefully inherit from the main class and just change base table and field API calls, although that needs some refactoring of the node schema which is another issue so might have to be left 'til later.

fago’s picture

It only allows for this if you pass both $ids and $conditions iirc (which is not a very useful feature IMO), if you pass $conditions only, there is no way to know if all possible items are in the cache already, so it falls through to the database whatever happens.

That's the way the default controller works, yes. But the current system allows other controllers to take that over. I've implemented that in the entity API controller for exportables, i.e. it skips the database part if all entities are cached.

That sounds good, as long as we can separate loading from IDs and conditions I'll be pretty happy with how this issue turned out, and other details we can always resolve later when refactoring the controller system more extensively.

Yep, we definitely want to separate that :)

At the moment I'd go for this:

entity_load($entity_type, $ids);

entity_load_revision($entity_type, $id, $vid);

entity_load_conditions($entity_type, $conditions);

Controller gets a ->resolveConditions() method.

Yep, sounds good. Though we should take #1160566: entity_load() is actually entity_load_multiple() into account. So what about

entity_load($entity_type, $id);

entity_load_multiple($entity_type, $ids);

entity_load_revision($entity_type, $id, $vid);

entity_load_by_condition($entity_type, $conditions);

Do we need a "load single by condition"? I don't think so as usually when this is needed there should be only one result anyway. In other cases one can still directly use efq + entity_load().

catch’s picture

All of this sounds good.

I would change:

entity_load_by_condition($entity_type, $condition);

to

entity_load_by_properties($entity_type, $properties);

that should make it more clear that it's not a replacement for EntityFieldQuery.

Do we need a "load single by condition"? I don't think so as usually when this is needed there should be only one result anyway. In other cases one can still directly use efq + entity_load().

It would save people having to do a reset($entities); themselves. MongoDb has a findOne() which does this - means "get me the first result that matches this query, don't care which one it is". This would apply for taxonomy_get_term_by_name(), user_load_by_name() and some other helpers, so it may be worth adding.

However entity_load_one_by_properties() is getting a bit silly for function names.

fago’s picture

entity_load_by_properties() sounds good.

>However entity_load_one_by_properties() is getting a bit silly for function names.
Yep. So my thought was to avoid introducing to much helpers with clumsy names, as we usually have dedicated wrapper functions for properties like term name and user name anyway.
It's just a thought, so I'd be ok with either way.

catch’s picture

Issue tags: +API change

Tagging.

Everett Zufelt’s picture

Everett Zufelt’s picture

I'm curious if this issue is dependent on other work going on in the Entity system, or if it is worth me working on. I'd like to understand the system more, and this would give me something to work on while learning.

fago’s picture

Priority: Major » Normal
Issue tags: -Entity system, -API clean-up

No, there is nothing we need to wait for here. So let's move on with this.

As suggested, let's go with

entity_load_revision($entity_type, $id, $vid);
entity_load_by_properties($entity_type, $values);

Question:
As properties might be translatable (in future), should we also add an optional language parameter to be used when querying translatable properties? I had a short look at the existing usages of $condition and found not many candidates for setting a language, but some as querying for a term with a given name.

Thus, let's do
entity_load_by_properties($entity_type, $values, $langcode = NULL);
whereas NULL just means query in defaulft entity language?

fago’s picture

oh, in the light of #1612014: Create an interface for revisionable entities we should probably go with
entity_load_version($entity_type, $id, $version_id);
instead.

catch’s picture

Won't language be a property? I'd think we could add that as a condition when required rather than as a separate parameter.

When I looked at this last, I was also wondering about just entity_load_version($entity_type, $version_id); - since we don't really need the ID there to look it up - assuming we require that $version_id is always a unique key which seems a reasonable requirement.

fago’s picture

Won't language be a property? I'd think we could add that as a condition when required rather than as a separate parameter.

True, but it means something else then. There is difference between looking up a node with X as default language and a node that has Y properties in language X, possibly being a translation as well.

When I looked at this last, I was also wondering about just entity_load_version($entity_type, $version_id); - since we don't really need the ID there to look it up - assuming we require that $version_id is always a unique key which seems a reasonable requirement.

Sounds reasonable to me as well.

sun’s picture

Priority: Normal » Major
Issue tags: +Entity system, +API clean-up

Bumping this to major, since

1) #1637370: Add UUID support to core entity types was a pain to implement with EFQ, whereas using the $conditions would have been a one-line function call.

2) It would be a huge shame if the deprecated $conditions would still exist in D8 and thus be "deprecated" for two major versions of Drupal core.

sun’s picture

Priority: Normal » Major
Issue tags: +Entity system, +API clean-up, +UUID
fago’s picture

Issue tags: +sprint
catch’s picture

Agreed on this being major, it also blocks the entity caching cleanup at #1596472: Replace hard coded static cache of entities with cache backends.

Berdir’s picture

entity_load_by_properties($entity_type, $values);

What exactly is $values supposed to be here, what is supported and what not? A simple $key => $value array and everything else needs custom EFQ queries? For example no support for additional conditions and so on?

pounard’s picture

I think what's supported are the base table and revision table fields only.

EDIT: Which is kind of heretic because we have multiple ways of querying entities depending on what we query, and that is both a consistency and DX problem, and makes it harder to both optimize the process it and unify the APIs.

fago’s picture

I think we want to move the query to efq and support every property there. Imo it should use = or IN as operation depending on whether there can be multiple values. For everything else, use efq.

Ideally, efq would so easy to use that we need that helper anymore. As of now, it's not, so I think it's best to do the helper now. If efq becomes that easy afterwards, we can still drop it.

catch’s picture

Yeah I agree with fago. If we add the helper we can ditch it later, but at least everything will be going through efq + entity_load() regardless of whether it's used or not.

Berdir’s picture

Assigned: Unassigned » Berdir

Working on this.

Berdir’s picture

What about $ids = FALSE, which loads all entities? This bypasses the cache currently. It would actually be supported with entity_load_by_properties($entity_type, array()) out of the box, but that's a bit non-intuitive. It's an uncommon and for most entities problematic use case though.

Berdir’s picture

I'm not 100% convinced about the function name. entity_load_multiple_by_properties() would make more sense, and be in line with e.g. taxonomy_term_load_multiple_by_name() (array of results) and user_load_by_mail() (single result).

Anyway, here is a patch that should implement this, with the currently suggested name.

I extended EntityStorageControllerInterface with a loadRevision($revision_id) and loadByProperties($values). While there are now two more methods, I think this makes much more sense. Instead of one method that tries to do three different things, we have three different methods that each do only one thing.

EntityTestStorageController caused me a bit of a headache, I implemented it with a db_select() query for now. Once EntityFieldQuery() supports translatable properties, we can just remove that again.

Let's see what the tests have to say...

Berdir’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, entity-load-conditions-begone-1184272-25.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
93.78 KB

Hm. It would help if I'd be able to type entity (hint: entitiy is wrong) and would save all files that I have open in my browser. Let's try that again.

Status: Needs review » Needs work

The last submitted patch, entity-load-conditions-begone-1184272-27.patch, failed testing.

Berdir’s picture

Fixed a number of bugs, renamed node_load_revision() to node_revision_load() to be able to %node_revision in the menu callbacks (isn't that nice, no more load arguments...) and added a return to entity_load_revision() (that does help ;))

Status: Needs review » Needs work

The last submitted patch, entity-load-conditions-begone-1184272-30.patch, failed testing.

Berdir’s picture

Ok, I think this patch might actually pass.

Noticed two things that I opened separate issues/follow-ups:

- #1723774: user_delete_multiple() does not use UserStorageController
- #1723784: Add file_load_multiple_by_uri($uri)

plach’s picture

+++ b/core/modules/entity/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestStorageController.php
@@ -48,7 +48,8 @@ class EntityTestStorageController extends DatabaseStorageController {
       $entity_fields = $this->entityInfo['schema_fields_sql']['base table'];
       $query->addField('data', $this->idKey);
       foreach ($values as $field => $value) {
-        $table = isset($entity_fields[$field]) ? 'base' : 'data';
+        // Check on which table the condition needs to be added.
+        $table = in_array($field, $entity_fields) ? 'base' : 'data';
         $query->condition($table . '.' . $field, $value);
       }
     }

Didn't have the time to look at the patch yet, just a quick thing: what about an array_flip here so that we can use isset in the loop?

Status: Needs review » Needs work

The last submitted patch, entity-load-conditions-begone-1184272-32.patch, failed testing.

Berdir’s picture

Had to reverse the logic anyway to fix the entity translation tests. langcode exists in both tables and it should only fallback to base if it's not in data.

sun’s picture

re: #30: If you'd do the same with entity_load_revision() » entity_revision_load(), then that could be used as menu argument loader, too. ;) [supplying $entity_type via 'load arguments']

Berdir’s picture

Makes sense, thought about that as well but then forgot it.

Berdir’s picture

Wanted to upload the interdiff, not the patch 2x.

plach’s picture

Status: Needs review » Needs work

Very very nice clean-up: we should try to get it in ASAP!

+++ b/core/modules/entity/entity.module
@@ -184,11 +184,31 @@ function entity_extract_ids($entity_type, $entity) {
+ * @return object

I think this should be Drupal\entity\EntityInterface.

+++ b/core/modules/entity/entity.module
@@ -260,29 +267,39 @@ function entity_load_by_uuid($entity_type, $uuid, $reset = FALSE) {
+ *   An associative array of properties of the entity, where
+ *   the keys are the property names and the values are the values those
+ *   properties must have.

+++ b/core/modules/entity/lib/Drupal/entity/DatabaseStorageController.php
@@ -219,6 +209,63 @@ class DatabaseStorageController implements EntityStorageControllerInterface {
+   *   An associative array of properties of the entity, where
+   *   the keys are the property names and the values are the values those
+   *   properties must have.

PHP docs do not wrap correctly.

+++ b/core/modules/entity/lib/Drupal/entity/DatabaseStorageController.php
@@ -193,13 +183,13 @@ class DatabaseStorageController implements EntityStorageControllerInterface {
+      $this->attachLoad($queried_entities);

I was wondering whether it would make sense to add also an attachLoadRevision() method.

+++ b/core/modules/entity/lib/Drupal/entity/DatabaseStorageController.php
@@ -219,6 +209,63 @@ class DatabaseStorageController implements EntityStorageControllerInterface {
+  public function loadRevision($revision_id) {

Wouldn't it make sense to cache also revisions (separately) now that we have a new shiny method?

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeLoadMultipleTest.php
@@ -59,30 +59,5 @@ class NodeLoadMultipleTest extends NodeTestBase {
-    // Load nodes by nid, where type = article. Nodes 1, 2 and 3 will be loaded.
-    $nodes = node_load_multiple(array(1, 2, 3, 4), array('type' => 'article'));
-    $count = count($nodes);
-    $this->assertTrue($count == 3, t('@count nodes loaded', array('@count' => $count)));
-    $this->assertEqual($nodes[$node1->nid]->label(), $node1->label(), t('Node successfully loaded.'));
-    $this->assertEqual($nodes[$node2->nid]->label(), $node2->label(), t('Node successfully loaded.'));
-    $this->assertEqual($nodes[$node3->nid]->label(), $node3->label(), t('Node successfully loaded.'));
-    $this->assertFalse(isset($nodes[$node4->nid]));
-
-    // Now that all nodes have been loaded into the static cache, ensure that
-    // they are loaded correctly again when a condition is passed.
-    $nodes = node_load_multiple(array(1, 2, 3, 4), array('type' => 'article'));
-    $count = count($nodes);
-    $this->assertTrue($count == 3, t('@count nodes loaded.', array('@count' => $count)));
-    $this->assertEqual($nodes[$node1->nid]->label(), $node1->label(), t('Node successfully loaded'));
-    $this->assertEqual($nodes[$node2->nid]->label(), $node2->label(), t('Node successfully loaded'));
-    $this->assertEqual($nodes[$node3->nid]->label(), $node3->label(), t('Node successfully loaded'));
-    $this->assertFalse(isset($nodes[$node4->nid]), t('Node was not loaded'));
-
-    // Load nodes by nid, where type = article and promote = 0.
-    $nodes = node_load_multiple(array(1, 2, 3, 4), array('type' => 'article', 'promote' => 0));
-    $count = count($nodes);
-    $this->assertTrue($count == 1, t('@count node loaded', array('@count' => $count)));
-    $this->assertEqual($nodes[$node3->nid]->label(), $node3->label(), t('Node successfully loaded.'));

Shouldn't this be replaced by some tests covering entity_load_by_properties()?

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -163,7 +163,8 @@ abstract class WebTestBase extends TestBase {
   function drupalGetNodeByTitle($title, $reset = FALSE) {
-    $nodes = node_load_multiple(array(), array('title' => $title), $reset);
+    entity_get_controller('node')->resetCache();

It seems this is ignoring the $reset parameter.

plach’s picture

Started work to enable multilingual property conditions in EFQ at #1691952: Make EntityFieldQuery work with multilingual properties. As discussed with @Berdir, that will allow (among the rest) to remove the custom solution introduced in EntityTestStorageController::loadByProperties().

Berdir’s picture

- Fixed wrapping of docs and @return object, I copied that from entity_load() and fixed it there as well.

- attachLoadRevision: Yes, thought about how to do that. We need to be carefull to not change too much here. I changed it to a boolean $load_revision argument now, it doesn't make sense to pass a single $revision_id if there are theoretically multiple entities. I think we could remove the argument and rely on EntityInterface::isCurrentRevision() instead, that would make more sense but it would require more changes as we need to check that for each entity. Follow-up?

- revision caching: Same here, there are issues that are trying to re-work entity caching and make it separately pluggable. Let's discuss that there.

- Those removed tests are specifically for a combination of ids *and* conditions. This is no longer supported. If you look at the changes, I have not found a single use case in core where it's not actually easier to understand what the code is doing without that "feature" outside of tests, for example:

+++ b/core/modules/user/user.pages.incundefined
@@ -122,9 +122,9 @@ function user_pass_reset($form, &$form_state, $uid, $timestamp, $hashed_pass, $a
-    // Some redundant checks for extra security ?
-    $users = user_load_multiple(array($uid), array('status' => '1'));
-    if ($timestamp <= $current && $account = reset($users)) {
+    $account = user_load($uid);
+    // Verify that the user exists and is active.
+    if ($timestamp <= $current && $account && $account->status) {

I've therefore removed all tests for this.

- Fixed the $reset argument. One thing that I've considered as well is only to reset the specified id when passing $reset to entity_load(). It is madness to clear the whole cache, especially when using entitycache. But again, patch is big enough already...

Status: Needs review » Needs work

The last submitted patch, entity-load-conditions-begone-1184272-41.patch, failed testing.

Berdir’s picture

Hah, now that $reset works correctly it actually shows that it never had any effect because it used $conditions. Set it to TRUE where in those two cases, this should pass.

plach’s picture

Changes look good and the answers in #41 are totally satisfying, we may want to start creating the missing follow up issues. Overall this looks very good to me, I'll wait for a while to mark it RTBC to give the people involved in the discussion above a chance to review the latest patch.

Berdir’s picture

I still have two questions that we should decide on:

- entity_load_by_properties() or entity_load_multiple_by_properties()?
While the second is a bit longer, I think it is technically correct and it's possible that the entity_load_by_properties() suggestion dates back to pre-entity_load_multiple() times.

- If entity_load_multiple($type, FALSE) should be a supported use case or not? Note that with the current patch, ..by_properties($entity_type, array()) would result in the same, in way that can be cached.

fago’s picture

- entity_load_by_properties() or entity_load_multiple_by_properties()?

I'd prefer entity_load_by_properties() - loading by property is ususally multiple anyway. entity_load_multiple_by_properties() is very verbose. But I guess that should depend on whether we want have a single-variant as well?

My take is that there are not many properties that are unique, thus where the single-load would make much sense. For stuff like UUID or user names we have separate helper functions anyway.

Ideally, we'd just have a getFirst() method or so on (simple-)EFQ.

sun’s picture

Heh. Just wanted to comment, and it looks like I disagree with @fago ;)

If it returns multiple, then the function name should contain "multiple". KISS. :)

Furthermore, on #45.2, I wondered whether we can replace that FALSE argument value entirely by making it optional?

This argument handling always looked very strange to me. I.e., what I mean is:

  // Load all entities of this type.
  $entities = entity_load_multiple($type);

i.e., with the function signature:

function entity_load_multiple($entity_type, array $ids = NULL, $reset = FALSE) {
plach’s picture

+100 for #47.2: looks like a far better DX.

Berdir’s picture

Yes, but as I said, the problem there isn't function signature, it's performance. a) it's not cached and b) is it a pattern that you should not be using unless you know exactly what you are doing. See http://www.metaltoad.com/blog/drupals-big-data-problem

The only place where core is currently doing that are vocabularies.

When going through by_properties(), we can first fetch the primary keys and then load them explicitly, with entity cache support. We could even add caching for the EFQ query in loadByProperties() so that multiple calls to that can be cached as well.

Any way, it's probably not something that we should be doing here and if we keep the behavior, then allowing it without an explicit FALSE would be fine for me as well.

pounard’s picture

If it returns multiple, then the function name should contain "multiple". KISS. :)

I actually disagree because it's obivious that loading by properties will return a result set and not a single result. The multiple infix seems visual garbage to me, and names such as load() (for single), loadByProperties(array|Traversable $properties) (for multiple) and loadAll(array|Traversable $identifiers) are explicit enough by themselves and sufficient.

plach’s picture

Status: Needs review » Needs work

Please, let's not block this patch on the multiple vs single bikeshed.

@Berdir:

Do you feel the other concern has been addressed? If so could you roll an updated patch?

Berdir’s picture

Talked with @fago in IRC, convinced him that multiple makes more sense.

@pounard: It's IMHO mostly about consistency. Right now, all load functions that return an array of results are called load_multiple, so we should do the same here.

Re-rolled with that and also defaulting $ids in load() to FALSE so that you don't need to explicitly specify it.

Status: Needs review » Needs work

The last submitted patch, entity-load-conditions-begone-1184272-52.patch, failed testing.

fago’s picture

Well, I still think we won't need multiple - #50 gets it. But I don't want to bikeshed on that and I'm fine with whatever seems to be the most common.

Yes, but as I said, the problem there isn't function signature, it's performance. a) it's not cached and b) is it a pattern that you should not be using unless you know exactly what you are doing. See http://www.metaltoad.com/blog/drupals-big-data-problem

Yes. I think it's nice to have, but it should probably do a LIMIT 100 or similar by default.

Berdir’s picture

Had to change all load multiple functions default value to FALSE, this should pass. Still not very fond of that. a limit 100 wouldn't be a default, it would be hardcoded, there's no way to change it either with load or loadByProperties() ?

catch’s picture

If vocabularies move to configuration we'll have no use-cases for the 'load everything' in core and could rip that out. Until that happens it should probably stay although we could for now add a big warning to the phpdoc?

sun’s picture

Actually, the current proposal is to turn all Configurable Thingies™ into configuration entities, so I guess we'll have many more use-cases in the end. We should definitely look into pager capabilities as a separate follow-up, but I guess that most of the configuration entity types will just be listed on a single page (since there cannot reasonably be that many).

@Berdir: Any particular reason for why you went with FALSE instead of array $ids = NULL, as proposed in #47? I re-read the last comments, but it wasn't really explained. The benefit of the latter is that 1) the argument must be an array, 2) a bogus FALSE throws an error, 3) NULL or omitting the argument is more natural and self-descriptive than passing FALSE.

Berdir’s picture

I went with FALSE because that's what is already used, so I just had to change the default value. changing to NULL would mean that we need to change documentation and so on as well.

I can change it to NULL if that's desired.

I still think this shouldn't be supported. If you want to use pager or sorting or more complex conditions, you should use EFQ (which should provide an easier way to load the entities). IMHO, entity_load_multiple() should be limited to load a set of entities based on their id's.

sun’s picture

FileSize
9.37 KB
116.05 KB

Sorry, I didn't want to "stuck" this.

Status: Needs review » Needs work

The last submitted patch, entity-conditions.59.patch, failed testing.

Berdir’s picture

Removed the assertion that failed, this is a feature now ;)

Status: Needs review » Needs work
Issue tags: -Entity system, -API change, -API clean-up, -UUID, -sprint

The last submitted patch, entity-load-conditions-begone-1184272-61.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +Entity system, +API change, +API clean-up, +UUID, +sprint
sun’s picture

Status: Needs review » Reviewed & tested by the community

Sorry, thanks! I wasn't sure whether that test actually expected no results (== FALSE), or whether it was really asserting the FALSE parameter... ;)

Looks ready to fly now! :)

That said, I wonder whether we want to postpone this commit to after the entity form controller has landed? Despite being large, this patch looks easier to re-roll in case of conflicts...?

Also, let's make sure to create a follow-up issue for the optional first argument for *_load() functions; e.g., node_load($id = NULL, ...) -- since that makes no sense at all (as it ends up as array(NULL) argument to _load_multiple())...

plach’s picture

There is no overlap: the two patches apply cleany together :)

Berdir’s picture

@sun: $nid = NULL is still possible because this is still supported: node_load(NULL, TRUE) which is the same as entity_get_controller('node')->resetCache(); Yes, you need to explicitly specify it anyway, but what we should actually do is drop support for that.

Which means either of those two possibilities:
A) Remove $reset arguments completely, probably provide a entity_reset($entity_type, $ids = array()) helper function instead.
B) Change $reset on single load function to only reset the the provided $nid, essentially making it the same as entity_load_unchanged().

Both would be ok IMHO. And in both cases, $nid would be a required argument and NULL wouldn't be allowed anymore.

fago’s picture

Status: Reviewed & tested by the community » Needs work

Great work - this cleans things up *a lot*!

+++ b/core/includes/form.inc
@@ -4629,7 +4629,8 @@ function _form_set_class(&$element, $class = array()) {
+ *   $nodes = entity_load_by_property('node', array('uid' => $uid, 'type' => $type));
+ *   $node = reset($nodes);

Old function naming? This function is not defined, so this cannot work. Looks like that's not covered by tests?

+++ b/core/modules/entity/lib/Drupal/entity/EntityStorageControllerInterface.php
@@ -42,13 +42,34 @@ interface EntityStorageControllerInterface {
+  /**
+   * Load entities by their properties.

This should say, by "property values".

The differentation between property and property value will become in particular important with the property api, where "property" means property object.

+++ b/core/modules/entity/lib/Drupal/entity/EntityStorageControllerInterface.php
@@ -42,13 +42,34 @@ interface EntityStorageControllerInterface {
+   *   An associative array of properties of the entity, where the keys are the
+   * property names and the values are the values those properties must have.

wrong indentation

Berdir’s picture

Fixed those things, the broken call was documentation, can't have test coverage ;)

The old line had exactly the same problem, that code would have never worked in 8.x nor 7.x.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Looks like @fago's remarks were addressed so back to RTBC.

fago’s picture

lol, thanks berdir.

ad #69: yep, so let's get this great patch in!

Which means either of those two possibilities:
A) Remove $reset arguments completely, probably provide a entity_reset($entity_type, $ids = array()) helper function instead.
B) Change $reset on single load function to only reset the the provided $nid, essentially making it the same as entity_load_unchanged().

Imo, just calling entity_get_controller($entity_type)->resetCache(); would do it. Usually calling reset manually shouldn't be required anyway as our save() and delete() operations already care about it. So it's mostly tests.

Berdir’s picture

Yes, there are 4 calls that pass TRUE to node_load() outside of the tests (56 with them included...), 3 within node.module and another strange documentation thingy in form.inc.

$ grep -R "node_load" core/ | grep "TRUE" | grep -v Tests
core/includes/form.inc: *     $node = node_load($row->nid, NULL, TRUE);
core/modules/node/node.admin.inc:  $node = node_load($nid, NULL, TRUE);
core/modules/node/node.module:        $node = node_load($nid, NULL, TRUE);
core/modules/node/node.module:  $nodes = node_load_multiple($nids, array(), TRUE);

You didn't say if you prefer A or B, though :)

fago’s picture

Oh sry, clearly A. Reseting should not be needed. If somethiing, then there should be entity_reset_cache() or so.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Here's a review. I'm not positive all of these are legit, so only downgrading to "needs review" not "needs work."

+++ b/core/includes/file.incundefined
@@ -580,27 +580,19 @@ function file_save_htaccess($directory, $private = TRUE) {
-function file_load_multiple($fids = array(), array $conditions = array()) {
-  return entity_load_multiple('file', $fids, $conditions);
+function file_load_multiple(array $fids = NULL) {

(here, and elsewhere)

$fids = NULL? not $fids = array()? That's a little strange. Is there a reason we don't check for empty array rather than NULL?

+++ b/core/modules/entity/entity.moduleundefined
@@ -220,31 +224,40 @@ function entity_load_by_uuid($entity_type, $uuid, $reset = FALSE) {
+ * @param array $values
+ *   An associative array of properties of the entity, where the keys are the
+ *   property names and the values are the values those properties must have.
...
+function entity_load_multiple_by_properties($entity_type, array $values) {

I find $values to be a very strange name for this argument. $properties makes more sense to me. I'm not sure why this was changed.

+++ b/core/modules/entity/lib/Drupal/entity/DatabaseStorageController.phpundefined
@@ -144,19 +144,9 @@ class DatabaseStorageController implements EntityStorageControllerInterface {
+  public function load($ids = NULL) {

(here, and elsewhere)

Should we type this as an array like elsewhere?

+++ b/core/modules/entity/lib/Drupal/entity/DatabaseStorageController.phpundefined
@@ -174,11 +164,11 @@ class DatabaseStorageController implements EntityStorageControllerInterface {
-    // is set to FALSE (so we load all entities), if there are any ids left to
-    // load, if loading a revision, or if $conditions was passed without $ids.
-    if ($ids === FALSE || $ids || $revision_id || ($conditions && !$passed_ids)) {
+    // is set to NULL (so we load all entities) or if there are any ids left to
+    // load.
+    if ($ids === NULL || $ids) {
...
-      $query_result = $this->buildQuery($ids, $conditions, $revision_id)->execute();
+      $query_result = $this->buildQuery($ids)->execute();

Well that certainly got a lot simpler. :)

But wait, didn't we just remove some flexibility here? It doesn't look like you can load a specific revision ID anymore.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeLoadMultipleTest.phpundefined
@@ -59,30 +59,5 @@ class NodeLoadMultipleTest extends NodeTestBase {
-
-    // Load nodes by nid, where type = article. Nodes 1, 2 and 3 will be loaded.
-    $nodes = node_load_multiple(array(1, 2, 3, 4), array('type' => 'article'));
-    $count = count($nodes);
-    $this->assertTrue($count == 3, t('@count nodes loaded', array('@count' => $count)));
-    $this->assertEqual($nodes[$node1->nid]->label(), $node1->label(), t('Node successfully loaded.'));
-    $this->assertEqual($nodes[$node2->nid]->label(), $node2->label(), t('Node successfully loaded.'));
-    $this->assertEqual($nodes[$node3->nid]->label(), $node3->label(), t('Node successfully loaded.'));
-    $this->assertFalse(isset($nodes[$node4->nid]));
-
-    // Now that all nodes have been loaded into the static cache, ensure that
-    // they are loaded correctly again when a condition is passed.
-    $nodes = node_load_multiple(array(1, 2, 3, 4), array('type' => 'article'));
-    $count = count($nodes);
-    $this->assertTrue($count == 3, t('@count nodes loaded.', array('@count' => $count)));
-    $this->assertEqual($nodes[$node1->nid]->label(), $node1->label(), t('Node successfully loaded'));
-    $this->assertEqual($nodes[$node2->nid]->label(), $node2->label(), t('Node successfully loaded'));
-    $this->assertEqual($nodes[$node3->nid]->label(), $node3->label(), t('Node successfully loaded'));
-    $this->assertFalse(isset($nodes[$node4->nid]), t('Node was not loaded'));
-
-    // Load nodes by nid, where type = article and promote = 0.
-    $nodes = node_load_multiple(array(1, 2, 3, 4), array('type' => 'article', 'promote' => 0));
-    $count = count($nodes);
-    $this->assertTrue($count == 1, t('@count node loaded', array('@count' => $count)));
-    $this->assertEqual($nodes[$node3->nid]->label(), $node3->label(), t('Node successfully loaded.'));

Can you explain why all of these tests were removed?

+++ b/core/modules/node/node.moduleundefined
@@ -982,47 +982,47 @@ function node_invoke($node, $hook, $a2 = NULL, $a3 = NULL, $a4 = NULL) {
+function node_revision_load($vid = NULL) {

Does this function need a $reset param to match node_load()?

webchick’s picture

Also, I think it might be a good idea to try and backport these helper functions to D7 once this patch is ready. This would be a simple thing for people to start implementing now that would make the D7 => D8 transition smoother.

Berdir’s picture

Short feedback...

- array $ids = NULL is because it's valid to pass NULL to it (which loads all arguments), while array() would load nothing.

- $values vs. $properties: We pass property name => property value to it. So it's both. Agree that it's strange to have by_property() but $values, but I'm not sure what would be better. An array of $properties would be something like array('name', 'uid', 'label') to me, not property values. and $property_values is a bit long.

- NULL was previously FALSE, which did not work with the type cast array. NULL seems to work, so we can add the type cast there as well.

- A specific revision is now loaded with loadRevision(), the revision argument was completely removed from load(), that's why it's removed from there as well.

- Removed tests: quote from my comment #41: "Those removed tests are specifically for a combination of ids *and* conditions. This is no longer supported. If you look at the changes, I have not found a single use case in core where it's not actually easier to understand what the code is doing without that "feature" outside of tests". See #41 for an example.

- We want to completely remove $reset arguments for all load functions in a follow-up, it is used only 4x times in core, 3 inside node.module and one is an example. Saving an entity automatically clears the relevant caches, this is mostly specific to tests. Those will use entity_get_controller($type)-resetCache(), possibly a small helper function for that.

- I don't think the new functions can be backported, it only works reliable if all entities support it and it requires new methods on entity controllers, which could break existing implementations.

Berdir’s picture

Status: Needs review » Needs work
Issue tags: +Novice

Setting to needs work to consistently add an array type cast to all $ids arguments of load_multiple() functions.

corvus_ch’s picture

Status: Needs work » Needs review
FileSize
2.28 KB
118.06 KB

Added the last remaining missing array type hint and clarified the comment for EntityStorageControllerInterface::loadByProperties().

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/modules/entity/entity.moduleundefined
@@ -255,8 +255,8 @@ function entity_load_multiple($entity_type, array $ids = NULL, $reset = FALSE) {
- *   An associative array of properties of the entity, where the keys are the
- *   property names and the values are the values those properties must have.
+ *   An associative where the keys are the property names and the
+ *   values are the values those properties must have.

Missing array.

corvus_ch’s picture

FileSize
1.31 KB

Fixed missing word in comments.

corvus_ch’s picture

Status: Needs work » Needs review
FileSize
118.08 KB

Me again. Hopefully with the right patch this time.

corvus_ch’s picture

FileSize
118.08 KB

Make interface and implementation match.

corvus_ch’s picture

FileSize
2.55 KB

Here the interdiff to the patch from comment #68.

Berdir’s picture

Assigned: Berdir » Unassigned
Status: Needs review » Reviewed & tested by the community

Ok, tests are passing, Changes look good to me, back to @webchick.

catch’s picture

Could we use the old $conditions argument for the properties/keys thing maybe?

Also wondering if we could add an entity_load_by_properties() which just returns the first result from the result set - with that we possibly wouldn't need some of the helpers like user_load_by_name() but definitely a follow-up.

Otherwise this looks great to me - but I'll give some time for webchick to have another look.

catch’s picture

Priority: Major » Critical
Status: Reviewed & tested by the community » Active

OK I thought about this some more and discussed quickly with berdir in irc. I don't think we should hold this up over the argument name, so I've opened #1742850: Follow-up for entity_load_multiple_by_properties() to thrash it out some more.

Committed/pushed to 8.x. This will need a change notification.

David_Rothstein’s picture

I pointed this out in #1742850: Follow-up for entity_load_multiple_by_properties() but quickly mentioning it here too since I'm not sure that's the right issue:

-      $existing_files = file_load_multiple(array(), array('uri' => $uri));
+      $existing_files = entity_load_multiple_by_properties('file', array('uri' => $uri));

That change (and others like it) introduces a big API inconsistency. Why isn't it file_load_multiple_by_properties(array('uri' => $uri)) instead?

We can handle that in a followup, though.

Berdir’s picture

@David_Rothstein: There's already a follow-up for that: #1723784: Add file_load_multiple_by_uri($uri), not in the way you suggested though. I don't think we need a wrapper function for every entity type for that, we don't have node_create() either, for example. Instead, my suggestion there is to add specific functions for things that we need frequently, like loading a file by the URI.

pounard’s picture

It's probably more consistent to drop all wrappers and use the controller directly, i.e.: entity_get_controller('file')->loadByProperties(array('uri' => $uri));? The more entity type will be defined the more wrapper we will have in the global namespace, it's almost code duplication IMO.

tim.plunkett’s picture

Title: Remove deprecated $conditions support from entity controller » Change notification for: Remove deprecated $conditions support from entity controller
Berdir’s picture

Assigned: Unassigned » Berdir

Working on a change notice.

Berdir’s picture

Status: Active » Needs review

Ok, what about this: http://drupal.org/node/1744046?

catch’s picture

Priority: Critical » Major
Status: Needs review » Fixed

Looks good to me.

plach’s picture

Title: Change notification for: Remove deprecated $conditions support from entity controller » Remove deprecated $conditions support from entity controller
sun’s picture

Automatically closed -- issue fixed for 2 weeks with no activity.