Remove default Graph API version fallback #643
Conversation
ghost
added
the
| @@ -74,13 +74,13 @@ class OAuth2Client | |||
| /** | |||
| * @param FacebookApp $app | |||
| * @param FacebookClient $client | |||
| * @param string|null $graphVersion The version of the Graph API to use. | |||
| * @param string $graphVersion The version of the Graph API to use. | |||
| @@ -127,7 +122,6 @@ public function __construct(array $config = []) | |||
| $config = array_merge([ | |||
| 'app_id' => getenv(static::APP_ID_ENV_NAME), | |||
| 'app_secret' => getenv(static::APP_SECRET_ENV_NAME), | |||
| 'default_graph_version' => static::DEFAULT_GRAPH_VERSION, | |||
There was a problem hiding this comment.
I'd keep this with a null value, as this list of keys is like a doc of available options
| @@ -141,6 +135,9 @@ public function __construct(array $config = []) | |||
| if (!$config['app_secret']) { | |||
| throw new FacebookSDKException('Required "app_secret" key not supplied in config and could not find fallback environment variable "' . static::APP_SECRET_ENV_NAME . '"'); | |||
| } | |||
| if (empty($config['default_graph_version'])) { | |||
There was a problem hiding this comment.
If you keep the key above, then I'll check here like above, if (!$config['default_graph_version']) {, for consistency
| @@ -102,7 +102,8 @@ public function __construct(FacebookApp $app = null, $accessToken = null, $metho | |||
| $this->setEndpoint($endpoint); | |||
| $this->setParams($params); | |||
| $this->setETag($eTag); | |||
| $this->graphVersion = $graphVersion ?: Facebook::DEFAULT_GRAPH_VERSION; | |||
| // @TODO Remove this fall-back | |||
| $this->graphVersion = $graphVersion ?: 'v2.7'; | |||
There was a problem hiding this comment.
I think the way to handle this case is to allow null graph version in requests. This parameter is here to override the version given in the client, so it can be null, we just have to take care of this value when sending the request, falling back to the client version when this is null :)
There was a problem hiding this comment.
Hello yguedidi! I have no certainties about this case version 2.8 yet and icoguinita for me, but I resolve to be with this team for sure has brilhates minds, we try faser tests, for and studying the case to come to perfection. I trust you guys the importate and not waste time, bad weather do quick pass worth every second. with an impeccable work. ok let's see what happens reversing from 2.7 to 2.8 ok
| @@ -36,6 +36,8 @@ | |||
|
|
|||
| class FacebookTest extends \PHPUnit_Framework_TestCase | |||
| { | |||
| const DEFAULT_GRAPH_VERSION = 'v2.7'; | |||
There was a problem hiding this comment.
should be changed to 2.8 :)
…h are now supported
…ault_graph_version is provided
|
Sorry for the delay @yguedidi! I've made the changes you requested and also updated the tests. :) Do you have any other thoughts on this one? |
|
|
||
| $this->assertEquals($expectedUrl, $getUrl); | ||
|
|
||
| $postRequest = new FacebookRequest($app, 'foo_token', 'POST', '/bar', ['foo' => 'bar']); | ||
|
|
||
| $postUrl = $postRequest->getUrl(); | ||
| $expectedUrl = '/' . Facebook::DEFAULT_GRAPH_VERSION . '/bar'; | ||
| $expectedUrl = '/bar'; | ||
|
|
||
| $this->assertEquals($expectedUrl, $postUrl); |
There was a problem hiding this comment.
I'd add a case for a versionned request, as it useful in the case of batched request to use a different version than the one of the master request :)
| @@ -36,9 +36,12 @@ | |||
|
|
|||
| class FacebookTest extends \PHPUnit_Framework_TestCase | |||
| { | |||
| const DEFAULT_GRAPH_VERSION = 'v2.8'; | |||
There was a problem hiding this comment.
I'd remove this constant and hardcode it's value everywhere it's used. In tests, we do not need to maintain a consistent value across all tests.
BTW, that why I'd change the value to vX.X or v4.2 (haha), remember to change the value each time the Graph API gets updated is a pain.
|
Thanks as always @yguedidi! :) |
This is a first step to knock out #608. I think in order to close that issue, we'll need to rethink how we're passing around the required vars. You can see I added a hard-coded fallback (temporarily) in
FacebookRequestsince the$graphVersioncan benulland it's at the end of a long list of optional params.Perhaps we could add a
GraphEnvironmententity that would include the Graph version,FacebookApp, access token, etc. Then we could just pass around theGraphEnvironmententity instead of all these individual params. What do you think? :)