Updated: Comment #11
Problem/Motivation
Currently, entity storage controllers have a load() method. However, it doesn't load an entity. It loads multiple entities as an array. You can't actually get a single entity out of a storage controller. That was all well and good if everyone was using entity_load()
(which was 3-4 layers of procedural calls to get to the storage controller).
Proposed resolution
Rename ->load()
to ->loadMultiple()
, add a ->load()
method that just sub-calls to ->loadMultiple()
like entity_load()
does, and update any current calls to ->load()
as needed.
Remaining tasks
- Replace and deprecate
entity_load_multiple()
andentity_load()
with callings to entity storage controllers methods:->loadMultiple()
and->load()
.
User interface changes
No UI changes.
API changes
- All entity storage controllers method
->load()
will be replaced by the method->loadMultiple()
. No changes made to arguments. - For retrieving a single entity, entity storage controllers will call their new
->load()
method with the id as argument. - All
*_load()
functions derived fromentity_load()
and also the menu loaders (*_load()
) will returnNULL
instead ofFALSE
in case of failure to retrieve the object.
Related Issues
N/A
Original report by Crell
Currently, entity storage controllers have a load() method. However, it doesn't load an entity. It loads multiple entities as an array. You can't actually get a single entity out of a storage controller. That was all well and good if everyone was using entity_load() (which was 3-4 layers of procedural calls to get to the storage controller), but we want to stop doing that. That means we need to make load() actually do what you'd expect.
Fortunately that's easy. Rename load() to loadMultiple(), add a load() method that just sub-calls to loadMultiple() like entity_load() does, and update any current calls to load() as needed. Simple!
Someone please beat me to it. :-) I'll try to find time but make no promises.
Comment | File | Size | Author |
---|---|---|---|
#61 | drupal-split-load-2024833-61.patch | 2.39 KB | claudiu.cristea |
#54 | broken-head-2024833-54.patch | 1.67 KB | tim.plunkett |
#49 | drupal-split-load-2024833-49.patch | 75.04 KB | claudiu.cristea |
#47 | drupal-split-load-2024833-47.patch | 75.1 KB | claudiu.cristea |
#47 | interdiff.txt | 9.68 KB | claudiu.cristea |
Comments
Comment #1
claudiu.cristeaLet's try...
Comment #2
claudiu.cristeaHere's a first patch. Didn't manage to run many tests locally.
Comment #4
claudiu.cristeaFixed. Retrying...
Comment #5
Crell CreditAttribution: Crell commentedThis should return NULL, not FALSE. See http://www.garfieldtech.com/blog/empty-return-values for why. ;-)
I'm not fully up on EntityListController, but this just looks suspicious.
Comment #6
claudiu.cristeaI agree. That was how I would implement but I "copied" from the old
entity_load()
:$this->storage
is an instance of\Drupal\Core\Entity\EntityStorageControllerInterface
. So it seems OK.Comment #7
claudiu.cristeaHere we go again...
Comment #9
claudiu.cristeaHmm. Now, returning
NULL
instead ofFALSE
we've ran in the next issue: Asentity_load()
has been refactored to use the new->load()
method it returnsNULL
instead ofFALSE
, as it used till now. This breaks things on the site. I retested failed tests with the oldentity_load()
and they passed.I can see 2 main options (with some variations):
entity_load()
returningFALSE
(notNULL
) if no entity found. This is kind of dirty solution but it's effective.entity_load()
call (~282 calls) and refactor to acceptNULL
as returned value. This would be the right approach. I see here 2 sub-options:entity_load()
entity_load()
in favour of->load()
but, here, I don't know what are the core team plans.Comment #10
claudiu.cristeaComment #11
Crell CreditAttribution: Crell commentedI don't think entity_load() is slated to be removed, but it should at least be @deprecated. (Same for entity_load_multiple()).
Since there's only 6 fails and 4 exceptions, I'd say let's just switch to NULL and fix anywhere that breaks. NULL and FALSE both evaluate to boolean FALSE on a weak check, so most code apparently can't tell the difference.
Comment #11.0
claudiu.cristeaIssue summary update.
Comment #12
claudiu.cristeaI narrowed the error zone and it seems they are triggered by the menu system, in fact when evaluating the menu loaders (
*_load()
functions). The menu system expects there a=== FALSE
in case of object missing.core/includes/menu.inc
My proposal is to replace
if ($return === FALSE)
withif (empty($return))
.This will fix those failures. I'm testing locally this solution right now.
Comment #13
Crell CreditAttribution: Crell commentedOf course it's the menu system... :-) +1 to #12.
Comment #14
claudiu.cristeaVoilà!
Comment #16
claudiu.cristeaDrupal\filter\Tests\FilterAdminTest->testFormatAdmin()
passes when testing manually with this latest patch. However running the automated test doesn't succeed locally. I cannot find any reason.Let's see what Drupal testing says.
Comment #18
claudiu.cristeaOh, forgot the editor.
Comment #19
Crell CreditAttribution: Crell commentedLooks OK to me. Thanks, Claudiu!
Comment #20
Berdir@inheritdoc.
@return is missing the type: Drupal\Core\Entity\EntityInterface.
Not sure about the NULL/FALSE change here. It might be a correct thing to do but it means we're doing two things at the same time here and it makes the patch considerable bigger, including a lot of unrelated changes in completely unrelated menu load callbacks. I would suggest to keep the existing behavior here and just add that method.
Comment #21
Crell CreditAttribution: Crell commentedBerdir: While I'd nominally agree with you, at this point the work has already been done and we're less than a week to freeze; I really don't want to ship another release with FALSE being used incorrectly. :-)
The docs changes should be easy enough to make.
Comment #22
claudiu.cristeaThank you @Berdir for review.
Updating doxygen lines I found that in
entity_load()
, when resetting the cache, I forgot to pass the$id
to->resetCache()
, in #18. This patch is fixing that. Even it's out of scope, I noticed thatentity_load_multiple()
does the same -- it invalidates the whole cache for that entity. This is not OK unless there's a special reason for doing that but I cannot get that point. I fixedentity_load_multiple()
too, seeinterdiff.txt
.Comment #23
claudiu.cristeaNeeds change notification.
Comment #24
Crell CreditAttribution: Crell commentedComment #25
alexpottNeeds a reroll...
Comment #26
claudiu.cristeaLet's see.
Comment #27
claudiu.cristeaDon't know what are the best practices when re-rolling. Should I set to "needs review" or to "reviewed & tested by the community" assuming that was previously RTBC?
Comment #29
claudiu.cristeaHEAD is advancing... :(
Comment #31
claudiu.cristea#29: drupal-split-load-2024833-29.patch queued for re-testing.
Comment #33
claudiu.cristeaThe test is failing before beginning. I opened a ticket here: #2029473: Test fail before starting tests.
Can anybody help with this?
Comment #34
alexpottSo I agree with @berdir in #20 we should be doing less change here... lets return FALSE and change to NULL in a followup... it will mean less rerolls.
Comment #35
claudiu.cristeaFinally I have to agree. It's too big to be handled.
Comment #36
alexpottSo in answer to the test fails... this patch currently breaks login :)
Comment #37
claudiu.cristeaOk but can you explain where? :)
Comment #38
claudiu.cristeaOk. Found :)
Are you OK to fix that and try again with both: load split & NULL?
Comment #39
claudiu.cristeaLet's try :)
Comment #40
claudiu.cristeaNow that it's green it needs a RTBC -- it was RTBC before reroll.
Comment #41
Crell CreditAttribution: Crell commentedThat's the downside of so much great work this week. Lots of patches colliding. :-)
Comment #42
alexpottNeeds reroll
Comment #43
claudiu.cristeaHere we go... :)
Comment #44
claudiu.cristeaForgot the patch :)
Comment #45
claudiu.cristeaNeeds a re-RTBC before it's not too late :)
Comment #46
jhodgdonI was just scanning this patch to avoid committing a conflict, and I happened to notice that right at the top, there are namespaces used in documentation that do not start with \ -- they MUST start with \ in documentation, or else leave out the namespace entirely (if it is clear from the context of the file you can probably just leave it out as you would in code).
Comment #47
claudiu.cristea@jhodgdon, Thank you for pointing out. I think that I inherited most them. Not an excuse, I should have been more careful.
I did some cleanup but only on docs related to this patch (and sometime a little bit around) because I wanted to keep the patch readable while it'a already big enough.
Comment #48
jhodgdonYes, most of them were inherited. I do not know why people are putting them into documentation all over the place. Very bad. Thanks for cleaning them up!
Comment #49
claudiu.cristeaThe patch fails to apply after last commits. Here's a rerolled version.
Comment #50
claudiu.cristea@Crell, desperately need a re-RTBC here. This HEAD race is gonna drive me crazy :)
Comment #51
alexpottMarked as major so this change is not subject to API freeze as we are just moving functionality from the procedural wrapper to the object. And this change will make working with entities an better developer experience. If we do not want to break BC then we can always just add a loadSingle method (nice idea @berdir).
We need to remove all unrelated change include the change of the return value to NULL and the unnecessary fixes to coding standards.
For example, this is out-of-scope
Comment #52
Crell CreditAttribution: Crell commentedAlex: I don't see why we need to back out the FALSE/NULL fix. It's done, we know we want to do it (lest we suffer another 3 years of a totally stupid and easily fixable API bug), and the only challenge here is really just commit conflicts. :-)
Fixing up docs along the way is what jhodgdon just asked be done, and it's something we've been doing. "Leave everything cleaner than you found it." I don't see any dead kittens here.
Comment #53
Dries CreditAttribution: Dries commentedThe patch in #49 actually applies, including the changes from FALSE to NULL. I decided to commit it to 8.x. Thanks all!
Comment #54
tim.plunkettComment #55
dawehnerThese seems to be the only places
Comment #56
Dries CreditAttribution: Dries commentedI just committed Tim's patch in #54, but based on #55, it looks like we may be missing two more.
Comment #57
neclimdulI think he meant we found all the locations since he RTBC'ed it. We'll see when https://qa.drupal.org/8.x-status comes back.
Comment #58
tim.plunkettThose other instances are on \Drupal\Core\Path\Path, so we don't need to worry. Thanks @Dries!
Comment #59
claudiu.cristeaIn fact you should use the new "load single" approach :)
EDITED!
instead of
reset()
stuff.Same here. The last lines of function should go as
Comment #60
tim.plunkettI was merely unbreaking HEAD, not getting fancy with it. I think that could be a follow-up issue, it's not urgent.
Comment #61
claudiu.cristea@tim.plunkett, thanks for fixing HEAD.
While I'm preparing a change notice for this issue, I leave a patch here for testing, reviewing and RTBC. Don't need a separate issue for this.
Comment #62
claudiu.cristeaAdded 2 change notices as they seems different API changes.
Comment #63
Crell CreditAttribution: Crell commented#61 looks correct to me.
Comment #64
claudiu.cristeaUntagging.
Comment #65
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #66
alexpottCommitted 3b0e0c5 and pushed to 8.x. Thanks!
Comment #67
amateescu CreditAttribution: amateescu commentedI corrected https://drupal.org/node/2032185 a bit, because there was no entity_load_multiple() in D7 and the loading code in D8 is much simpler, you just call entity_load() and entity_load_multiple().
https://drupal.org/node/2032185/revisions/view/2748581/2764149
Comment #68.0
(not verified) CreditAttribution: commentedTypo.