Problem/Motivation

As of #2844979: Index nested attachments (eg. entity reference, paragraphs) - arbitrary level there is an ExtractedText formatter that can be applied to any file field to index arbitrary nestings of file content. This works great, except that the isFileIndexable() function has been copied over from the plugin without being altered to use formatter settings. The result is that these calls don't work properly:

$this->extractor->getExcludedMimes()
$this->extractor->isFileSizeAllowed($file)
$this->extractor->isPrivateFileAllowed($file)

The first 2 bail out or use defaults since there is no configuration set, but the private file setting will always return FALSE. This results in private file fields using this formatter never being indexed.

Proposed resolution

Replicate the 3 settings from the processor plugin on to the formatter and use them to do the validation instead of the processor.

Remaining tasks

  • Code
  • Test
  • Review
CommentFileSizeAuthor
#18 results_in_private_files_never_being_indexed-2955936-18.patch21.11 KBYunqiang
#18 interdiff_16-18.txt1.78 KBYunqiang
#16 results_in_private_files_never_being_indexed-2955936-16.patch21.23 KBYunqiang
#16 interdiff_14-16.txt9.59 KBYunqiang
#14 interdiff_5-14.txt12.54 KBYunqiang
#14 results_in_private_files_never_being_indexed-2955936-14.patch20.58 KBYunqiang
#12 interdiff_5-9.txt12.25 KBYunqiang
#9 results_in_private_files_never_being_indexed-2955936-9.patch20.63 KBYunqiang
#5 results_in_private_files_never_being_indexed-2955936-5.patch16.72 KBYunqiang
#2 results_in_private_files_never_being_indexed-2955936-1.patch9.89 KBYunqiang

Comments

acbramley created an issue. See original summary.

Yunqiang’s picture

Yunqiang commented
StatusFileSize
new results_in_private_files_never_being_indexed-2955936-1.patch9.89 KB

As described, duplicate settings in Text extracted formatter and update validation from local settings.

acbramley’s picture

acbramley commented
Status: Active » Needs work 
  1. +++ b/src/Plugin/Field/FieldFormatter/ExtractedText.php
@@ -209,4 +222,142 @@ class ExtractedText extends FileFormatterBase implements ContainerFactoryPluginI
+    $form['excluded_extensions'] = [

    Missing $form = parent::settingsForm($form, $form_state); here. 

  2. +++ b/src/Plugin/Field/FieldFormatter/ExtractedText.php
@@ -209,4 +222,142 @@ class ExtractedText extends FileFormatterBase implements ContainerFactoryPluginI
+    $excluded_mimes = array_combine($excluded_mimes, $excluded_mimes);
+    return array_keys($excluded_mimes);

    This could be simplified with an array_unique().

Looks good in general but there's now a huge amount of duplication between ExtractedText and FilesExtrator. You could refactor the getExcludedMimes, isFileSizeAllowed, and isPrivateFileAllowed functions into a Validator class that takes the settings so both classes could use the Validator and pass it their settings.

izus’s picture

izus commented

+1 for acbramley's suggestion

Yunqiang’s picture

Yunqiang commented
StatusFileSize
new results_in_private_files_never_being_indexed-2955936-5.patch16.72 KB

Reply to #3

Both updated.
Also move reusable functions into a common Validator class.

fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented
Status: Needs work » Needs review

.

acbramley’s picture

acbramley commented
Status: Needs review » Needs work 
+++ b/src/Plugin/search_api_attachments/ExtractFileValidator.php
@@ -0,0 +1,110 @@
+  public static function getExcludedMimes($extensions = NULL, $excluded_extensions = NULL, MimeTypeGuesserInterface $mimeTypeGuesser) {

Instead of passing the MimeTypeGuesser in here, you should make this class into a service and inject the MimeTypeGuesser into it.

Then the ExtractedText and FilesExtractor classes can have the validator injected rather than the mime type guesser.

Other than that it's looking great!

fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented

Just a note to @yunqiang when you upload the changes from #7 to set the issue to "Needs review" so that it triggers the testbot.

Yunqiang’s picture

Yunqiang commented
StatusFileSize
new results_in_private_files_never_being_indexed-2955936-9.patch20.63 KB

As discussed, update the ExtractFileValidator to be a service in Drupal. Both ExtractedText and FilesExtrator inject this service to invoke the validation method. Thanks @acbramley and it's a good pattern.

Yunqiang’s picture

Yunqiang commented
Status: Needs work » Needs review

updated.

acbramley’s picture

acbramley commented

@yunqiang would you mind providing an interdiff between #5 and #9? https://www.drupal.org/documentation/git/interdiff

Yunqiang’s picture

Yunqiang commented
StatusFileSize
new interdiff_5-9.txt12.25 KB

@acbramley, here is the interdiff.

acbramley’s picture

acbramley commented 
+++ b/config/schema/search_api_attachments.schema.yml
@@ -40,3 +40,16 @@ search_api_attachments.admin_config:
\ No newline at end of file

+++ b/search_api_attachments.services.yml
@@ -2,3 +2,6 @@ services:
\ No newline at end of file

Need newlines at the end of these files.

Other than that, this looks good!

Yunqiang’s picture

Yunqiang commented
StatusFileSize
new results_in_private_files_never_being_indexed-2955936-14.patch20.58 KB
new interdiff_5-14.txt12.54 KB

Add new line in both files.

fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented
Status: Needs review » Needs work

Some fairly minor nits:

  1. +++ b/src/Plugin/Field/FieldFormatter/ExtractedText.php
@@ -12,6 +12,8 @@ use Drupal\search_api\Processor\ProcessorPluginManager;
+use Drupal\Core\Form\FormStateInterface;
+use Drupal\search_api_attachments\Plugin\search_api_attachments\ExtractFileValidator;

    These should be in alphabetical order. 

  2. +++ b/src/Plugin/search_api/processor/FilesExtrator.php
@@ -19,8 +19,8 @@ use Drupal\search_api\Utility\FieldsHelperInterface;
+use Drupal\search_api_attachments\Plugin\search_api_attachments\ExtractFileValidator;

    Alphabetical order again. 

  3. +++ b/src/Plugin/search_api_attachments/ExtractFileValidator.php
@@ -0,0 +1,126 @@
+namespace Drupal\search_api_attachments\Plugin\search_api_attachments;

    Should be \Drupal\search_api_attachments\ExtractFileValidator - needs to be updated in a couple other places too. 

  4. +++ b/src/Plugin/search_api_attachments/ExtractFileValidator.php
@@ -0,0 +1,126 @@
+  const DEFAULT_EXCLUDED_EXTENSIONS = 'aif art avi bmp gif ico mov oga ogv png psd ra ram rgb flv';

    Needs a doc block summary of what it is. 

  5. +++ b/src/Plugin/search_api_attachments/ExtractFileValidator.php
@@ -0,0 +1,126 @@
+  ¶

    Stray whitespace. 

  6. +++ b/src/Plugin/search_api_attachments/ExtractFileValidator.php
@@ -0,0 +1,126 @@
+  ¶

    Stray whitespace.

Yunqiang’s picture

Yunqiang commented
StatusFileSize
new interdiff_14-16.txt9.59 KB
new results_in_private_files_never_being_indexed-2955936-16.patch21.23 KB

All updated.
Thanks Saul/Adam.

acbramley’s picture

acbramley commented 
+++ b/src/ExtractFileValidator.php
@@ -0,0 +1,129 @@
+namespace Drupal\search_api_attachments\Plugin\search_api_attachments;

This namespace now doesn't match the location of the file. Should just be Drupal\search_api_attachments

Yunqiang’s picture

Yunqiang commented
StatusFileSize
new interdiff_16-18.txt1.78 KB
new results_in_private_files_never_being_indexed-2955936-18.patch21.11 KB

Updated.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review
fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented
Status: Needs review » Reviewed & tested by the community

There's some minor PHPCS issues in the patch, but most of them existed before and this is just moving them around.

Would be great to expand the module's test coverage, and get the bot actually running the test, but that's not really the task for this issue.

Marking as RTBC. Thanks @yunqiang!

  • yunqiang authored a88825c on 8.x-1.x 
    Issue #2955936 by yunqiang, acbramley, fenstrat, izus: ExtractedText...
izus’s picture

izus commented
Status: Reviewed & tested by the community » Fixed

This is now merged
Thanks all

Status: Fixed » Closed (fixed)

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