Part of meta-issue #2225347: [META] Replace calls to ENTITY_TYPE_load() and ENTITY_TYPE_load_multiple() with static method calls

See the detailed explanations and examples there.

CommentFileSizeAuthor
#25 replace_all_instances-2377113-25.patch12 KBjeroent
#25 interdiff-23-25.txt656 bytesjeroent
#23 interdiff-21-23.txt2.49 KBjeroent
#23 replace_all_instances-2377113-23.patch12.01 KBjeroent
#21 interdiff-17-21.txt3.89 KBjeroent
#21 replace_all_instances-2377113-21.patch10.33 KBjeroent
#17 interdiff-16-17.txt1.95 KBjeroent
#17 replace_all_instances-2377113-17.patch10.01 KBjeroent
#16 interdiff-15-16.txt900 byteslinl
#16 replace_all_instances-2377113-16.patch10.15 KBlinl
#15 replace_all_instances-2377113-15.patch10.19 KBlinl
#10 replace_all_instances-2377113-10.patch10.18 KBpcambra
#7 interdif-2377113-4-7.txt492 bytesmglaman
#7 replace_all_instances-2377113-7.patch8.65 KBmglaman
#5 interdiff-2377113-#2-#4.txt712 bytesmglaman
#4 replace_all_instances-2377113-4.patch8.66 KBmglaman
#2 replace_all_instances-2377113-2.patch8.56 KBmglaman

Comments

yesct’s picture

yesct commented
Title: Replace all instances of block_content_load(), block_content_load_multiple(), entity_load('block_content') and entity_load_multiple('block_content') with static method calls » Replace all instances of entity_load('block_content') and entity_load_multiple('block_content') with static method calls
mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Active » Needs review
StatusFileSize
new replace_all_instances-2377113-2.patch8.56 KB

Patch with changes.

Status: Needs review » Needs work

The last submitted patch, 2: replace_all_instances-2377113-2.patch, failed testing.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Needs work » Needs review
StatusFileSize
new replace_all_instances-2377113-4.patch8.66 KB

Fix entity cache before checking revision for test.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
StatusFileSize
new interdiff-2377113-#2-#4.txt712 bytes

Attaching interdiff!

yesct’s picture

yesct commented
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Novice

Thanks for the interdiff. :)

1.
Why did we need to clear the cache using the static load()? and we didn't using the entity_load_multiple()?

+++ b/core/modules/block_content/src/Tests/PageEditTest.php
@@ -55,7 +56,8 @@ public function testPageEdit() {
-    $revised_block = entity_load('block_content', $block->id(), TRUE);
+    \Drupal::entityManager()->getStorage('block_content')->resetCache(array($block->id()));

was that what the TRUE was doing? ah yes. ok.

2.

+++ b/core/modules/block_content/src/Tests/BlockContentTypeTest.php
@@ -6,7 +6,7 @@
 namespace Drupal\block_content\Tests;
-
+use Drupal\block_content\Entity\BlockContentType;

This blank line should not have been removed.
We have a blank line between the name space and the block of use statements (although I couldn't find that in the OO white space standards... https://www.drupal.org/node/608152 )

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Needs work » Needs review
StatusFileSize
new replace_all_instances-2377113-7.patch8.65 KB
new interdif-2377113-4-7.txt492 bytes

Correct - you can't call reset within load anymore, so we have to tell the entity storage to reset the entity cache.

Attached updated patch to fix up my screwy line spacing on BlockContentTypeTest.

LinL queued 7: replace_all_instances-2377113-7.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 7: replace_all_instances-2377113-7.patch, failed testing.

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented
Status: Needs work » Needs review
StatusFileSize
new replace_all_instances-2377113-10.patch10.18 KB

Re-rolled and added a couple more loads that were introduced since #7

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Component: entity system » custom_block.module

Moving this to the right component :) (Which has the wrong name...)

chanderbhushan’s picture

chanderbhushan commented

#10 applying successfully

gaurav_varshney’s picture

gaurav_varshney commented

#10 Patch Applied Successfully

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Component: custom_block.module » block_content.module
linl’s picture

linl commented
StatusFileSize
new replace_all_instances-2377113-15.patch10.19 KB

Reroll following #2345833: Convert assetEqual to assertIdentical in migrate_drupal and #2403105: Add an empty text on "Add Custom Block" page when no custom block types are available

linl’s picture

linl commented
StatusFileSize
new replace_all_instances-2377113-16.patch10.15 KB
new interdiff-15-16.txt900 bytes

Also noticed a couple of namespace spacing issues.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new replace_all_instances-2377113-17.patch10.01 KB
new interdiff-16-17.txt1.95 KB

Nitpick, sort use statements alphabetical.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Status: Reviewed & tested by the community » Needs review

Go testbot!

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

No occurrences of entity_load('block_content') and entity_load_multiple('block_content') left and tests pass.

Marking as RTBC as the only thing I did was rearrange the use statements.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/block_content/src/BlockContentForm.php
@@ -80,7 +81,7 @@ public static function create(ContainerInterface $container) {
-    $block_type = entity_load('block_content_type', $block->bundle());
+    $block_type = BlockContentType::load($block->bundle());

+++ b/core/modules/block_content/src/BlockContentTranslationHandler.php
@@ -37,7 +38,7 @@ public function entityFormAlter(array &$form, FormStateInterface $form_state, En
-    $block_type = entity_load('block_content_type', $entity->bundle());
+    $block_type = BlockContentType::load($entity->bundle());

+++ b/core/modules/block_content/src/Plugin/Derivative/BlockContent.php
@@ -17,7 +18,7 @@ class BlockContent extends DeriverBase {
-    $block_contents = entity_load_multiple('block_content');
+    $block_contents = BlockContentEntity::loadMultiple();

All of these should have the block content storage injected.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Status: Needs work » Needs review
StatusFileSize
new replace_all_instances-2377113-21.patch10.33 KB
new interdiff-17-21.txt3.89 KB

Injected the block content storage in BlockContent and BlockContentForm class.

Status: Needs review » Needs work

The last submitted patch, 21: replace_all_instances-2377113-21.patch, failed testing.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Status: Needs work » Needs review
StatusFileSize
new replace_all_instances-2377113-23.patch12.01 KB
new interdiff-21-23.txt2.49 KB

.

Status: Needs review » Needs work

The last submitted patch, 23: replace_all_instances-2377113-23.patch, failed testing.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-23-25.txt656 bytes
new replace_all_instances-2377113-25.patch12 KB

This should fix the failing tests.

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented
Status: Needs review » Reviewed & tested by the community

Patch looks fine and there are no other entity_load('block_content') or entity_load_multiple('block_content') left.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 3f7f953 and pushed to 8.0.x. Thanks!

Beta evaluation is in the meta issue.

  • alexpott committed 3f7f953 on 8.0.x 
    Issue #2377113 by JeroenT, mglaman, LinL, pcambra: Replace all instances...

Status: Fixed » Closed (fixed)

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