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

Extra checks #60

Merged

Conversation

frankmullenger
Copy link
Contributor

Checks for session cookies and cache headers in responses, introduces Guzzle dependency.

@frankmullenger
Copy link
Contributor Author

Happy to remove the return type declarations if Travis builds for earlier PHP versions are going to remain in the module for now?

@ScopeyNZ
Copy link
Member

Happy to remove the return type declarations if Travis builds for earlier PHP versions are going to remain in the module for now?

Yeah that'd be great. Have to wait a few more months before we can start using PHP7 specific features.

@NightJar
Copy link
Contributor

That was going to be my feedback, yeah :)

@NightJar
Copy link
Contributor

NightJar commented Feb 15, 2019

phpcs @frankmullenger ;)

@frankmullenger
Copy link
Contributor Author

Not sure what the policy is on merging with failed code coverage tests but if these need unit tests that is probably a job for another hack day :-)

@NightJar
Copy link
Contributor

Ideally they should exist, but on the other hand they could be a follow up.
I'll leave it open for colleagues to evaluate. I think tests would be bests.

Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

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

Agree with @NightJar - new features should have new tests ideally

* @param array|null $extraConfig Extra configuration
* @return ResponseInterface
*/
public function fetchResponse(string $url, array $extraConfig = [])
Copy link
Contributor

Choose a reason for hiding this comment

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

Scalar typehints aren't PHP 5.6 friendly =)

@frankmullenger
Copy link
Contributor Author

Fair enough, will look to add some unit tests next hackday 👍

Copy link
Contributor

@NightJar NightJar left a comment

Choose a reason for hiding this comment

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

Nice work :D

@NightJar NightJar merged commit c5783a2 into silverstripe:master Mar 19, 2019
@frankmullenger frankmullenger deleted the feature/extra-checks branch March 20, 2019 00:12
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

4 participants