Posted by xjm

Problem/Motivation

(From #1346032-14: Ordering of hook_entity_delete() is inconsistent.)

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
CommentFileSizeAuthor
#61 entity_load_hooks_are-1371744-61.patch7.48 KBkostyashupenko
#55 entity_load_hooks_are-1371744-55.patch7.48 KBAjitS
#49 entity_load_hooks_are-1371744-49.patch7.48 KBAjitS
#41 interdiff-1371744-39-41.txt3.35 KBLendude
#41 entity_load_hooks_called_wrong_order-1371744-41.patch7.51 KBLendude
#39 entity_load_hooks_called_wrong_order-1371744-39.patch4.16 KBLendude
#36 1371744-36.patch4.32 KBmichaellenahan
#31 1371744-30.patch3.09 KBmichaellenahan
#22 1371744-22.patch5.33 KBaspilicious
#19 entity-load-hook-order-1371744-19.patch4.08 KBjjchinquist
#11 1371744-11.patch4.93 KBjibran
#5 1371744-attachload-order-5.diff4.52 KBaspilicious
#4 1371744-attachload-order-4.patch4.51 KBaspilicious
#2 1371744-attachload-order-2.patch3.62 KBaspilicious
#1 1371744-attachload-order-1.patch1.23 KBaspilicious
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue summary: View changes

Updated issue summary.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Active » Needs review
FileSize
1371744-attachload-order-1.patch1.23 KB
aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
FileSize
1371744-attachload-order-2.patch3.62 KB

Previous patch should fail. Let's try this one...

Status: Needs review » Needs work

The last submitted patch, 1371744-attachload-order-2.patch, failed testing.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs work » Needs review
FileSize
1371744-attachload-order-4.patch4.51 KB

Hmmmm, let's try this one...

This doubles the rdf_mapping_load calls for each $comment. But it only does a lookup the second time.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
FileSize
1371744-attachload-order-5.diff4.52 KB

Ok previous one is crap

scor’s picture

scor CreditAttribution: scor commented

yes, rdf_mapping_load() is tapping into cached data, so should have no performance impact.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

#5: 1371744-attachload-order-5.diff queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1371744-attachload-order-5.diff, failed testing.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +Needs reroll

This will need to be updated for PSR-0.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +Novice
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs review » Needs work
FileSize
1371744-11.patch4.93 KB

reroll of #5

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs work » Needs review

updated status

Issue tags: -Novice, -Needs reroll

The last submitted patch, 1371744-11.patch, failed testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs work » Needs review

#11: 1371744-11.patch queued for re-testing.

jthorson’s picture

jthorson CreditAttribution: jthorson commented
Issue tags: +Novice, +Needs reroll

#11: 1371744-11.patch queued for re-testing.

jthorson’s picture

jthorson CreditAttribution: jthorson commented
Issue tags: -Needs reroll

Updating tags

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented

#11: 1371744-11.patch queued for re-testing.

jjchinquist’s picture

jjchinquist CreditAttribution: jjchinquist commented
Assigned: Unassigned » jjchinquist

patch fails on the newest dev version. Investigating whether or not this is still an issue.

jjchinquist’s picture

jjchinquist CreditAttribution: jjchinquist commented
FileSize
entity-load-hook-order-1371744-19.patch4.08 KB

Am not certain if there are any other sections - but the patch applies and checks out.

Status: Needs review » Needs work

The last submitted patch, entity-load-hook-order-1371744-19.patch, failed testing.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented 
+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
@@ -416,6 +411,12 @@ protected function attachLoad(&$queried_entities, $load_revision = FALSE) {
     }

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

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Issue summary: View changes

Updated issue summary.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs work » Needs review
FileSize
1371744-22.patch5.33 KB

Let's get this in

The last submitted patch, 1371744-22.patch, failed testing.

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

Removing myself from the author field so I can unfollow. --xjm

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review

22: 1371744-22.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 22: 1371744-22.patch, failed testing.

michaellenahan’s picture

michaellenahan CreditAttribution: michaellenahan commented
Assigned: jjchinquist » michaellenahan
Issue summary: View changes
Issue tags: +D8MA, +SprintWeekend2014
michaellenahan’s picture

michaellenahan CreditAttribution: michaellenahan commented

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

michaellenahan’s picture

michaellenahan CreditAttribution: michaellenahan commented
Assigned: michaellenahan » Unassigned
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

That has moved to EntityStorageControllerBase::postLoad()

michaellenahan’s picture

michaellenahan CreditAttribution: michaellenahan commented
Assigned: Unassigned » michaellenahan
michaellenahan’s picture

michaellenahan CreditAttribution: michaellenahan commented
Status: Needs work » Needs review
FileSize
1371744-30.patch3.09 KB

Here's the patch.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented

Looks good to me, just a documentation question:

+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerBase.php
@@ -164,16 +164,16 @@ protected function invokeHook($hook, EntityInterface $entity) {
-    // Call hook_entity_load().

Does it worth adding a comment why TYPE specific should be first?

michaellenahan’s picture

michaellenahan CreditAttribution: michaellenahan commented

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

michaellenahan’s picture

michaellenahan CreditAttribution: michaellenahan commented
Assigned: michaellenahan » Unassigned

Status: Needs review » Needs work

The last submitted patch, 31: 1371744-30.patch, failed testing.

michaellenahan’s picture

michaellenahan CreditAttribution: michaellenahan commented
Status: Needs work » Needs review
FileSize
1371744-36.patch4.32 KB

Try again.

Berdir queued 36: 1371744-36.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 36: 1371744-36.patch, failed testing.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude commented
Status: Needs work » Needs review
Issue tags: +Amsterdam2014
FileSize
entity_load_hooks_called_wrong_order-1371744-39.patch4.16 KB

Rerolled, manual testing for the changed tests works.

Status: Needs review » Needs work

The last submitted patch, 39: entity_load_hooks_called_wrong_order-1371744-39.patch, failed testing.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude commented
Status: Needs work » Needs review
FileSize
entity_load_hooks_called_wrong_order-1371744-41.patch7.51 KB
interdiff-1371744-39-41.txt3.35 KB

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

Status: Needs review » Needs work

The last submitted patch, 41: entity_load_hooks_called_wrong_order-1371744-41.patch, failed testing.

Lendude queued 41: entity_load_hooks_called_wrong_order-1371744-41.patch for re-testing.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude commented
Status: Needs work » Needs review
jsobiecki’s picture

jsobiecki CreditAttribution: jsobiecki commented
Issue tags: +dcwroc2014

jibran queued 41: entity_load_hooks_called_wrong_order-1371744-41.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 41: entity_load_hooks_called_wrong_order-1371744-41.patch, failed testing.

AjitS’s picture

AjitS
he/him
Primary language Marathi
Location Pune
CreditAttribution: AjitS commented
Assigned: Unassigned » AjitS
Issue tags: +Goa2015

Working on it now.

AjitS’s picture

AjitS
he/him
Primary language Marathi
Location Pune
CreditAttribution: AjitS at QED42 commented
Status: Needs work » Needs review
FileSize
entity_load_hooks_are-1371744-49.patch7.48 KB

The patch was not applicable. Re-rolling.

piyuesh23’s picture

piyuesh23 CreditAttribution: piyuesh23 commented
Issue tags: -Goa2015 +#drupalgoa2015
AjitS’s picture

AjitS
he/him
Primary language Marathi
Location Pune
CreditAttribution: AjitS at QED42 commented

AjitS queued 49: entity_load_hooks_are-1371744-49.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 49: entity_load_hooks_are-1371744-49.patch, failed testing.

AjitS’s picture

AjitS
he/him
Primary language Marathi
Location Pune
CreditAttribution: AjitS at QED42 commented
Issue tags: +Needs reroll

Rerolling in a few.

AjitS’s picture

AjitS
he/him
Primary language Marathi
Location Pune
CreditAttribution: AjitS as a volunteer commented
Issue tags: -Needs reroll
FileSize
entity_load_hooks_are-1371744-55.patch7.48 KB

Rerolling

AjitS’s picture

AjitS
he/him
Primary language Marathi
Location Pune
CreditAttribution: AjitS as a volunteer commented
Assigned: AjitS » Unassigned
Status: Needs work » Needs review

correct status

disasm’s picture

disasm CreditAttribution: disasm at AppliedTrust commented

I 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

disasm’s picture

disasm CreditAttribution: disasm at AppliedTrust commented
Issue tags: -Novice
mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford at OpenConcept Consulting Inc. commented
Status: Needs review » Needs work

Needs re-roll.

AjitS’s picture

AjitS
he/him
Primary language Marathi
Location Pune
CreditAttribution: AjitS as a volunteer commented
Issue tags: +Needs reroll
kostyashupenko’s picture

kostyashupenko
Primary language Russian
Location Omsk
CreditAttribution: kostyashupenko at Skilld commented
Issue tags: -Needs reroll
FileSize
entity_load_hooks_are-1371744-61.patch7.48 KB

Re-rolled with auto merge

kostyashupenko’s picture

kostyashupenko
Primary language Russian
Location Omsk
CreditAttribution: kostyashupenko at Skilld commented
Status: Needs work » Needs review

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at iO commented
Status: Needs review » Closed (won't fix)
Issue tags: +Bug Smash Initiative

This 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