Allow alternative tools to crawl attachments for text

hi,

i am ok with having an extra method in the module.

here is a starting patch

can you please test it ?

thanks

Hello,

Thanks for the patch.

I added instruction to install python pdf2txt by example on debian. The file name for me is pdf2txt and not pdf2txt.py.

I added default value for the location /usr/bin. There was a trailling slash ('/') exceeding.

I fixed some indentation problems.

For the pdftotext extraction, there was a bug, the option '-' needed to have a white space between the option and the file name.

The 2 new extractions method are working fine for me.

Once the patch will be merged and #2502073: schema.yml and #2507751: Not using the conf value "search_api_attachments_java" will be emrged, I will make a port to the D8 version.

Thanks for the review.

hi,
here is my final review

1) we should document the the search_api_attachments_debug variable in README.txt
2) we should more document 'Pdftotext' option (including the option name itself), is it present on all distribs by default ? should one download sth to have it ?
3) the watchdogs every time a file is not if pdf mimetypes may be a bad idea for performance, i suggest we wrapp it with an if condition to only be triggered in debug mode.
4) for the python part, is a version recommended ? or at least what are the versions that we tested and know it work with.

Hello,

Thanks for the review.

For the python part, there is no version of this tool. So I added the version in my package manager.

For pdftotext, i saw on the wikipedia page that it is included by default on many linux distributions and that it is part of a package for window if necessary.

quite good :)
only one check missing

+++ b/includes/callback_attachments_settings.inc
@@ -174,6 +174,22 @@ class SearchApiAttachmentsAlterSettings extends SearchApiAbstractAlterCallback {
+            watchdog('search_api_attachments', 'The python_pdf2txt extraction method does not support %mime_type', array('%mime_type' => $file['filemime']), WATCHDOG_WARNING);

needs to be checked against the search_api_attachments_debug variable too :)

Hello,

Should be good now.

D7 feature is merged.
Thanks all !

now seeting it for D8 port
we will need a plugin per method
i'm not for adding the debug variable here for D8. we should think for it more globally, especially i don't think it's to the plugins to do the debug, let's focus on plugins implementation and throw ideas later about the debug stuff

Hello,

Here is the D8 port. I also use this patch to fix some typos, documentation standards. Remove entity_create (deprecated)

I have not tested the extraction, because the live test use an html file and the two new methods are for pdf only.

For D7, the patch introduce a typo in readme.txt "attachements"

Thanks for the review.

Hi,
Thanks for the contribution

- For tests, let's move globally to a pdf file instead of html one.
- It's not easy to remember which one depends on python pdf2txt or pdftotxt without reading back the README. so i suggest that all classes related with pdf2text get perfixed with 'Python', also for the extraction method label and the config variables related to it.

for the code review :

1. +++ b/README.txt
   @@ -1,7 +1,12 @@
   +This module will extract the content out of attached files using one method
   

   using choosed method among

2. +++ b/src/Plugin/SearchApiAttachmentsTextExtractor/Pdf2txtExtractor.php
   @@ -0,0 +1,103 @@
   +      $cmd = escapeshellcmd('python') . ' ' . escapeshellarg($pdf2txt) . ' -C -t text ' . escapeshellarg($filepath);
   

   This may not be available in global path
   we should have an option to configure path to python

3. +++ b/src/Plugin/SearchApiAttachmentsTextExtractor/Pdf2txtExtractor.php
   @@ -0,0 +1,103 @@
   +      '#description' => $this->t('Enter the path to pdf2txt executable. Default value is "/usr/bin".'),
   ...
   +      '#description' => $this->t('Enter the filename of the pdf2txt script. Default value is "pdf2txt".'),
   

   instead of "Default value is" + hardcoded value.(which may be wrong), we can replace it by "Example: "

4. +++ b/src/TextExtractorPluginBase.php
   @@ -103,4 +103,21 @@ abstract class TextExtractorPluginBase extends PluginBase implements TextExtract
   +  public function getPdfMimeTypes() {
   

   I don't know if there is a better way in D8 for this. I know that there are Mimetype guesser from a file extension and also methods to get extension from mymetypes.

   if there is a way to recognise mimes of pdfs, go for that to avoid maintaining a list of pdf mimes

Thanks for the review.

points 1, 2, 3 done.

I also removed deprecated functions.

I am searching for points 4 and the pdf live test.

Do you want to generate a pdf on the fly or having one in a tests folder?

Hello,

I fixed the mispelling mimeGesser -> mimeGuesser

and I found a way to remove the hardcoded list of pdf mimetype.

There is something stange. In /core/lib/Drupal/Core/File/MimeType/ExtensionMimeTypeGuesser.php, you can see the mapping between mimetypes and extensions. And there is only one mimetype for pdf...

I don't know where the list from patch 1 come from.

There is also something that disturbed me: the \Drupal::service, I think we should rely more on dependency injection. But I didn't manage to make it quickly.

Still remain the live check against a pdf. I will wait your response.

+++ b/src/Plugin/SearchApiAttachmentsTextExtractor/PdftotextExtractor.php
@@ -0,0 +1,68 @@
+  public function validateConfigurationForm(array &$form, FormStateInterface $form_state) {

at least a parent::validate ? (in case sth is addaed to the parent in the future)

There is something stange. In /core/lib/Drupal/Core/File/MimeType/ExtensionMimeTypeGuesser.php, you can see the mapping between mimetypes and extensions. And there is only one mimetype for pdf...

we will need to test with pdfs and be sure this doesn't limit the support to a special mimetype

I don't know where the list from patch 1 come from.

this comes form the D7 patch of search_api_attachments, and this liste comes from an aggregation of mimetypes in wikipedia and maybe some php libraries that deal with pdfs

There is also something that disturbed me: the \Drupal::service, I think we should rely more on dependency injection. But I didn't manage to make it quickly.

=> let's create a separate issue for this

(btw, we are now using HTML::escape instead of deprecated safemarkup, i pushed for that change)

Thanks

+++ b/README.txt
@@ -59,4 +64,17 @@ This means that you delete this part:
+EXTRACTION CONFIGURATION (Pdf2txt)

(python Pdf2txt)

+++ b/src/Plugin/SearchApiAttachmentsTextExtractor/PythonPdf2txtExtractor.php
@@ -0,0 +1,114 @@
+      $python_path = realpath($this->configuration['python_path']);

This Realpath doesn't seem to work for me.

i suggest we have two configs only :
- full path to python executable (/usr/bin/python)
- full path to pdf2text script (/usr/bin/pdf2txt.py)

Hello,

Do you want separated patches?

- one for the deprecated functions
- one for the coding styles
- one for python
- one for pdftotext

If think it will accelerate the issue to have smaller patch, easier to merge.

as you want :)
for the deprecated, it's already fixed.
i'm ok for both !

pushed the code to relay on pdf file instead of a html one.

so,i merged the patches and added a fix for them.
here is some communication :
- for python pdf2text, we have only two configuration fields one for 'python' and other for 'fulll path to script' (as in tika config form)
- validateConfigurationForm is added to TextExtractorPluginBase as (the submit one) and deleted from the pdftotext plugin implementation introduced by #15
- for the pdf mimetype discovery, there is \Drupal\file\Entity\File\preCreate that ensures Drupal uses the same method as ours when creatin a pdf file, se we are sure there will be no file object with a non known pdf mimetype for us. (if core knows a mimetype, so we are)
- all other review notices are taken into account.
- the \Drupal::service point will be added to https://www.drupal.org/node/2470553

and this task can be closed :)