Following on #2055577: Module disable/uninstall - when a module providing custom datasource controller is disabled, but indexes are left on the site, any access to its relevant view/status/settings/fields/workflow/delete page results in SearchApiException: Unknown or invalid item type search_api_et_node. in search_api_get_datasource_controller()

Also, another relevant issue (though much more specific) is #1414048: Fix exception in views.inc removes all Search API tables

I started looking through the code, and adding catch (SearchApiException, but I'm not really convinced this is the way to go, especially when it started to get a bit messy when I got to Workflow tab.

As I see it, we have 2 possible solutions:

  1. Continue with what I've started doing, and catch SearchApiExceptions in all places where SearchApiIndex->entityWrapper() is called (either directly or through other functions/methods). (I will attach an intermediate patch with the changes I started doing as an example)
  2. Alternatively just define custom access callback for all $pre . '/index/%search_api_index* menu items, and disallow access to them if an index is disabled, plus update search_api_admin_overview() and do not show "name" and "edit" links for disabled indexes. Plus try to find all other situations like described in #1414048: Fix exception in views.inc removes all Search API tables, which do not come directly from viewing/editing an index.

To be honest, I am not completely sure which way would be better here. Part of me would prefer the first solution, where a user could access some info on such indexes, but then some other info won't be displayed (like field lists for example), some features won't be available (like enabling or clearing the index), so it could get kind of confusing...

Thoughts?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

maciej.zgadzaj’s picture

Promised "patch" attached. It does not fix a lot yet, it's more a proof of concept of where I was heading with solution #1 mentioned above. For the moment it just allows error-free access to view/status/settings/fields tabs, but without trying to use any further operations available from these pages.

drunken monkey’s picture

Title: Handle indexes with custom datasources provided by disabled modules » Fix reaction to disabled modules
Component: Miscellaneous » Framework
Status: Active » Needs review
FileSize
2.68 KB

I don't think either of these is a good solution. Being able to edit disabled indexes is something we definitely support, and adding try-catch statements everywhere, while probably being the proper thing to do in any case (we propagate exceptions way to often) significantly reduces the readability of code, and would also be a rather large task.

I think the proper thing to do would be to react on hook_modules_disabled() and delete all indexes with item types that don't exist anymore. While we're at it, we should do the same thing for servers with disappearing service classes.

Patch attached, please review!

Status: Needs review » Needs work

The last submitted patch, 2069023-2--disabled_modules.patch, failed testing.

maciej.zgadzaj’s picture

Well, generally it looks ok, and although it uses the same approach I've implemented in #2055577: Module disable/uninstall, I still don't quite like it to be honest. In ideal world, when the module providing an item type/service class/server is disabled, the relevant index should be disabled too (but not deleted yet); it should be deleted only when the module is uninstalled completely. The question is, do we want to work on and live in that ideal world?

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
3.42 KB

I can't reproduce the test failure from #3, but I still think I can guess where it goes wrong (and am actually kind of puzzled why it works for me locally …). Revised patch attached, which should hopefully fix this.

Regarding your concerns, I have to agree that deleting data when a module is just disabled is far from ideal. Most people seem to agree that disabling modules is broken anyways (#1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed) – but of course, while the function exists we should support it properly.
Just disabling the indexes, however, leads to a lot of problems and will probably not make sense in most situations anyways. "Hiding" the indexes in some way so they only become accessible again when the module is re-enabled would probably be the cleanest option – but also a huge effort since no part of this is currently supported. We'd also have to keep track of which module provides which item type (easily possible, of course, and maybe a good idea anyways – but …) even after the module is disabled and the item type removed.

However, there is another option, which I just remembered: we could, like the Field module in core, just forbid disabling of all modules that provide item types which are used by existing indexes. A bit of a restriction in functionality, but not too hard to implement, low-impact and also much better than automatically deleting data.
Would you be more comfortable with that?

drunken monkey’s picture

Of course, it's also possible that the module providing the last service class was just disabled (or there just are none).
This can't really happen with item types, though, I guess, unless a contrib module runs amok.

maciej.zgadzaj’s picture

However, there is another option, which I just remembered: we could, like the Field module in core, just forbid disabling of all modules that provide item types which are used by existing indexes. A bit of a restriction in functionality, but not too hard to implement, low-impact and also much better than automatically deleting data.
Would you be more comfortable with that?

This idea I really like. It would help users to avoid having a nasty surprise when they just disable the module and suddenly all their indexes are unexpectedly deleted - instead it will force them to take a conscious decision and deal with their own data themselves. Safe, sane and consensual. :)

drunken monkey’s picture

Excellent, then let's do that!
Patch attached, which also takes care of service classes the same way.

maciej.zgadzaj’s picture

Status: Needs review » Reviewed & tested by the community

Magic.

drunken monkey’s picture

Status: Reviewed & tested by the community » Fixed

Great, thanks for testing!
Committed.

Status: Fixed » Closed (fixed)

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