Skip to content
This repository was archived by the owner on Jan 13, 2022. It is now read-only.

[Bugfix] Facebook AccessToken is being validated correctly #535

Merged
merged 2 commits into from
Nov 10, 2015
Merged

[Bugfix] Facebook AccessToken is being validated correctly #535

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5e7d44c
[Bugfix] Facebook AccessToken is being validated correctly in Faceboo…
aleksey-kozakov Nov 10, 2015
1535d1b
Rename test method - testAFacebookRequestValidatesTheAccessTokenWhenO…
aleksey-kozakov Nov 10, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Facebook/FacebookClient.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ public function prepareRequestMessage(FacebookRequest $request)
*/
public function sendRequest(FacebookRequest $request)
{
if (get_class($request) === 'FacebookRequest') {
if (get_class($request) === 'Facebook\FacebookRequest') {
Copy link
Contributor

@SammyK SammyK Nov 10, 2015

Choose a reason for hiding this comment

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

It took me a while to remember why this if block is here. It's because the access token isn't required on the root request of a batch request. So this line might actually read better if it were changed to this:

if (get_class($request) !== 'Facebook\FacebookBatchRequest') {

@yguedidi What are your thoughts?

Copy link
Author

@aleksey-kozakov aleksey-kozakov Nov 10, 2015

Choose a reason for hiding this comment

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

Hm, but $request could never be "FacebookBatchRequest". FacebookRequest does not implement any interface.

Copy link
Contributor

@yguedidi yguedidi Nov 10, 2015

Choose a reason for hiding this comment

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

if (!$request instanceof FacebookBatchRequest) {

;)

@lexakozakov FacebookBatchRequest extends FacebookRequest, so you can pass a FacebookBatchRequest to sendRequest

Copy link
Contributor

@SammyK SammyK Nov 10, 2015

Choose a reason for hiding this comment

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

There isn't an interface but FacebookBatchRequest extends from FacebookRequest. When you send a batch request it eventually calls the sendRequest() method.

Copy link
Contributor

@SammyK SammyK Nov 10, 2015

Choose a reason for hiding this comment

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

@yguedidi You beat me to it again! :)

And yes to instanceof!

Copy link
Author

@aleksey-kozakov aleksey-kozakov Nov 10, 2015

Choose a reason for hiding this comment

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

Hm, "FacebookBatchRequest" improvement is not related to the fixed bug, isn't it?

Copy link
Contributor

@SammyK SammyK Nov 10, 2015

Choose a reason for hiding this comment

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

It is related since the get_class() implementation was broken, but this improvement also makes the code a lot more readable. Do you mind making the change to instanceof? :) This should be good to go after that.

Copy link
Author

@aleksey-kozakov aleksey-kozakov Nov 10, 2015

Choose a reason for hiding this comment

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

No, of course I don't mind to use instanceof. Why should I? :)
I'm just wondering if I should make all changes related to "FacebookBatchRequest" in current pull request.

Copy link
Contributor

@SammyK SammyK Nov 10, 2015

Choose a reason for hiding this comment

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

Because this:

if (!$request instanceof FacebookBatchRequest) {

Explains why access token validation gets skipped a lot better than this:

if (get_class($request) === 'Facebook\FacebookRequest') {

It also prevents more hard-coded fully-qualified class names. :)

$request->validateAccessToken();
}

Expand Down
8 changes: 8 additions & 0 deletions tests/FacebookClientTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,14 @@ public function testARequestWithFilesWillBeMultipart()
$this->assertContains('multipart/form-data; boundary=', $headersSent['Content-Type']);
}

public function testAFacebookRequestValidatesTheAccessTokenWhenOneIsNotProvided()
{
$this->setExpectedException('Facebook\Exceptions\FacebookSDKException');

$fbRequest = new FacebookRequest($this->fbApp, null, 'GET', '/foo');
$this->fbClient->sendRequest($fbRequest);
}

/**
* @group integration
*/
Expand Down