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

[WIP] Adding psr's support #1

Open
wants to merge 12 commits into
base: 5.x
from

Conversation

Projects
None yet
2 participants
@JJarrie
Copy link
Owner

commented Jun 7, 2019

No description provided.

"php": "^5.4|^7.0"
"php": "^7.0",
"psr/http-client": "^1.0",
"psr/http-factory": "^1.0"

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 7, 2019

none of the changes above here are legit: this is a HUGE change for the project, and unrelated to what we want to achieve (which is allowing other kind of clients next to existing ones)

"guzzlehttp/guzzle": "~5.0"
"guzzlehttp/guzzle": "~5.0",
"symfony/http-client": "^4.3",
"nyholm/psr7": "^1.1"

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 7, 2019

since PSR18 doesn't deal with timeouts, let's remove the parts about PSR7/17/18 and focus on the symfony contracts

},
"suggest": {
"symfony/http-client": "Provide an implementation of PSR-18 HTTP client",
"nyholm/psr7": "Provide an implementation of PSR-17 factories and PSR-7 message",

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 7, 2019

should be removed then

},
"suggest": {
"symfony/http-client": "Provide an implementation of PSR-18 HTTP client",

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 7, 2019

let's remove, suggests line are noise anyway
maybe remove the line about guzzle instead?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 7, 2019

OK, no let's keep it (but alpha order + no mention of PSR18)

use Psr\Http\Message\StreamFactoryInterface;
use Symfony\Component\HttpClient\Psr18Client;
class FacebookPsrHttpClient implements FacebookHttpClientInterface

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 7, 2019

let's replace this by FacebookSymfonyHttpClient
PSR18 doesn't handle timeouts, so it cannot be correctly used here (at least we don't have to push it)

@@ -53,6 +53,10 @@ public static function createHttpClient($handler)
if ($handler instanceof FacebookHttpClientInterface) {
return $handler;
}
if ('psr' === $handler) {

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 7, 2019

symfony + missing block before:

if ($handler instanceof HttpClientInterface) { // + corresponding "use" from contracts namespace
    return new FacebookSymfonyHttpClient($handler);
}
},
"suggest": {
"symfony/http-client": "Provide an implementation of PSR-18 HTTP client",

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 7, 2019

OK, no let's keep it (but alpha order + no mention of PSR18)

},
"require-dev": {
"phpunit/phpunit": "~4.0",
"mockery/mockery": "~0.8",
"guzzlehttp/guzzle": "~5.0"
"guzzlehttp/guzzle": "~5.0",
"symfony/http-client": "^4.3",

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 7, 2019

since this prevents usage on PHP5, this should be added "live" in the .travis/etc config

This comment has been minimized.

Copy link
@JJarrie

JJarrie Jun 7, 2019

Author Owner

I don't know where to do this, the doc make me feel like this is the solution, can you confirm please?

language: php

php:
  - 5.4
  - 5.5
  - 5.6
  - 7.0
  - 7.1
  - 7.2
  - 7.3

sudo: false

cache:
  directories:
    - $HOME/.composer/cache

matrix:
  include:
    - php: hhvm
      dist: trusty
    - php: ['7.1', '7.2', '7.3']
      before_install: travis_retry composer require symfony/http-client

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 7, 2019

You should be able to setup the CI on your repo to confirm

JJarrie added some commits Jun 7, 2019

.travis.yml Outdated
@@ -19,6 +16,9 @@ matrix:
include:
- php: hhvm
dist: trusty
- php: [7.1, 7.2, 7.3]
install:
- travis_retry composer require --dev --no-update symfony/http-client symfony/contracts squizlabs/php_codesniffer

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 7, 2019

symfony/contracts should be removed
why squizlabs/php_codesniffer?

"guzzlehttp/guzzle": "Allows for implementation of the Guzzle HTTP client"
"guzzlehttp/guzzle": "Allows for implementation of the Guzzle HTTP client",
"paragonie/random_compat": "Provides a better CSPRNG option in PHP 5",
"symfony/http-client": "Allows for implementation of the Symfony/Contracts HTTP client"

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 7, 2019

bad indentation + we should keep the original order for existing lines

Allows for implementation of the Symfony HttpClient Contracts

{
$response = $this->httpClient->request($method, $url, ['headers' => $headers, 'timeout' => $timeOut]);
return new GraphRawResponse($response->getHeaders(), $response->getContent(), $response->getStatusCode());

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 7, 2019

how does the lib handle errors? how do other clients do?

public function __construct(HttpClientInterface $httpClient = null)
{
if (null === $httpClient && !class_exists('Symfony\Component\HttpClient\HttpClient')) {
throw new \LogicException('You cannot use the "Facebook\HttpClients\FacebookSymfonyHttpClient" as no class implement "Symfony\Contracts\HttpClient\HttpClientInterface" have been provided. Try running "composer require symfony/http-client".');

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 7, 2019

as no class implementing ... has been

JJarrie added some commits Jun 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.