Revert of exportables not working

While I realise "me too" messages aren't always helpful, I would just like to say that I can confirm this is an issue.

The configuration all gets loaded up correctly from the feature but the tables don't get created.

Am having a read of the Features API (features.api.php) to see if I can put together a patch but I've not used it before so not sure how successful I will be...

Ok here's what I've found so far:

* If I save this page: "/admin/config/search/search_api/index/default_node_index/fields" - it causes my feature to be displayed as overriden.
* If I then revert the "search_api_index" component, the tables get created successfully
* By reverting this component, this causes the "search_api_server" component to display as being overriden.
* If I then revert the "search_api_server" component, the tables get deleted again!

So the tables actually are getting created, but there is something strange going on between these two components that is causing the tables to get created and then blown away again.

I will keep looking into it, just posting a running commentary here in case anyone else knows what's going on.

I believe the line of code which is dropping the tables again is in the preDelete method in the SearchApiDbService class (service.inc)

Temporarily commenting out the db_drop_table() line prevents the tables from being dropped, allowing the feature to be fully reverted, with all the tables created.

It strikes me that the problem may be in this function in search_api.module (In which case this ticket may be in the wrong issue queue)

/**
 * Implements hook_search_api_server_delete().
 *
 * Calls the preDelete() method for the server.
 */
function search_api_search_api_server_delete(SearchApiServer $server) {
  $server->preDelete();

  // Only react on real delete, not revert.
  if (!$server->hasStatus(ENTITY_IN_CODE)) {
    foreach (search_api_index_load_multiple(FALSE, array('server' => $server->machine_name)) as $index) {
      $index->update(array('server' => NULL, 'enabled' => FALSE));
    }
  }

  $tasks = variable_get('search_api_tasks', array());
  unset($tasks[$server->machine_name]);
  variable_set('search_api_tasks', $tasks);
}

That $server->preDelete() call is the one that is removing the tables. Yet below it we have a comment that says "only react on real delete, not revert". Should the preDelete() call be inside that ENTITY_IN_CODE test? Or has the search_api_db module incorrectly implemented the preDelete() method?

Just a note - the above change prevents the tables from being dropped, but it doesn't fully fix the features workflow. When installing a feature containing all of the search api settings, not all of the tables get created. In order to make this work, you have to:

* Visit the search index's fields config form and save it (Observe that the config is loaded correctly from the feature here, but the tables don't exist)
* Go to the features page and make sure the feature is fully reverted. This then creates the tables.

Not quite sure what is going on here. I've tried to see if I could find the problem but I haven't quite been able to pin down what's wrong.

I'm pretty sure this is caused by the Search API module, not by the DB search backend.
What seems to happen is that, while a "revert" can logically be represented as a delete and an insert (which is why the function looks the way it does), the Entity API only seems to trigger the delete hook. I guess if this is the case we'll just have to invoke the insert hook (or our implementation of it) ourselves.

Additionally, code in the DB search backend might be practical to prevent the internal config from being overridden when it is inserted as a feature.

Oh, and it could also be that we fail to properly add the indexes if a server is added with indexes in place, through a feature.

I'll ask fago to confirm this and share his opinion. He should understand the internal feature workflow better than I do.

We already have
if (!$server->hasStatus(ENTITY_IN_CODE)) {

what should account for the revert operation? If that somehow doesn't work it probably needs some debugging to find out why.

Yes, but $server->preDelete(); is executed in any case. That's the problem there.

I've mentioned the problem in the Entity API queue (#1643184: Invoke insert hook on existing entities on revert), let's see if we can maybe fix this there.
(Although we'll have to see what we then still have to change here.)

Just realized we probably also react completely wrong to index reverts. It seems we don't even check and invoke the delete hooks, removing the index from the server?
Can anyone confirm this? (I.e., problems when changing fields, then reverting back to other field settings? Especially on the DB backend?)

#3 and #4 are good tips to get basic Features integration working without having your tables deleted all the time. I've created a patch here for reference and use in makefiles. Obviously the issue is still open and this is not a proper fix.

a better quick-fix would be to put the ->preDelete inside the ENTITY_IN_CODE condition in the search_api_search_api_server_delete() method. I realize that the db problem of having tables deleted is specific to the backend (proxy), but in essence we are describing a situation where a delete is being called during a revert, so we should allow that other backends will also have the same problem.

Is a good fix to have the preDelete method also check for ENTITY_IN_CODE and then correlate fields before deleteing them, or should we be deleting the fields and then recreating them?

[edited: added coments]

Here is a patch that moves the predelete into the revert test. Now the server->preDelete() method is only run if this is not a revert.

This still only prevents blood from getting on your shoes, it doesn't fix the problem.

There is a patch in the drunkenmonkey #8 comment thread that worked for me. It was suggested 11/30. It validates but needs more testing in the entity case. I have tested it and it works for me (prevented index tables from being deleted.)

#1643184: Invoke update or insert hook on reverts

I "solved" the problem by copying the code generated by the features export and programatically saving it in my install profile:

$local_database_search = entity_import('search_api_server', '{
...
}');
$local_database_search->save();

$document_index = entity_import('search_api_index', '{
..
}');
$document_index->save();

Floretan: so you use this code instead of a feature revert?

I am still worried that another developer on my team in my company (:P) may run a feature revert at a later time, thereby breaking my search. If the resulting entity matches the feature, then I guess it is never reverted accidentally?

After struggling with this problem for a while, trying to get an install profile working that includes search api exportables, I came up with a workaround. In a custom module I created a function which loads all enabled indexes then, for each index, using drupal_submit_form() it first unsets the server, then reassigns it to its original value, then re-enables the index. Executing this sequence rebuilds all the DB tables correctly.

I also created an extra tab on the search api admin which contains a form with nothing but a submit button. This form calls the function mentioned above when submitted.

Lastly, I added a call to this same function as a task in my profile using hook_install_tasks().

Here's the code for my function. It could be refined so it only recreates indexes using default_db as the server. But better would be a fix for the underlying issue :)

<?php
function mymodule_recreate_indexes() {
  module_load_include('inc', 'search_api', 'search_api.admin');
  $indexes = search_api_index_load_multiple(FALSE, array('enabled' => 1));
  foreach($indexes as $id => $index) {
    $server = $index->server;
    $form_state = array(
      'values' => array(
        'name' => $index->name,
        'enabled' => $index->enabled? 1: NULL,
        'description' => $index->description,
        'server' => '',
        'read_only' => $index->read_only? 1: NULL,
        'index_directly' => $index->options['index_directly']? 1: NULL,
        'cron_limit' => $index->options['cron_limit'],
      ),
    );
    drupal_form_submit('search_api_admin_index_edit', $form_state, $index);
    $index = search_api_index_load($id, TRUE);
    $form_state = array(
      'values' => array(
        'name' => $index->name,
        'enabled' => 1,
        'description' => $index->description,
        'server' => $server,
        'read_only' => $index->read_only? 1: NULL,
        'index_directly' => $index->options['index_directly']? 1: NULL,
        'cron_limit' => $index->options['cron_limit'],
      ),
    );
    drupal_form_submit('search_api_admin_index_edit', $form_state, $index);
    if ($index->update(array('enabled' => 1))) {
       drupal_set_message(t('Index !name was successfully recreated.', array('!name' => $index->name)));
    }
  }
}
?>

Slightly improved version of the function. Note that because the form_state is passed by reference it's necessary to recreate it before each submission to avoid side-effects.

<?php
function mymodule_recreate_indexes() {
  module_load_include('inc', 'search_api', 'search_api.admin');
  $indexes = search_api_index_load_multiple(FALSE, array('enabled' => 1));
  foreach($indexes as $id => $index) {
    $server = $index->server;
    $values = array(
        'name' => $index->name,
        'enabled' => $index->enabled? 1: NULL,
        'description' => $index->description,
        'server' => '',
        'read_only' => $index->read_only? 1: NULL,
        'index_directly' => $index->options['index_directly']? 1: NULL,
        'cron_limit' => $index->options['cron_limit'],
    );
    $form_state = array('values' => $values, );
    drupal_form_submit('search_api_admin_index_edit', $form_state, $index);
    $index = search_api_index_load($id, TRUE);
    $values['server'] = $server;
    $form_state = array('values' => $values, );
    drupal_form_submit('search_api_admin_index_edit', $form_state, $index);
    if ($index->update(array('enabled' => 1))) {
       drupal_set_message(t('Index !name was successfully recreated.', array('!name' => $index->name)));
    }
  }
}
?>

We stumbled into the exact same issue. We have found out that during feature revert, first the 'search indexes' are reverted then the 'search server'. We changed the order of the feature components in the feature's .info file, so that the server comes before the indexes. Doing the revert after this change resulted in reverting the components in the correct order (first server, then indexes) and in the end all the required tables got created!

Could you please verify that this solves your problems? (As far as I can tell, the feature components are in alphabetic order in the .info file, which in this case is a big problem.)

#17 After changing the order and executing drush fr myfeature --force it worked, nice catch!

For the record, the patch at #1643184-2: Invoke insert hook on existing entities on revert fixed the problem here for me.

Edit: I may have spoken too soon... The patch fixes reverting an index but not a server.

I'm sorry this is taking so long, everyone, but without a reaction from fago in #1643184: Invoke insert hook on existing entities on revert this really cannot be solved adequately.

However, before this remains broken another year, we should really just commit a workaround now, I guess. If the patches in #10 and #12 work, the attached one is what I'd propose.
We don't know if other servers don't want preDelete() being called on reverts, and when I make an API change for that I want it to be the right one (i.e., when the Entity API issue is fixed, calling the right methods and not just not the wrong ones).
So, fixing this only for the DB search for now seems like the best option. Afterwards I'll move this issue back to the Search API and set it to "postponed", waiting for the Entity API issue to go in.

Note that this still doesn't fix the issue with reverting indexes. But it seems most people here have only a problem with reverting servers? Otherwise maybe we can just skip the index' delete code if it's a revert. Things will go wrong either way, so probably the best we can do is make them go wrong in the rarer cases, or as little as possible.

Now with attachment.

That patch works for me. Thanks, should help a lot (get quite a bug few reports from testers which turn out to be down to missing index tables caused by this issue).

This fixes the problem with reverting a server; thanks!

Committed the patch from #21. Thanks for testing.

As detailled in #20, moving this back to the Search API and setting it to "postponed".

It seems I wronged fago in #1643184: Invoke insert hook on existing entities on revert, hook_entity_insert() is already properly called after a revert. Thus, there's nothing stopping us from solving our specific problems here ourselves. (I don't know why I didn't realize this earlier, I'm sorry!)

I split of the DB backend-specific part into #2026883: Fix workflow for adding a server with indexes in a feature, it's already got a patch.

For the rest, my plan would be to remember the servers/indexes we revert in the delete hook and then redirect the insert hook to the update hook in those cases.
Patch attached, it would be great to get some tests/reviews on it!

Good that I haven't commited this yet – it seems I missed that the search_api_item table references the index by ID, rather than machine name. This is actually quite a problem, as that table is abstracted by the datasource controller, so we cannot just change the index ID there directly. Or, at least that wouldn't be very clean, and would necessitate datasource controllers using a different table to do the same thing.

Attached is a patch implementing it this way nonetheless, and including a note in the documentation for datasource controller implementations. I think this is still preferable to returning to mark all items for re-indexing, and maybe even deleting them from the server.

Please test!

+++ b/search_api.module
@@ -548,6 +560,16 @@ function search_api_search_api_server_delete(SearchApiServer $server) {
+    search_api_search_api_server_update($index);

I suppose that this is typo, becouse this function needs SearchApiServer object instead index object. So i believe here should be search_api_search_api_index_update

Recoverable fatal error: Argument 1 passed to search_api_search_api_server_update() must be an instance of SearchApiServer, instance of SearchApiIndex given, called in sites/all/modules/search_api/search_api.module on line 569 and defined in search_api_search_api_server_update() (line 476 sites/all/modules/search_api/search_api.module).

Oh, wow, thanks for spotting that! You're right of course, a silly but grave copy/paste error.
Attached is a fixed version, please test again!
And again, thanks!

Seems to fix the issue with index being cleared after revert

Great to hear, thanks for testing!
Committed.

I'm facing the same problem with panopoly 7.x-1.0-rc4 and search_api 7.x-1.7, search_api_solr 7.x-1.0 + features 7.x-2.0-beta2 module.

The latest patch worked well to prevent the Solr index to be cleared.

Then update to Search API 1.8.