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

CRM-21564: Change from using exec to WP_CLI::Launch #119

Merged
merged 1 commit into from
Apr 16, 2018

Conversation

TBSliver
Copy link
Contributor

@TBSliver TBSliver commented Dec 2, 2017

exec does not seem to be available on all systems. Also makes the code
more consistent for running external processes.

This was discussed in the CiviCRM chat in the wordpress channel about a month ago, have finally got around to pushing this PR


exec does not seem to be available on all systems. Also makes the code
more consistent for running external processes.
@kcristiano
Copy link
Member

@TBSliver This looks like a good improvement. Thank you for the patch. I am away, but will test next week.

It would be great if you can log an issue in our Jira at https://issues.civicrm.org so we can track this in the changelog as well.

@TBSliver
Copy link
Contributor Author

@kcristiano have just signed up for a Jira account (well, a CiviCRM account atleast) but have to wait for authorisation for it... So cant create a ticket for this, or the other issue I pushed (#120) :(

@mlutfy
Copy link
Member

mlutfy commented Dec 11, 2017

Hi @TBSliver, your account is now active.

@kcristiano
Copy link
Member

Thanks @TBSliver I have tested this and it works. This looks ready to merge once we have a JIRA issue to reference.

@TBSliver
Copy link
Contributor Author

Jesus I sometimes hate Jira ticket creation... thats some epic form. @kcristiano https://issues.civicrm.org/jira/browse/CRM-21564

@kcristiano
Copy link
Member

@colemanw I am OK with merging this, but is there anyway to update the PR to include the JIRA issue for the release notes? cc @agh1

@TBSliver TBSliver changed the title Change from using exec to WP_CLI::Launch CRM-21564: Change from using exec to WP_CLI::Launch Dec 21, 2017
@TBSliver
Copy link
Contributor Author

@kcristiano modified the title a while ago, hope this can be merged soon :)

@kcristiano
Copy link
Member

@totten any chance you could give this a look and merge? This is an improvement we should have as soon as practical

@TBSliver
Copy link
Contributor Author

TBSliver commented Apr 6, 2018

@totten Any chance that this can get merged soon, as its now missed atleast 3 patch versions of Civi... and means that upgrades doesnt work on some deployments...

@christianwach
Copy link
Member

@totten I agree with @TBSliver - with the caveat that the WPCLI docs suggest running the command through Utils\esc_cmd() first. See: https://make.wordpress.org/cli/handbook/internal-api/wp-cli-launch/#notes

@colemanw
Copy link
Member

Merging based on positive reviews and feedback. Reading the code and referring to the WP_CLI documentation this looks like a sensible change.

@colemanw colemanw merged commit ed8737f into civicrm:master Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants