While we currently implement onDependencyRemoval() for the index entity to react properly when its server is removed, we currently don't make any attempts to "save the situation" (i.e., remove the dependency instead of deleting the index) when a plugin's dependency is removed, which is bound to be very annoying when it happens (you delete a view mode or disable a module and suddenly your search index, view and facet configuration are all gone).

E.g., some of the following might happen:

We should probably do two things:

  • Have some kind of onDependencyRemoval() method for plugins as well and call that from the index's implementation of the method.
  • If that doesn't work for some plugin, just remove the plugin and not the complete index.

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 : 5
Story Points: 8

Files: 

Comments

drunken monkey created an issue. See original summary.

Nick_vh’s picture

Issue tags: +beta blocker

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 : 5
Story Points: 8

Nick_vh’s picture

Issue summary: View changes
borisson_’s picture

Status: Active » Needs review
FileSize
5.45 KB

This test will fail, but it's supposed to. Let's see if we can make it pass by implementing some code.

Status: Needs review » Needs work

The last submitted patch, 4: properly_react_when_a-2574633-4.patch, failed testing.

borisson_’s picture

Status: Needs work » Needs review
FileSize
1.44 KB
6.14 KB

Test is green now. This triggers when a module providing a processor is removed. Do we want the other parts of this issue seperatly or integrated with this issue?

borisson_’s picture

Issue summary: View changes
Status: Needs review » Needs work
Related issues: +#2573997: Fix dependencies of the RenderedItem processor

We still need to write tests and code for:

I'll start work on the first one now; kicking this issue back to NW.

borisson_’s picture

Status: Needs work » Needs review
FileSize
2.7 KB
7.27 KB

I figure this is enough to test: "An entity type that the index indexes is removed."

I think the other test should go in #2573997: Fix dependencies of the RenderedItem processor, but I'd love to keep that test code in the same class.

Anyway this needs review.

borisson_’s picture

Some better comments in the test.

drunken monkey’s picture

Status: Needs review » Needs work
FileSize
10.37 KB
12.6 KB

Thanks a lot, especially for the tests, that's really good to have!

However, the solution itself is a lot too hard-coded for my taste to just the modules providing processors. It won't react properly …
- for uninstalled modules that provide used datasource or tracker plugins.
- for removed (further) dependencies of any of those plugins.

And just removing those if the dependency gets removed also isn't always the proper solution. Think of the "Rendered item" processor: just because one of the used view modes gets removed, we don't want to remove the processor, probably just set all bundles that used that view mode to "Don't render" (once that's available).ö

So, as said in the OP:

We should probably do two things:

  • Have some kind of onDependencyRemoval() method for plugins as well and call that from the index's implementation of the method.
  • If that doesn't work for some plugin, just remove the plugin and not the complete index.

To find out which dependency comes from which plugin is of course a bit more complicated, but also manageable: we just need to do the same as when calculating dependencies, but just remembering where each dependency comes from (-> just reset $this->dependencies between calls).

Also, testContentTypeRemoval() currently just passes because content types aren't actually added to the index's dependencies, so that's not really a worthwhile test to have. However, I would argue that adding the individual bundles indexed for a datasource doesn't make that much sense, since nothing really needs to change in the case that gets deleted – perhaps a non-existent content type is in the config, then, but I don't think that will have any actual effect. (Same for the "Rendered item" processor – a non-existent view mode is bad, but a non-existent bundle won't cause any trouble.)
Also, this would be a dependency of the datasource, then, anyways, so with a proper solution for all plugins this would be solved as a side effect anyways.

Anyways, since this needs quite some architectural changes, I'd actually have thought I would implement this. So feel free to leave this lying for me if you are unsure what I have in mind. Otherwise, feel free to get cracking, I guess. ;)

The attached patch fixes some other things I noticed, especially for the tests.

drunken monkey’s picture

Forgot to mention: for tests we should also let our processor declare some other dependencies, and check their removal. Same then for a datasource and tracker plugin, and maybe even a server backend. We should also check beforehand whether those dependencies are correctly listed for the index – also important.

And then maybe an integration test for whether the dependencies of our actual plugins are correct – but that should probably be another issue.

borisson_’s picture

I rerolled the patch and fixed the errors. I know we still have to do all the work mentioned in #10. I'll give that a stab tomorrow.

The last submitted patch, 12: properly_react_when_a-2574633-12--reroll.patch, failed testing.

The last submitted patch, 12: properly_react_when_a-2574633-12--reroll.patch, failed testing.

borisson_’s picture

So, I spent some time looking at this but it seems like I don't really understand how to move on, I'll leave this for you Thomas.

borisson_’s picture

More time spent, but with a fresh set of eyes. I think this solution is in the right direction. Needs more work but I'd like validation of that before going on.

Status: Needs review » Needs work

The last submitted patch, 16: properly_react_when_a-2574633-16.patch, failed testing.

The last submitted patch, 16: properly_react_when_a-2574633-16.patch, failed testing.

borisson_’s picture

Hopefully fixes that php5.5 error.

borisson_’s picture

drunken monkey’s picture

Status: Needs review » Needs work

Thanks a lot, Joris, that's indeed a pretty good start! The basic approach seems to make sense, yes.
However, some remarks:

  • The onDependencyRemoval() method can be in the ConfigurablePluginInterface, I think, since it will be the same for all our (configurable) plugins.
  • The method should only be called for those plugins for which dependencies are getting removed, I'd say, and probably only an array containing their actual dependencies.
  • When the plugin returns TRUE, the plugin's settings saved in the index should be updated. Otherwise, the plugin should be removed from the index. (In the case of the tracker, switch back to the default, I guess. In the case of datasources, we should take care to not remove all of the index's datasources.)
  • The code will probably get a bit ugly, or at least repititive, with three types of plugins that can't be handled at once but need to be handled almost the same. That can't really be helped, I guess, but might be an indication what LazyPluginCollection might be able to do for us.
  • I also don't understand why you once load even disabled processors, in Index:.onDependencyRemoval(), but that's of course already more of a detail.

In any case, great work so far, thanks!
As said, though, just say the word and I can take over. Although you now seem to have quite a good grasp of it already.

borisson_’s picture

  1. Yep, done.
  2. This would of course be ideal, but I think it doesn't really hurt to do it this way.
  3. If I understood this correctly, this is fixed now.
  4. If we introduce a LazyPluginCollection that should be a new issue that we can do as a non-blocker follow-up. It shouldn't change anything functionally and I don't think it will break anything either. So let's not do that in this issue - we might be able to keep this a small patch :).
  5. Should be fixed now.

I attached a new patch, I removed one of the test processors and that works with state now, so we need to have less code for the tests.

I'll put in some work for a tracker plugin now.

borisson_’s picture

The attached patch fails with a SchemaIncompleteException and I don't really understand why.

Status: Needs review » Needs work

The last submitted patch, 23: properly_react_when_a-2574633-23.patch, failed testing.

The last submitted patch, 23: properly_react_when_a-2574633-23.patch, failed testing.

drunken monkey’s picture

This would of course be ideal, but I think it doesn't really hurt to do it this way.

Hm, not sure. It's a significant change from how the same method works in entities, and that's gotta be confusing. Look at Index::onDependencyRemoval() – we'd have to, e.g., check a passed server for whether it is actually the one the index is attached to.

If we introduce a LazyPluginCollection that should be a new issue that we can do as a non-blocker follow-up. It shouldn't change anything functionally and I don't think it will break anything either. So let's not do that in this issue - we might be able to keep this a small patch :).

Oh, yes, definitely! I didn't mean to suggest we should incldue that change in this issue, just wanted to point it out, since I'm always wondering myself whether and why we should use that.

Further comments:

  1. +++ b/src/Entity/Index.php
    @@ -1529,12 +1529,16 @@ class Index extends ConfigEntityBase implements IndexInterface {
           if ($is_processor_changed) {
    +        unset($processors[$name]);
             $changed = TRUE;
           }
    

    On the contrary, the unset() should be in else here – if the plugin doesn't react to the removal, the dependency will still be there, so we have to remove the plugin. (Another reason to only call this for plugins that are actually affected.)

  2. +++ b/src/Plugin/ConfigurablePluginInterface.php
    @@ -35,4 +36,17 @@ interface ConfigurablePluginInterface extends PluginInspectionInterface, Derivat
    +   * @return bool
    +   *   Has something changed?
    +   */
    +  public function onDependencyRemoval(IndexInterface $index, array $dependencies);
    

    First off I don't think it's good style to have a question as the documentation.
    Also, I think this should only return TRUE if all the removed dependencies were successfully removed – because otherwise, as said, we need to remove the plugin from the index. (For the server, there is only one plugin, which we cannot change or remove, so things will have to be handled slightly differently there, but the principle is the same).

Since the new Fields UI is now committed, I'd have the time to work on this now. So if you want to hand it off, just say so. (I know, I say that every time anyways. Just want to make sure.)

drunken monkey’s picture

Assigned: Unassigned » drunken monkey
drunken monkey’s picture

Not a particularly easy or simple issue, but I think I've managed to solve it – see the attached patch. You can probably ignore the interdiff, though – it's actually larger than the patch itself, so most likely not a big help.

In the course of writing the tests, I also discovered #2642374: Dependency removal logic incorrectly affects indirect dependents, which seems to be a Core bug in this area. I commented out one assertion due to that, let's see how it progresses.

Status: Needs review » Needs work

The last submitted patch, 28: 2574633-28--removed_plugin_dependencies.patch, failed testing.

The last submitted patch, 28: 2574633-28--removed_plugin_dependencies.patch, failed testing.

drunken monkey’s picture

borisson_’s picture

Status: Needs review » Needs work

The DataSourcePluginBase has a lot of changes that don't seem directly related. Why are they needed here?

  1. +++ b/src/Entity/Index.php
    @@ -1368,21 +1454,130 @@ class Index extends ConfigEntityBase implements IndexInterface {
    +    foreach ($call_on_removal as $plugin_type => $plugins) {
    +      foreach ($plugins as $plugin_id => $plugin_dependencies) {
    +        $removal_successful = $all_plugins[$plugin_type][$plugin_id]->onDependencyRemoval($plugin_dependencies);
    +        if ($removal_successful) {
    

    Can we document what's happening here? I had to read this a couple of times.

  2. +++ b/src/Entity/Index.php
    @@ -1368,21 +1454,130 @@ class Index extends ConfigEntityBase implements IndexInterface {
    +    // There also always needs to be a datasource, but here we have no easy way
    +    // out – if we had to remove all datasources, the operation fails. Return
    +    // FALSE to indicate this (and hope this means the right thing).
    +    if (!$this->datasources) {
    +      return FALSE;
    +    }
    

    Can we add in a drupal_set_message or throw an exception when this happens? So we don't have to hope.

  3. +++ b/src/Plugin/ConfigurablePluginInterface.php
    @@ -35,4 +35,23 @@ interface ConfigurablePluginInterface extends PluginInspectionInterface, Derivat
    +  public function onDependencyRemoval(array $dependencies);
    

    The documentation for this method is awesome! drunken_monkey++

  4. +++ b/src/Plugin/search_api/processor/RenderedItem.php
    @@ -315,4 +315,36 @@ class RenderedItem extends ProcessorPluginBase {
    +    return empty($dependencies);
    

    Can we add a comment somewhere in this method about what's going on? Something like this maybe?

    The dependencies that this processor cares about are all entity view modes. All the view modes that were configured on this plugin have been changed. If there are no other dependencies left, we can let this plugin be enabled with the new settings.

  5. +++ b/tests/src/Kernel/DependencyRemovalTest.php
    @@ -0,0 +1,409 @@
    +    // Check the dependencies were calculated correctly.
    

    Nitpick; There's a word missing here.

    Check that the dependencies

  6. +++ b/tests/src/Kernel/DependencyRemovalTest.php
    @@ -0,0 +1,409 @@
    +      $this->assertNotNull($server, 'Server was not removed');
    

    We could add $this->assertInstanceOf($server, '\Drupal\search_api\ServerInterface'); this is more explicit than $this->assertNotNull. Very minor though.

  7. +++ b/tests/src/Kernel/DependencyRemovalTest.php
    @@ -0,0 +1,409 @@
    +//      $this->assertEquals($server->id(), $this->index->getServerId(), "Index's server was not changed");
    

    alignment of the comment is different here, is that on purpose?

  8. +++ b/tests/src/Kernel/DependencyRemovalTest.php
    @@ -0,0 +1,409 @@
    +    // Since in this test the index will be removed, we need a mock
    

    I think this comment is unfinished.

  9. +++ b/tests/src/Kernel/DependencyRemovalTest.php
    @@ -0,0 +1,409 @@
    +    // If the index resets the tracker, it needs to have the config setting to
    +    // work correctly.
    

    Can we reword this to:
    If the index reset the tracker, it needs to know the name of the default tracker.

  10. +++ b/tests/src/Kernel/DependencyRemovalTest.php
    @@ -0,0 +1,409 @@
    +    return array(
    +      array(TRUE),
    +      array(FALSE),
    +    );
    

    These can get an array key to have more descriptive name in the test suite (currently data set #0).

    I think it'd be better to do:

    return array(
      'Remove dependency' => array(TRUE),
      'Keep dependency' => array(FALSE),
    );
    
drunken monkey’s picture

Thanks a lot once again for the thorough review!

The DataSourcePluginBase has a lot of changes that don't seem directly related. Why are they needed here?

Admittedly, that should better go into #2635872: Move methods from ContentEntity datasource to plugin base class. However, I was creating the test datasource and then discovered I had to implement about ten methods, all of them trivial. Having them in the plugin base class makes tons more sense – otherwise we'd just have to move them again in the other issue.
But if you want I can move that part of the patch over to the proper issue, we can quickly commit it and then rebase the work here on top of that patch. Or just keep both here.

Can we add in a drupal_set_message or throw an exception when this happens? So we don't have to hope.

An exception would mean something entirely different, and not really be appropriate, I think. But we don't have to hope, we now actually have a test for it – so updating the comment to reflect that.
I also don't think we should set a message – it's just an entity being deleted because its dependencies got removed, that's something that will regularly happen in D8. (Also, we can't even be sure what's actually happening there, since onDependencyRemoval() is not only called if the dependency is actually removed, but also to tell what would happen if it were.)

Can we add a comment somewhere in this method about what's going on? Something like this maybe?

You're right, that's utterly undocumented.
However, the code also had two bugs in it, one probably fatal and one potentially leading to index deletion, so it needed some work anyways.

I also wanted to add a test for it, but it seems to me there's a lot wrong with the current code there as well, which kept my additions from working, so moving this to a new issue: #2642792: Fix and expand RenderedItemTest.

Nitpick; There's a word missing here.

I'm not completely sure whether the current phrasing isn't correct, too. This SE question's top answer, e.g., seems to say so. I'm pretty sure I wrote it like this on purpose, at least.
However, adding the "that" won't do any harm either, and might make it easier to read for some – so just added it.

We could add $this->assertInstanceOf($server, '\Drupal\search_api\ServerInterface'); this is more explicit than $this->assertNotNull. Very minor though.

It's not really the scope of the test case to verify that the entity load always produces an instance of that interface if it doesn't return NULL, so we should be able to rely on that.
But, on the other hand, it of course won't do any harm either, I guess.

alignment of the comment is different here, is that on purpose?

Yes, since it's not a normal comment, but a commented-out line of code. I usually comment that out with slashes at the start of the line, to distinguish it (and also because any IDE I know does it like that, iirc).

These can get an array key to have more descriptive name in the test suite (currently data set #0).

Oh, didn't know that. Thanks for the tip!

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

The datasource changes can stay here, but we should close #2635872: Move methods from ContentEntity datasource to plugin base class I guess. I'm otherwise happy with the added changes, and so is the testbot. I feel this can go in

drunken monkey’s picture

Status: Reviewed & tested by the community » Fixed

Excellent, great to hear!
Removed one trailing newline and committed.
Thanks again for your work on this!

Status: Fixed » Closed (fixed)

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