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-30013: --env option not passed to parallel processes of ezplatform:reindex Command #2526

Merged
merged 4 commits into from Feb 15, 2019

Conversation

mateuszbieniek
Copy link
Contributor

@mateuszbieniek mateuszbieniek commented Jan 16, 2019

Question Answer
JIRA issue EZP-30013
Bug yes
New feature no
Target version 6.7, 6.13, 7.4
BC breaks no
Tests pass yes
Doc needed no

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This starts to be a maintenance hell, we should consider some uniform handling of these.
But env is probably a special case anyway, so +1 with the following change:

eZ/Bundle/EzPublishCoreBundle/Command/ReindexCommand.php Outdated Show resolved Hide resolved
@gggeek
Copy link
Contributor

gggeek commented Jan 16, 2019

As a side note: when generating SF (sub)commands, I found out that it helps to also add the following snippet:

        if (!$kernel->isDebug()) {
            $subProcessArgs[] = '--no-debug';
        }

to propagate usage of the --no-debug flag (in case the user is on a Dev env but wants to save some ram and cpu)

@andrerom
Copy link
Contributor

andrerom commented Jan 16, 2019

This starts to be a maintenance hell, we should consider some uniform handling of these.

@alongosz Fully agree, there is a followup to do here, one option:
In Sf 4 there is something for this afaik, so maybe backport this feature to symfony-tools repo and take advantage of it already. There is nothing in Symfony for this, the PR I had in mind was never merged. So we can add trait or abstract controller in kernel if we'd like to reuse this.

@andrerom
Copy link
Contributor

andrerom commented Jan 21, 2019

@mateuszbieniek Could you also add the logic @gggeek mentioned?

@mateuszbieniek
Copy link
Contributor Author

@andrerom I'm on it

@ezrobot

This comment has been minimized.

@ezsystems ezsystems deleted a comment from ezrobot Jan 21, 2019
@lserwatka lserwatka merged commit b393c8b into ezsystems:6.7 Feb 15, 2019
@lserwatka
Copy link
Member

Could you merge it up?

@kmadejski
Copy link
Member

@lserwatka merged up in bbf4332 / 9f8c347 / d6c2968 / 4f484f6.

@gggeek
Copy link
Contributor

gggeek commented May 17, 2019

Sorry for the late comment, and maybe I am missing something evident here, but it seems wrong for this command to access directly the spi layer bypassing the Repo services, as it introduces a strong tie to the DB schema... Is it due to the fact that there is currently no way to force the Repo services to execute db queries even when the Search is configured to use a separate system such as Solr?

@mateuszbieniek
Copy link
Contributor Author

@gggeek it seems that your comment is out of the scope of this PR.
Maybe you could create a public ticket for it here: jira.ez.no?

@gggeek
Copy link
Contributor

gggeek commented May 17, 2019

@mateuszbieniek
Copy link
Contributor Author

@gggeek thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
9 participants