Skip to content
This repository has been archived by the owner on Jan 21, 2020. It is now read-only.

PHP 7.2 + QA Tools #13

Merged
merged 14 commits into from May 8, 2018
Merged

PHP 7.2 + QA Tools #13

merged 14 commits into from May 8, 2018

Conversation

michalbundyra
Copy link
Member

Please enable coveralls for the library

- test with lowest/locked/latest dependencies
- dropped HHVM support
- added PHP 7.1 and 7.2 builds
- disabled email notifications
- use composer scripts
- added composer.lock to the repository
- added coveralls configuration
- updated PHPUnit configuration
- moved all support documents into docs directory
- added PR and ISSUE templates
- added coveralls and travis build badges into README.md
- updated LICENSE.md skeleton
- updated composer.json skeleton
- added .gitattributes and updated .gitignore
@weierophinney weierophinney added this to Work in Progress in PHP 7.2 May 2, 2018
Copy link
Member Author

@michalbundyra michalbundyra left a comment

Choose a reason for hiding this comment

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

ping @akrabat

$this->reCaptcha->verify('challenge', 'response');
}

public function testVerifyWithMissingChallengeField()
Copy link
Member Author

Choose a reason for hiding this comment

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

This test is now removed as verify function has only one parameter - responseField.

}

public function testVerifyWithMissingResponseField()
{
$this->reCaptcha->setSecretKey($this->secretKey);
$this->reCaptcha->setIp('127.0.0.1');
$response = $this->reCaptcha->verify('challenge', '');
$response = $this->reCaptcha->verify(null);
Copy link
Member Author

Choose a reason for hiding this comment

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

This test is still failing, doesn't matter what I set here always I'm not getting false status response.
I wonder how it passed with v3 release?

Copy link
Member

Choose a reason for hiding this comment

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

I've done some research, and it appears that payloads to the API verification point expect:

{
    "secret": "secret key",
    "remoteip": "optional; IP address of user",
    "response": "required: user response token verifying user on your site"
}

What's interesting is that while "response" is required, it does not appear that the ReCAPTCHA API cares what the value is, just so long as it is present. As such, this test is essentially moot; it will always return with a "success": true so long as the API key is valid.

I'll remove this test during merge.

@@ -241,6 +241,10 @@ public function getParams()
*/
public function getParam($key)
{
if (! isset($this->params[$key])) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This condition is added to fix tests, previously there was typo in test methods, see: https://github.com/zendframework/ZendService_ReCaptcha/pull/13/files#diff-2e1212952b04d9b7cc6e538238eb2279L67

@@ -300,6 +304,10 @@ public function getOptions()
*/
public function getOption($key)
{
if (! isset($this->options[$key])) {
Copy link
Member Author

Choose a reason for hiding this comment

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

As above

@weierophinney weierophinney merged commit c74fab8 into zendframework:develop May 8, 2018
weierophinney added a commit that referenced this pull request May 8, 2018
PHP 7.2 + QA Tools

Conflicts:
	test/ReCaptchaTest.php
PHP 7.2 automation moved this from Work in Progress to Done May 8, 2018
weierophinney added a commit that referenced this pull request May 8, 2018
@weierophinney
Copy link
Member

Thanks, @webimpress!

@michalbundyra michalbundyra deleted the php-7.2 branch May 8, 2018 21:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
PHP 7.2
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants