-
Notifications
You must be signed in to change notification settings - Fork 16
Conversation
Migrating to api v2
The private and public key properties have been renamed, so we have to update in this class. Introduce getPrivateKey(), getPublicKey() and setPublicKey() so as MailHide’s API is still v1.
Remove PHP 5.5 & add 7.1. Do not allow 7.0 to be a failure
Update to current standards with PHP 5.6 or above.
This helps Travis with PHP 5.6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted a few additional changes. In particular, update the composer.json to require zendfamework/zend-coding-standard, as that has a few more rules which will pick up some of the CS issues I flagged. Additionally, maybe consider putting the Google test keys in the phpunit.xml.dist
file as the default values.
Overall, looks great!
@@ -10,21 +10,20 @@ cache: | |||
matrix: | |||
fast_finish: true | |||
include: | |||
- php: 5.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
README.md
Outdated
@@ -7,7 +7,7 @@ You can install using: | |||
``` | |||
curl -s https://getcomposer.org/installer | php | |||
php composer.phar install | |||
composer require zendframework/zendservice-recaptcha | |||
php composer.phar require zendframework/zendservice-recaptcha | |||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternately, we point them to the Composer download page, and demonstrate using composer require
(not usage of the phar
file). Either way works, though we've been trending towards just saying composer require
.
public function getPublicKey() | ||
{ | ||
return parent::getSiteKey(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these proxy methods maybe be marked deprecated now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They aren't actually deprecated for MailHide as that's the correct wording for its API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, sounds good.
@@ -273,11 +303,11 @@ public function getHtml($email = null) | |||
throw new MailHideException('Missing email address'); | |||
} | |||
|
|||
if ($this->publicKey === null) { | |||
if ($this->getPublicKey() === null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why getPublicKey()
and not getSiteKey()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MailHide is still on v1's API which uses the nomenclature "public_key" and "private_key".
Seemed sensible to be consistent within this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Maybe add some verbiage indicating that, then?
throw new MailHideException('Missing public key'); | ||
} | ||
|
||
if ($this->privateKey === null) { | ||
if ($this->getPrivateKey() === null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why getPrivateKey()
and not getSecretKey()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MailHide is still on v1's API which uses the nomenclature "public_key" and "private_key".
composer.json
Outdated
}, | ||
"require-dev": { | ||
"phpunit/phpunit": "~4.0", | ||
"phpunit/phpunit": "^5.7 || ^6.0", | ||
"squizlabs/php_codesniffer": "^2.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change this dependency to "zendframework/zend-coding-standard": "~1.0.0"
, please, and then re-run phpcs and/or phpcbf; they'll pick up the various CS bits I've noticed.
src/Response.php
Outdated
* @param string $errorCode | ||
* @param HTTPResponse $httpResponse If this is set the content will override $status and $errorCode | ||
* @param array $errorCodes | ||
* @param \Zend\Http\Response $httpResponse If this is set the content will override $status and $errorCode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the same typehint as found in the signature; IDEs and phpdoc will expand them correctly.
src/Response.php
Outdated
* @param string $status | ||
* @return Response | ||
* @param boolean $status | ||
* @return \ZendService\ReCaptcha\Response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@return self
src/Response.php
Outdated
@@ -57,16 +57,12 @@ public function __construct($status = null, $errorCode = null, HTTPResponse $htt | |||
/** | |||
* Set the status | |||
* | |||
* @param string $status | |||
* @return Response | |||
* @param boolean $status |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bool
instead of boolean
(internal consistency)
test/ReCaptchaTest.php
Outdated
$this->secretKey = getenv('TESTS_ZEND_SERVICE_RECAPTCHA_SECRET_KEY'); | ||
|
||
if (empty($this->siteKey)) { | ||
$this->siteKey = '6LeIxAcTAAAAAJcZVRqyHh71UMIEGNQ_MXjiZKhI'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe these should be in the phpunit.xml.dist
as the default values, with a comment above indicating users should change them if they want to use their own keys? That would simplify the test code a bit.
- Simplify information about composer - Link to documentation
@weierophinney Review items addressed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good!
Hi guys. Some time ago I wrote full service for recaptcha v2 https://github.com/xorock/zend-service-recaptcha-v2. The only thing that is missing here are tests. |
Update @Lansoweb's PR (#8) to ensure MailHide still works with its v1 API as it's not been updated to v2 by Google.
Also ensure Travis tests on PHP 5.6, 7, and 7.1. For v2, Google provide test keys, so we can use them and let Travis test better.