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

Fix parameter parsing in wp-cli civicrm api. #114

Merged
merged 3 commits into from
Sep 3, 2017

Conversation

jonesinator
Copy link
Contributor

The entity and action were also being parsed as an invalid parameter.
This was fixed by shifting the args array after parsing them.

An unassigned $format variable was being referenced inside of the
default case for the switch statement if the --in option was neither
args nor json. This was fixed by assigning $format to the output of
getOption rather than switching on getOption directly.

The parameter matching regex had spaces added to it in pull request #110
that caused arguments of the format foo=bar to not be matched correctly,
so no parameters were actually being passed to the underlying api
method. The spaces were removed from the regex and the parsing worked.

Two other regex matches had spaces added to them by pull request #110
that I have not touched, but they seem suspect.

The entity and action were also being parsed as an invalid parameter.
This was fixed by shifting the args array after parsing them.

An unassigned $format variable was being referenced inside of the
default case for the switch statement if the --in option was neither
args nor json. This was fixed by assigning $format to the output of
getOption rather than switching on getOption directly.

The parameter matching regex had spaces added to it in pull request civicrm#110
that caused arguments of the format foo=bar to not be matched correctly,
so no parameters were actually being passed to the underlying api
method. The spaces were removed from the regex and the parsing worked.

Two other regex matches had spaces added to them by pull request civicrm#110
that I have not touched, but they seem suspect.
@totten
Copy link
Member

totten commented Sep 1, 2017

Looks like this is your first patch for civicrm-wordpress. Thank you, @jonesinator!

A couple things:

  1. If @kcristiano or @christianwach approves, then we should merge. (Other reviewers welcome, but those guys are pretty sharp about the civicrm-wordpress patches.)
  2. Most PR's should reference a JIRA issue (eg CRM-XXXXX). You can find some background about issue tracking in the dev docs and then signup for an account at https://civicrm.org/user/register . (Warning: due to anti-spam measures, it can take a day or so for a new account to get setup. Feel free to ping if it's taking a while.)

@kcristiano
Copy link
Member

@jonesinator Thanks for the patch.

Can you detail the issue you are having with steps to reproduce?

I agree the spacing added in #110 needs to be fixed.

I've applied the patch to my local dev env and I am getting the following error on any wp cv command:

PHP Parse error:  syntax error, unexpected 'switch' (T_SWITCH) in /srv/www/wpmaster/wp-content/plugins/civicrm/wp-cli/civicrm.php on line 187
Parse error: syntax error, unexpected 'switch' (T_SWITCH) in /srv/www/wpmaster/wp-content/plugins/civicrm/wp-cli/civicrm.php on line 187

I am testing against the latest master branch, I get this error wth both php 7.0 and php 5.6. Any details on your environment would be helpful as well.

@jonesinator
Copy link
Contributor Author

jonesinator commented Sep 2, 2017

Sorry, not sure how a semicolon disappeared between my testing and my commit. Your parsing error should be fixed by my latest patch. Also, apologies for not creating an issue in Jira. I'll try to follow the correct procedure with any changes in the future.

I'm only testing on php 7.0 at the moment. My issue is that any wp-cli civicrm api command that I pass arguments to in the format key1=value1 key2=value2 using --in=args behaves as if no arguments are passed to it. The ability to pass --in=json works correclty. As I was debugging this issue I found the additional issues of use of the unassigned variable $format in the default case and the fact that the arguments were not being shifted prior to parsing, thus including the API entity and action being invoked as parameters.

For example, prior to the fix, the command $ wp cv api contribution.get id=18 actually returned all contributions because the argument id=18 is being ignored accidentally. After the fix it correctly only returns the contribution with an ID of 18.

@kcristiano
Copy link
Member

@jonesinator Thanks again for the patch and the details on how to reproduce.

Reproduced with the current civicrm.php file. Applied patch. Patch applies cleanly and now accepts arguments properly.

@totten This looks good to merge. I'd target 4.7.25

@totten
Copy link
Member

totten commented Sep 3, 2017

Thank you, @jonesinator @kcristiano . Merging into master which will become 4.7.25.

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.

4 participants