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.
Comment | File | Size | Author |
---|---|---|---|
#82 | 1184272-81-interdiff.txt | 2.55 KB | corvus_ch |
#81 | 1184272-81.patch | 118.08 KB | corvus_ch |
#80 | 1184272-80.patch | 118.08 KB | corvus_ch |
#79 | 1184272-79.patch | 1.31 KB | corvus_ch |
#77 | 1184272-77.patch | 118.06 KB | corvus_ch |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedAlternatively, we could just remove the feature from the controller, but re-implement it as a EFQ inside the entity_load() wrapper.
Comment #2
catchThat 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).
Comment #3
fagoOne 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.
Comment #4
catchIt 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).
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.
Comment #5
fagoThat'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.
Yep, we definitely want to separate that :)
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().
Comment #6
catchAll of this sounds good.
I would change:
to
that should make it more clear that it's not a replacement for EntityFieldQuery.
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.
Comment #7
fagoentity_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.
Comment #8
catchTagging.
Comment #9
Everett Zufelt CreditAttribution: Everett Zufelt commentedOpened #1373336: Remove deprecated $conditions support from User module
Comment #10
Everett Zufelt CreditAttribution: Everett Zufelt commentedI'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.
Comment #11
fagoNo, there is nothing we need to wait for here. So let's move on with this.
As suggested, let's go with
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?
Comment #12
fagooh, 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.
Comment #13
catchWon'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.Comment #14
fagoTrue, 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.
Sounds reasonable to me as well.
Comment #15
sunBumping 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.
Comment #16
sunComment #17
fagoComment #18
catchAgreed on this being major, it also blocks the entity caching cleanup at #1596472: Replace hard coded static cache of entities with cache backends.
Comment #19
BerdirWhat 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?
Comment #20
pounardI 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.
Comment #21
fagoI 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.
Comment #22
catchYeah 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.
Comment #23
BerdirWorking on this.
Comment #24
BerdirWhat 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.
Comment #25
BerdirI'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...
Comment #26
BerdirComment #28
BerdirHm. 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.
Comment #30
BerdirFixed 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 ;))
Comment #32
BerdirOk, 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)
Comment #33
plachDidn'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?
Comment #35
BerdirHad 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.
Comment #36
sunre: #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'
]Comment #37
BerdirMakes sense, thought about that as well but then forgot it.
Comment #38
BerdirWanted to upload the interdiff, not the patch 2x.
Comment #39
plachVery very nice clean-up: we should try to get it in ASAP!
I think this should be
Drupal\entity\EntityInterface
.PHP docs do not wrap correctly.
I was wondering whether it would make sense to add also an
attachLoadRevision()
method.Wouldn't it make sense to cache also revisions (separately) now that we have a new shiny method?
Shouldn't this be replaced by some tests covering
entity_load_by_properties()
?It seems this is ignoring the
$reset
parameter.Comment #40
plachStarted 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()
.Comment #41
Berdir- 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:
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...
Comment #43
BerdirHah, 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.
Comment #44
plachChanges 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.
Comment #45
BerdirI 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.
Comment #46
fagoI'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.
Comment #47
sunHeh. 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:
i.e., with the function signature:
Comment #48
plach+100 for #47.2: looks like a far better DX.
Comment #49
BerdirYes, 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.
Comment #50
pounardI 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 asload()
(for single),loadByProperties(array|Traversable $properties)
(for multiple) andloadAll(array|Traversable $identifiers)
are explicit enough by themselves and sufficient.Comment #51
plachPlease, 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?
Comment #52
BerdirTalked 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.
Comment #54
fagoWell, 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. I think it's nice to have, but it should probably do a LIMIT 100 or similar by default.
Comment #55
BerdirHad 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() ?
Comment #56
catchIf 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?
Comment #57
sunActually, 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.Comment #58
BerdirI 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.
Comment #59
sunSorry, I didn't want to "stuck" this.
Comment #61
BerdirRemoved the assertion that failed, this is a feature now ;)
Comment #63
Berdir#61: entity-load-conditions-begone-1184272-61.patch queued for re-testing.
Comment #64
sunSorry, 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 asarray(NULL)
argument to _load_multiple())...Comment #65
plachThere is no overlap: the two patches apply cleany together :)
Comment #66
Berdir@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.
Comment #67
fagoGreat work - this cleans things up *a lot*!
Old function naming? This function is not defined, so this cannot work. Looks like that's not covered by tests?
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.
wrong indentation
Comment #68
BerdirFixed 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.
Comment #69
plachLooks like @fago's remarks were addressed so back to RTBC.
Comment #70
fagolol, thanks berdir.
ad #69: yep, so let's get this great patch in!
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.
Comment #71
BerdirYes, 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.
You didn't say if you prefer A or B, though :)
Comment #72
fagoOh sry, clearly A. Reseting should not be needed. If somethiing, then there should be entity_reset_cache() or so.
Comment #73
webchickHere's a review. I'm not positive all of these are legit, so only downgrading to "needs review" not "needs work."
(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?
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.
(here, and elsewhere)
Should we type this as an array like elsewhere?
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.
Can you explain why all of these tests were removed?
Does this function need a $reset param to match node_load()?
Comment #74
webchickAlso, 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.
Comment #75
BerdirShort 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.
Comment #76
BerdirSetting to needs work to consistently add an array type cast to all $ids arguments of load_multiple() functions.
Comment #77
corvus_ch CreditAttribution: corvus_ch commentedAdded the last remaining missing array type hint and clarified the comment for EntityStorageControllerInterface::loadByProperties().
Comment #78
BerdirMissing array.
Comment #79
corvus_ch CreditAttribution: corvus_ch commentedFixed missing word in comments.
Comment #80
corvus_ch CreditAttribution: corvus_ch commentedMe again. Hopefully with the right patch this time.
Comment #81
corvus_ch CreditAttribution: corvus_ch commentedMake interface and implementation match.
Comment #82
corvus_ch CreditAttribution: corvus_ch commentedHere the interdiff to the patch from comment #68.
Comment #83
BerdirOk, tests are passing, Changes look good to me, back to @webchick.
Comment #84
catchCould 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.
Comment #85
catchOK 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.
Comment #86
David_Rothstein CreditAttribution: David_Rothstein commentedI 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:
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.
Comment #87
Berdir@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.
Comment #88
pounardIt'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.Comment #89
tim.plunkettComment #90
BerdirWorking on a change notice.
Comment #91
BerdirOk, what about this: http://drupal.org/node/1744046?
Comment #92
catchLooks good to me.
Comment #93
plachComment #94
sunCreated: #1757586: Remove MODULE_load*() & Co functions in favor of entity_*() functions