Allowing Symfony 3.2 or 4 can break reindexing

[textagroup]

In the following commit https://github.com/silverstripe/silverstripe-fulltextsearch/commit/34e0594e48f8f5c140e3b41544d39c8f88184114 we allow Symfony 3.2 or 4 however the Process class constructor in these 2 versions works differently.

In Symfony Process 3.2 the constructor accepts a string for the commandline parameter, in Symfony Process 4 the constructor requries an array for the commandline parameter.

SolrReindexImmediateHandler.php sends the command line as a array (previous versions sent it as a string)

[textagroup]

https://github.com/symfony/process/blob/3.2/Process.php

https://github.com/symfony/process/blob/v4.4.0/Process.php

[GuySartorelli]

~~Process 3.2 accepts a string, but it also accepts and handles an array. The array was slightly malformed in that commit which breaks reindexing in dev mode - #309 fixes that though, and will be in the next minor release. Given that it's a bugfix it could be git cherry-picked into 4.10 and given a patch release.~~

Sorry, I just double checked. 3.4 accepts an array but 3.2 does explicitly expect and operate on the commandline as a string. You're quite right.

[emteknetnz]

Linked PR has been merged, I'll backport it to the 3.9 branch to tag a new patch release as it's essentially a bugfix

[tim-mediasuite]

I am still getting a failure on this - my indexes work in my tests but they are always indexed as STAGE instead of LIVE. This appears to be because the updated code still wraps $statevar in single quotes here, json_decode fails on the quoted string producing null, and then null is used for the stage update.

https://github.com/silverstripe/silverstripe-fulltextsearch/blob/9da3c2b3315c00540bc6e09d728f767194b432cc/src/Solr/Reindex/Handlers/SolrReindexImmediateHandler.php#L85

Commenting out this line fixes things on my end.

Do lines 80 -> 84 still make sense with the symfony changes?

[emteknetnz]

Just to confirm, you are experiencing this using the newly released 3.9.3 (or 3.9.4)?

Before we remove the quotes, there may be a valid reason why they're there, such as a $statevar with a space in it (sounds like a wild edge case though)

Would you be able to try switching it to double quotes? i.e.

$statevar = "'" . $statevar . "'";

becomes

$statevar = '"' . $statevar . '"';

And let me know how it goes

[tim-mediasuite]

Confirming this is with 3.9.4 (I saw the update come in yesterday while I was investigating this and updated)

I have tried that and my tests start failing again, with items indexed in Stage again. $state is NULL here

https://github.com/silverstripe/silverstripe-fulltextsearch/blob/9da3c2b3315c00540bc6e09d728f767194b432cc/src/Solr/Tasks/Solr_Reindex.php#L107

If I dump $request->getVar('variantState') here the result is

string(83) ""{"SilverStripe\\FullTextSearch\\Search\\Variants\\SearchVariantVersioned":"Live"}""

which as you can see has the surrounding quotes passed through - when the single quotes are used you see

string(83) "'{"SilverStripe\\FullTextSearch\\Search\\Variants\\SearchVariantVersioned":"Live"}'"

I've tried dumping the generated command line from Symfony's Process class with the change to double quotes and I get

exec 'php' '/var/www/html/vendor/silverstripe/framework/cli-script.php' 'dev/tasks/Solr_Reindex' 'index=CWP\Search\Solr\CwpSolrIndex' 'class=SilverStripe\CMS\Model\SiteTree' 'group=0' 'groups=1' 'variantstate="{"SilverStripe\\FullTextSearch\\Search\\Variants\\SearchVariantVersioned":"Live"}"' 'verbose=1'

[emteknetnz]

OK right, so Symfony is surrounding everything with single quotes to handle any spaces, so there's no need for Silverstripe to do this. Thanks for investigating. Could you please open a pull-request branched from and targetting 3.9 removing the code block 80 -> 84. I'll test, merge and tag a new release for you. Cheers

[michalkleiner]

It's a tick from me on the PR, once @emteknetnz has a look he can merge it and release 3.9.5. Thanks for doing the extra testing @tim-mediasuite.

[michalkleiner]

We should forward-port this to 3 as well after it's merged.

[emteknetnz]

@tim-mediasuite 3.9.5

Thanks for your contribution. Let me know if there's any further issues.