Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EZP-29504 : add siteaccess parameter to ezplatform:reindex #2418

Merged
merged 2 commits into from
Sep 9, 2018

Conversation

jlchassaing
Copy link
Contributor

@jlchassaing jlchassaing commented Aug 13, 2018

Question Answer
JIRA issue EZP-29504
Bug yes
New feature no
Target version 6.x
BC breaks no
Tests pass yes
Doc needed no

Add siteaccess parameter when ezplatform:reindex is run in multi-process on a multi-siteaccess multi-repository environment

TODO:

  • Implement feature / fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

Add siteaccess parameter when ezplatform:reindex is run in multi-process
@ezrobot

This comment has been minimized.

@andrerom andrerom requested review from vidarl and alongosz August 21, 2018 08:11
@andrerom andrerom changed the title Fix EZP-29504 : add siteaccess parameter to ezplatform:reindex EZP-29504 : add siteaccess parameter to ezplatform:reindex Aug 22, 2018
@andrerom andrerom merged commit ec4621e into ezsystems:6.7 Sep 9, 2018
@andrerom
Copy link
Contributor

andrerom commented Sep 18, 2018

@emodric has reported a regression on this: https://jira.ez.no/browse/EZP-29640

I'm guessing he hits a case where $this->siteaccess is null, where Process ends up escaping it as empty string: https://github.com/symfony/process/blob/3.4/Process.php#L1719-L1721

@jlchassaing Are you up for improving this a bit to avoid passing this if not set?
@emodric would you be able to confirm that assumption?

@emodric
Copy link
Contributor

emodric commented Sep 18, 2018

@andrerom Yep, when adding an explicit siteaccess argument, then it works fine.

@andrerom
Copy link
Contributor

@emodric Would something like this work for you:

diff --git a/eZ/Bundle/EzPublishCoreBundle/Command/ReindexCommand.php b/eZ/Bundle/EzPublishCoreBundle/Command/ReindexCommand.php
index 807cdf9725..e6104be304 100644
--- a/eZ/Bundle/EzPublishCoreBundle/Command/ReindexCommand.php
+++ b/eZ/Bundle/EzPublishCoreBundle/Command/ReindexCommand.php
@@ -375,12 +375,12 @@ EOT
      */
     private function getPhpProcess(array $contentIds, $commit)
     {
-        $process = new ProcessBuilder([
+        $process = new ProcessBuilder(array_filter([
             file_exists('bin/console') ? 'bin/console' : 'app/console',
             $this->siteaccess ? '--siteaccess=' . $this->siteaccess : null,
             'ezplatform:reindex',
             '--content-ids=' . implode(',', $contentIds),
-        ]);
+        ]));
         $process->setTimeout(null);
         $process->setPrefix($this->getPhpPath());
 

@emodric
Copy link
Contributor

emodric commented Sep 19, 2018

@andrerom It should work, yes.

andrerom added a commit that referenced this pull request Sep 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants