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

Make PHP API check more specific #587

Closed
wants to merge 1 commit into from

Conversation

iedsapala
Copy link
Contributor

Description

As described in issue 586 the PHP API check is not specific enough and catches other php -i lines that look like PHP API.

I have revised the regex adding in the capability for varying whitespace characters and repetition and checked this going back to GNU Grep 2.0 to ensure maximum compatibility:

[dsapala@DansMacBookProIE grep-2.0]$ ./grep -V
GNU grep version 2.0
usage: grep [-[[AB] ]<num>] [-[CEFGVchilnqsvwx]] [-[ef]] <expr> [<files...>]

current regex:

[dsapala@DansMacBookProIE grep-2.0]$ echo "PHP API => 20131106" | ./grep 'PHP API' | awk '{print $NF}'
20131106
[dsapala@DansMacBookProIE grep-2.0]$ echo "ChartDirector PHP API" | ./grep 'PHP API' | awk '{print $NF}'
API

revised regex:

[dsapala@DansMacBookProIE grep-2.0]$ echo "PHP API => 20131106" | ./grep '^PHP[[:space:]]\+API[[:space:]]\+=>' | awk '{print $NF}'
20131106
[dsapala@DansMacBookProIE grep-2.0]$ echo "ChartDirector PHP API" | ./grep '^PHP[[:space:]]\+API[[:space:]]\+=>' | awk '{print $NF}'

Readiness checklist

  • (only for Members) Changelog has been added to the appropriate release draft. Create one if necessary.
  • Tests added for this feature/bug.

Reviewer checklist

  • Appropriate labels assigned.
  • Milestone is set.
  • Changelog has been added to the appropriate release draft. For community contributors the reviewer is in charge of this task.

@blackstar257
Copy link

👍

@morrisonlevi
Copy link
Collaborator

Thanks for investigating this. Can you double-check that this works in your environment? php -i | awk '/^PHP API =>/ { print $NF }', which should be even more resilient.

@iedsapala
Copy link
Contributor Author

@morrisonlevi I'm not especially familiar with the php -i output format. Is it guaranteed that PHP, API, and => will be separated by exactly one character that is a space?

What about awk '/^PHP[[:space:]]+API[[:space:]]+=>/ {print $NF}' so it's extremely resilient?

@morrisonlevi
Copy link
Collaborator

I agree, although let's change that last + to a *. So:

php -i | awk '/^PHP[[:space:]]+API[[:space:]]*=>/ { print $NF }'

@iedsapala
Copy link
Contributor Author

I don't think API=> would be valid though. I imagine it would have to have at least one whitespace character between API and =>, hence the +. What is the intent of the * in this case?

@morrisonlevi
Copy link
Collaborator

Closed in favor of #590. Thanks you for the contribution!

@iedsapala
Copy link
Contributor Author

@morrisonlevi happy to help, but I have to ask...

Why did you make your own PR that duplicated my work in favor of letting me adjust mine?

@iedsapala iedsapala deleted the git-issue-586 branch September 26, 2019 15:46
@morrisonlevi
Copy link
Collaborator

morrisonlevi commented Sep 26, 2019

Ah, I should have communicated that better. This PR's tests will fail because it isn't based on the latest master, and because of the testing infrastructure requires a github token that you don't have. To be clear, the testing infrastructure should not be needed the token for reads, only writes, so that's a mistake on our end.

You are still credited as the author in the commit: bdf0375.

Thanks for your contribution!

@iedsapala
Copy link
Contributor Author

No problem at all. I just want to understand the process going forward. Thanks for explanation.

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.

None yet

3 participants