Add exception handling to prevent indexing from halting

This may also be related: https://www.drupal.org/node/2343705 for apachesolr_attachments.module. The main idea is to track files that have failed in the past, so we stop trying to extract them each-and-every-time.

+++ b/src/Plugin/search_api/processor/FilesExtrator.php
@@ -518,4 +533,46 @@ class FilesExtrator extends ProcessorPluginBase implements PluginFormInterface {
+      $this->connection
+        ->insert('search_api_attachments_log')
+        ->fields([
+          'index_id' => $item->getIndex()->id(),
+          'datasource' => $item->getDatasourceId(),
+          'entity_id' => $entity->id(),
+          'fid' => $file->id(),
+          'message' => $exceptionMessage,
+          'timestamp' => REQUEST_TIME,
+        ])
+        ->execute();

Why not just use Drupal's logger service to log this instead of creating a db table for it? That way it's up to the site builder to determine where that log goes (be it syslog, stdout etc)

@malik.kotob re #5 understood, if further work is going to be done to ignore the problematic files then that makes sense.

Further review:

+++ b/src/Plugin/search_api/processor/FilesExtrator.php
@@ -518,4 +540,46 @@ class FilesExtrator extends ProcessorPluginBase implements PluginFormInterface {
+    try {
+      $this->connection
+        ->insert('search_api_attachments_log')
+        ->fields([
+          'index_id' => $item->getIndex()->id(),
+          'datasource' => $item->getDatasourceId(),
+          'entity_id' => $entity->id(),
+          'fid' => $file->id(),
+          'message' => $exceptionMessage,
+          'timestamp' => REQUEST_TIME,
+        ])
+        ->execute();
+    }
+    catch (Exception $e) {
+      throw $e;
+    }

Not sure what the point of this is?
EDIT: To clarify, the try/catch is what I'm questioning. It's just catching and throwing the exception anyway so there's no point having the try/catch in the first place?

Fixed #12 and also fixed the update hook which was failing.

+++ b/src/Plugin/search_api/processor/FilesExtrator.php
@@ -183,7 +195,7 @@ class FilesExtrator extends ProcessorPluginBase implements PluginFormInterface {
-                $extraction .= $this->extractOrGetFromCache($file, $extractor_plugin);
+                $extraction .= $this->extractOrGetFromCache($entity, $file, $item, $extractor_plugin);

This change causes Fatals in the ExtractedText formatter since the $entity and $item parameters have not been added.

That formatter doesn't have access to the search api item object so we may have to remove that from the logging since that is the only reason it is being added here.

Here's a new patch that uses a simple logger approach.

@malik.kotob no worries! #17 is looking much better, I do however think we need some form of tests for this.

Removing the test log from #15

Reroll of #19 against HEAD with a couple of minor fixes:

1) $extracted_data was undefined when an exception was thrown leading to warnings when setting the keyVale store.
2) extractFileContents wasn't returning NULL when isFileIndexable returned FALSE.

It'd be really great to get this committed as I imagine most sites using this module would want their indexing to continue to work when a file can't be extracted.

This does exactly what it says on the box, so marking as RTBC.

Again it'd be great to expand this module tests, but it's probably not the task for this issue.

Thanks all
this is now merged