Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Posted by xjm
Problem/Motivation
(From #1346032-14: Ordering of hook_entity_delete() is inconsistent.)
- For most operations, the entity-specific hook is called first and the generic entity hook is second.
- In entity_load(), the generic entity hook is first. See DrupalDefaultEntityController::attachLoad().
Proposed resolution
- Change the hook order so that the type-specific hook is first.
Remaining tasks
- TBD
User interface changes
- The entity-type-specific load hook will now precede the generic entity load.
API changes
- TBD
Comment | File | Size | Author |
---|---|---|---|
#61 | entity_load_hooks_are-1371744-61.patch | 7.48 KB | kostyashupenko |
#55 | entity_load_hooks_are-1371744-55.patch | 7.48 KB | AjitS |
#49 | entity_load_hooks_are-1371744-49.patch | 7.48 KB | AjitS |
#41 | interdiff-1371744-39-41.txt | 3.35 KB | Lendude |
#41 | entity_load_hooks_called_wrong_order-1371744-41.patch | 7.51 KB | Lendude |
Comments
Comment #0.0
xjmUpdated issue summary.
Comment #1
aspilicious CreditAttribution: aspilicious commentedComment #2
aspilicious CreditAttribution: aspilicious commentedPrevious patch should fail. Let's try this one...
Comment #4
aspilicious CreditAttribution: aspilicious commentedHmmmm, let's try this one...
This doubles the rdf_mapping_load calls for each $comment. But it only does a lookup the second time.
Comment #5
aspilicious CreditAttribution: aspilicious commentedOk previous one is crap
Comment #6
scor CreditAttribution: scor commentedyes, rdf_mapping_load() is tapping into cached data, so should have no performance impact.
Comment #7
xjm#5: 1371744-attachload-order-5.diff queued for re-testing.
Comment #9
xjmThis will need to be updated for PSR-0.
Comment #10
xjmComment #11
jibranreroll of #5
Comment #12
jibranupdated status
Comment #14
jibran#11: 1371744-11.patch queued for re-testing.
Comment #15
jthorson CreditAttribution: jthorson commented#11: 1371744-11.patch queued for re-testing.
Comment #16
jthorson CreditAttribution: jthorson commentedUpdating tags
Comment #17
fago#11: 1371744-11.patch queued for re-testing.
Comment #18
jjchinquist CreditAttribution: jjchinquist commentedpatch fails on the newest dev version. Investigating whether or not this is still an issue.
Comment #19
jjchinquist CreditAttribution: jjchinquist commentedAm not certain if there are any other sections - but the patch applies and checks out.
Comment #21
fagoThere is some unused whitespace.
The test fails, as we've got more storage controllers to take care of now. See the DatabaseStorageControllerNG which overrides this.
Comment #21.0
fagoUpdated issue summary.
Comment #22
aspilicious CreditAttribution: aspilicious commentedLet's get this in
Comment #23.0
(not verified) CreditAttribution: commentedRemoving myself from the author field so I can unfollow. --xjm
Comment #24
Berdir22: 1371744-22.patch queued for re-testing.
Comment #26
michaellenahan CreditAttribution: michaellenahan commentedComment #27
michaellenahan CreditAttribution: michaellenahan commentedI tried to reroll this patch but I hit the following problem.
in the patch in ...
core/lib/Drupal/Core/Entity/DatabaseStorageController.php
... there is a function ...
attachLoad()
... that doesn't seem to exist any more.
So I'm not sure exactly what replaces attachLoad().
Comment #28
michaellenahan CreditAttribution: michaellenahan commentedComment #29
BerdirThat has moved to EntityStorageControllerBase::postLoad()
Comment #30
michaellenahan CreditAttribution: michaellenahan commentedComment #31
michaellenahan CreditAttribution: michaellenahan commentedHere's the patch.
Comment #32
vijaycs85Looks good to me, just a documentation question:
Does it worth adding a comment why TYPE specific should be first?
Comment #33
michaellenahan CreditAttribution: michaellenahan commentedTo be honest I'm not sure exactly *why* the hook_TYPE_load() needs to be called before hook_entity_load().
I agree it would be nice to know the exact reason. I think we're just making the change in this patch to make the code consistent. Elsewhere in code, the specific case comes before the general case.
In any case, I'm not sure that this is the correct place to document the order in which the hooks are run.
(Greetings from the Mannheim Sprint. tstoeckler your co-winner of the Prague Quiz is here and says hi. I'm originally from London but live in Germany now.)
Comment #34
michaellenahan CreditAttribution: michaellenahan commentedComment #36
michaellenahan CreditAttribution: michaellenahan commentedTry again.
Comment #39
LendudeRerolled, manual testing for the changed tests works.
Comment #41
LendudeUpdated failing tests to the new order. It's unclear to me why the KeyValueEntityStorageTest test the order of the functions called. Can't really find a reason for it, but maybe somebody else can.
Comment #44
LendudeComment #45
jsobiecki CreditAttribution: jsobiecki commentedComment #48
AjitSWorking on it now.
Comment #49
AjitSThe patch was not applicable. Re-rolling.
Comment #50
piyuesh23 CreditAttribution: piyuesh23 commentedComment #51
AjitSComment #54
AjitSRerolling in a few.
Comment #55
AjitSRerolling
Comment #56
AjitScorrect status
Comment #57
disasm CreditAttribution: disasm at AppliedTrust commentedI am removing the Novice tag from this issue because there are no clear tasks to complete.
I’m using this documentation as a source: https://www.drupal.org/core-mentoring/novice-tasks#avoid
Comment #58
disasm CreditAttribution: disasm at AppliedTrust commentedComment #59
mgiffordNeeds re-roll.
Comment #60
AjitSComment #61
kostyashupenkoRe-rolled with auto merge
Comment #62
kostyashupenkoComment #74
LendudeThis came up as a daily target for #bugsmash.
Me and @longwave discussed this a bit and we couldn't come up with a way of how to do this without breaking BC. Since this is more an annoyance than something that breaks things it would probably not be worth it to break BC.
We figured this could be fixed if core were to move to events for the entity system, see #2551893: Add events for matching entity hooks.
Feel free to reopen this if you feel that this can be fixed while maintaining BC or if you feel this is important enough to break BC