silverstripe / silverstripe-fulltextsearch

Adds external full text search engine support to Silverstripe
BSD 3-Clause "New" or "Revised" License
44 stars 83 forks source link

Solr_Reindex build task doesn't work in dev mode in 3.9.0 #308

Closed GuySartorelli closed 2 years ago

GuySartorelli commented 2 years ago

From 3.9.0 the Solr_Reindex build task doesn't work in dev mode. With verbose=1 I can see all of the processes that should get run, but there is no output from any of them. The solr index doesn't get updated at all.

In test mode the correct queued jobs are created, and running them does update the solr index.

GuySartorelli commented 2 years ago

In SolrReindexImmediateHandler.php line 116, if I replace $process->run(); with $process->mustRun(); exposes the following error:

The command "'php /home/webspace/ss4-scratch/site/vendor/silverstripe/framework/cli-script.php dev/tasks/Solr_Reindex' 'index=App\\Search\\SolrIndex' 'class=SilverStripe\\CMS\\Model\\SiteTree' 'group=0' 'groups=7' 'variantstate='\''{"SilverStripe\\FullTextSearch\\Search\\Variants\\SearchVariantVersioned":"Live"}'\''' 'verbose=1'" failed. Exit Code: 127(Command not found) Working directory: /home/webspace/ss4-scratch/site/public Output: ================ Error Output: ================ sh: 1: exec: php /home/webspace/ss4-scratch/site/vendor/silverstripe/framework/cli-script.php dev/tasks/Solr_Reindex: not found

I think this is caused by retaining this line: https://github.com/silverstripe/silverstripe-fulltextsearch/blob/355e928ec9bfcb4a12253139d918d074dd50b858/src/Solr/Reindex/Handlers/SolrReindexImmediateHandler.php#L96 as a single string instead of including each component as a distinct value in the $cmd array https://github.com/silverstripe/silverstripe-fulltextsearch/blob/355e928ec9bfcb4a12253139d918d074dd50b858/src/Solr/Reindex/Handlers/SolrReindexImmediateHandler.php#L98-L106

Making that change results in this error:

Class App\\Search\\SolrIndex does not exist in /home/webspace/ss4-scratch/site/vendor/silverstripe/framework/src/Core/Injector/InjectionCreator.php:17

And after not escaping the backslashes in the index class name we get:

SilverStripe\\CMS\\Model\\SiteTree is not a subclass of DataObject

Finally, when we don't escape the backslashes in that class name either, we get the reindex working correctly.

I'll raise a PR for this momentarily, but putting this sequence here in case anyone wonders why the PR removes the backslash escaping.

michalkleiner commented 2 years ago

Thanks for troubleshooting this!

Do you know/have you found what changed that this stopped working?

GuySartorelli commented 2 years ago

@michalkleiner In 3.9.0 a change was made to allow symfony/process ^4, and part of the change included changing the command from being passed as a string to being passed as an array (see #301). Passing the string worked fine (with process 3.x) but it seems (and this is just based on the error output I was seeing) like there is some step if the command is passed as an array to confirm that the first item in the array is a valid executable.

Edit: Okay so there's nothing done to it prior to being executed... I can't really make heads of exactly why it ends up failing the way it does (the full array is ultimately imploded and the resultant string, with some alterations, is passed to proc_open)...

So I can't say why it's failing the way it is, just that it is and it didn't used to when it was passed as a string - and that splitting out that first array element into three distinct elements resolves it.

The escaping does make sense though, as both process 3 and 4 use implode(' ', array_map([$this, 'escapeArgument'], $commandline)); when imploding the command array, so we had been unintentionally double escaping the class names.

michalkleiner commented 2 years ago

Thanks for looking into it further, @GuySartorelli. I'm under the impression you did what you could and I would say that's more than enough here. You're doing great work lately, keep it up!