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

Include PHP Version to the USER-AGENT header when make a request to Omise API. #56

Merged
merged 2 commits into from
Mar 21, 2017

Conversation

guzzilar
Copy link
Contributor

1. Objective

Enhance Omise-PHP library, that Omise can provide an appropriate and better support on each of PHP environment by knowing PHP version from clients.

Related information:
Related issue(s): 🙅

2. Description of change

  • Include PHP version to the USER-AGENT header when make a request to the Omise APIs.

3. Quality assurance

🔧 Environments:

  • PHP version: 7.0.16.

✏️ Details:

  1. Try make any request to Omise API, at the requested log in Omise dashboard, you will see PHP/x.y.z appeared at the USER AGENT section.

    025

4. Impact of the change

No impact.

5. Priority of change

Normal.

6. Additional Notes

@@ -250,7 +250,7 @@ private function _executeTest($url, $requestMethod, $key, $params = null)
*/
private function genOptions($requestMethod, $userpwd, $params)
{
$user_agent = "OmisePHP/".OMISE_PHP_LIB_VERSION;
$user_agent = "OmisePHP/" . OMISE_PHP_LIB_VERSION . " PHP/" . phpversion();
Copy link

Choose a reason for hiding this comment

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

It seems that current coding convention does not have spaces between dot (.) used for concatenation. Does this matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iwat nice catch 👍 yah, usually I have 1 space between string concat in other plugin projects.
But better to not do it here for now, as you mentioned.

Will remove

Copy link

Choose a reason for hiding this comment

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

Note that the version string returned by phpversion() may include more information than expected: "5.5.9-1ubuntu4.17", for example. source

Copy link
Contributor Author

@guzzilar guzzilar Mar 21, 2017

Choose a reason for hiding this comment

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

noted 👌

@iwat
Copy link

iwat commented Mar 21, 2017

👍 LGTM

Unit test passed on 5.3, 5.5, 5.6, 7.0, 7.1.
Coverage 73.11% LOC

@guzzilar
Copy link
Contributor Author

👍

@guzzilar guzzilar merged commit 5aae8a7 into master Mar 21, 2017
@guzzilar guzzilar deleted the useragent-include-php-version branch March 21, 2017 08:50
@guzzilar guzzilar changed the title Include PHP Version to the USER-AGENT header when make a request to O… Include PHP Version to the USER-AGENT header when make a request to Omise API. Mar 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants