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-18799 remove use of exec in cv #21

Closed
wants to merge 1 commit into from

Conversation

kcristiano
Copy link
Member

No description provided.

@totten
Copy link
Member

totten commented Feb 7, 2018

This appears to be a port of civicrm/civicrm-wordpress#108, which was previously merged into civicrm-wordpress. That code has since been in a number of releases, and no one seems to have changed it since then, so that's positive.

OTOH, I did a quick spot-check on my local dmaster using a basic test command (cv ev 'return CRM_Utils_System::version();'). Without the patch, it works:

~/buildkit/build/dmaster/sites$ ~/src/cv/bin/cv ev 'return CRM_Utils_System::version();'
"4.7.31"

But with the patch, it fails:

~/buildkit/build/dmaster/sites$ ~/src/cv/bin/cv ev 'return CRM_Utils_System::version();'

  [Exception]
  Failed to locate civicrm.settings.php. By default, this tool searches the parent directories for a standard CMS (Drupal, WordPress, etal) and standard civi
  crm.settings.php. Symlinks and multisite configurations may interfere. To customize, set variable CIVICRM_SETTINGS to point to the preferred civicrm.settin
  gs.php.


php:eval [--out OUT] [--level LEVEL] [-t|--test] [-U|--user USER] [--] [<code>]

@kcristiano
Copy link
Member Author

@totten Thanks for the feedback. It was a straight port to be consistent between wp-cli and cv. My current thinking is that we don't need to exclude exec from cv. If cv is used I would expect that the command is available.

If we do want to be consistent I can take another look at this.

@kcristiano kcristiano closed this Nov 26, 2019
@kcristiano kcristiano deleted the crm18799 branch November 26, 2019 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants