We currently have code in ContentEntity::loadMultiple() to remove items that could not be loaded from tracking, as kind of a fail-safe if something goes wrong during entity deletion.

However, since that's (very probably) something which is common to all data sources, I think it actually makes much more sense to move that code to the index's loadMultipleItems() method.

There's also an @todo in that method to the same effect.

Estimated Value and Story Points

This issue was identified as a Beta Blocker for Drupal 8. We sat down and figured out the value proposition and amount of work (story points) for this issue.

Value and Story points are in the scale of fibonacci. Our minimum is 1, our maximum is 21. The higher, the more value or work a certain issue has.

Value : 1
Story Points: 3

CommentFileSizeAuthor
#15 2574589-15--remove_unloadable_items.patch83.81 KBdrunken monkey
#15 2574589-15--remove_unloadable_items--tests_only.patch78.76 KBdrunken monkey
#15 2574589-15--remove_unloadable_items--interdiff.txt913 bytesdrunken monkey
#11 2574589-11--remove_unloadable_items.patch83.88 KBdrunken monkey
#11 2574589-11--remove_unloadable_items--tests_only.patch78.82 KBdrunken monkey
#3 2574589-3--remove_unloadable_items.patch15.7 KBdrunken monkey
#3 2574589-3--remove_unloadable_items--tests_only.patch10.48 KBdrunken monkey

Comments

drunken monkey created an issue. See original summary.

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
Issue summary: View changes
Issue tags: +beta blocker
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Active » Needs review
StatusFileSize
new 2574589-3--remove_unloadable_items--tests_only.patch10.48 KB
new 2574589-3--remove_unloadable_items.patch15.7 KB

Here is a first attempt, complete with test.

The last submitted patch, 3: 2574589-3--remove_unloadable_items--tests_only.patch, failed testing.

The last submitted patch, 3: 2574589-3--remove_unloadable_items--tests_only.patch, failed testing.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Related issues: +#2693593: Merge search_api_test and search_api_test_backend modules

Linking a follow-up for simplifying the test structure a bit.

Status: Needs review » Needs work

The last submitted patch, 3: 2574589-3--remove_unloadable_items.patch, failed testing.

The last submitted patch, 3: 2574589-3--remove_unloadable_items.patch, failed testing.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs work » Postponed
Related issues: +#2693613: Correctly react when content entity datasource languages are changed

Looks like #2693613: Correctly react when content entity datasource languages are changed finally made itself known.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Related issues: +#2574611: Unify our two task systems

Dependency got moved to #2574611: Unify our two task systems. Which should soon be committed, finally enabling us to untangle this whole dependency chain.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Postponed » Needs review
StatusFileSize
new 2574589-11--remove_unloadable_items--tests_only.patch78.82 KB
new 2574589-11--remove_unloadable_items.patch83.88 KB

The other issue got committed, so here is a re-roll for this one – including #2693593: Merge search_api_test and search_api_test_backend modules and some further test module merging.

The last submitted patch, 11: 2574589-11--remove_unloadable_items--tests_only.patch, failed testing.

The last submitted patch, 11: 2574589-11--remove_unloadable_items--tests_only.patch, failed testing.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Needs work

Overall, I think this looks great and I only have nitpicks.

  1. +++ b/src/Entity/Index.php
@@ -817,23 +817,54 @@ public function loadItem($item_id) {
+      $missing_ids = array_reduce(array_map('array_values', $items_by_datasource), 'array_merge', array());

    I had to read this line 4 times to understand what's going on, but I don't think we can make this easier to read so let's keep it like this. 

  2. +++ b/tests/search_api_test/src/TestPluginTrait.php
@@ -0,0 +1,95 @@
+  protected function getPluginType() {
+    if (!isset($this->pluginType)) {
+      $class = explode("\\", get_class($this));
+      array_pop($class);
+      $this->pluginType = array_pop($class);
+    }
+
+    return $this->pluginType;
+  }

    This is really clever, @drunken monkey++ 

  3. +++ b/tests/src/Kernel/DependencyRemovalTest.php
@@ -38,8 +41,8 @@ class DependencyRemovalTest extends KernelTestBase {
-    'search_api_test_backend',
-    'search_api_test_dependencies',
+    'search_api_test',
+    'search_api_test',

    We only need that module once. 

  4. +++ b/tests/src/Kernel/IndexLoadItemsTest.php
@@ -0,0 +1,105 @@
+    $items = $this->index->loadItemsMultiple($item_ids);
+    $this->assertEquals(array(), $items, 'No items loaded from test datasource.');

    This can also be $this->assertEmpty($items); but I don't think that makes a big difference. There's a couple other places in the patch where we can change this as well. 

  5. +++ b/tests/src/Kernel/ServerTaskTest.php
@@ -134,12 +128,13 @@ public function testAddIndex() {
+    $this->setError('backend', 'addIndex');
+    $this->setError('backend', 'addIndex');

    I think we can remove one of those lines.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs work » Needs review
StatusFileSize
new 2574589-15--remove_unloadable_items--interdiff.txt913 bytes
new 2574589-15--remove_unloadable_items--tests_only.patch78.76 KB
new 2574589-15--remove_unloadable_items.patch83.81 KB

I had to read this line 4 times to understand what's going on, but I don't think we can make this easier to read so let's keep it like this.

Oh god, yes, this is aweful. But that's why I put a comment above it, so, as you say, I think we can just leave it like this.

This can also be $this->assertEmpty($items); but I don't think that makes a big difference. There's a couple other places in the patch where we can change this as well.

So, do you think we should change it? I find it a bit clearer like this, but both would be OK for me.

Revised patch attached.

The last submitted patch, 15: 2574589-15--remove_unloadable_items--tests_only.patch, failed testing.

The last submitted patch, 15: 2574589-15--remove_unloadable_items--tests_only.patch, failed testing.

borisson_’s picture

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

I don't have strong opinions either way.

  • drunken monkey committed ad157bb on 8.x-1.x 
    Issue #2574589 by drunken monkey: Moved the "remove unloadable items...
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Reviewed & tested by the community » Fixed

OK, thanks for reviewing!
Committed.

Status: Fixed » Closed (fixed)

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