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

FacebookBatchResponse failed #619

Open
AidasK opened this issue Jul 18, 2016 · 16 comments
Open

FacebookBatchResponse failed #619

AidasK opened this issue Jul 18, 2016 · 16 comments
Labels

Comments

@AidasK
Copy link

AidasK commented Jul 18, 2016

I was sending loads of successful batch requests when this happened:

...
 if (empty($requests)) {
                continue;
}
$a = $facebook->sendBatchRequest($requests)->getResponses();
 [Symfony\Component\Debug\Exception\FatalThrowableError]
  Type error: Argument 1 passed to Facebook\FacebookResponse::__construct() must be an instance of Facebook\FacebookRequest, null given, called in /var/www/versions/1468485597/vendor/facebook/php-sdk-v4/src/Facebook/FacebookBatchResponse.php on line 108

facebook/php-sdk-v4 5.2.0
php7.0

@SammyK
Copy link
Contributor

SammyK commented Jul 21, 2016

Hey @AidasK! Can you include a bit more code that shows how you are creating the requests before you send them to sendBatchRequest()? :)

@yguedidi
Copy link
Contributor

I think the case of timeouted batched request is not handled properly

@SammyK
Copy link
Contributor

SammyK commented Jul 21, 2016

@yguedidi Ah! Good call. We'll need to look into this. I should be able to give some more TLC in August. :)

@SammyK SammyK added the bug label Jul 21, 2016
@AidasK
Copy link
Author

AidasK commented Jul 21, 2016

I haven't included it, because it is quite straight forward. This error has happened to me only once. I have sent thousands of notifications before and only one time this error occurred.

$requests = [];
foreach ($ids as $id) {
                $requests[] = $facebook->request('post', "/$id/notifications", [
                    'template' => 'You have a new message',
                    'href' => '?p=' . urlencode($id),
                    'ref' => 'abc',
                ]);
}
 if (empty($requests)) {
                continue;
}
$a = $facebook->sendBatchRequest($requests)->getResponses();

@SammyK
Copy link
Contributor

SammyK commented Jul 21, 2016

Thanks @AidasK! I think @yguedidi nailed it on the head - we need to do better handling of failed batch requests. :)

@yguedidi
Copy link
Contributor

@SammyK I'll try ASAP

SammyK added a commit to SammyK/php-graph-sdk that referenced this issue Aug 19, 2016
@yguedidi
Copy link
Contributor

@AidasK can you please confirm that the bug happen when you have more that 50 items in your $ids array? Thanks

@AidasK
Copy link
Author

AidasK commented Mar 21, 2017

I think I was sending exactly 50 notifications in one batch at a time. But not more, because of FB limitations.

@AidasK
Copy link
Author

AidasK commented Mar 21, 2017

p.s. I don't use facebook notifications anymore, so I don't know if it was fixed. You can close it for now

@yguedidi
Copy link
Contributor

@SammyK what do you think if we close this? We can't reproduce it, it's not the more than 50 requests (we already have a safeguard), and it only happened once to @AidasK.

@AidasK
Copy link
Author

AidasK commented Jun 29, 2017

This error has appeared 4 times today:

Type error: Argument 1 passed to Facebook\FacebookResponse::__construct() must be an instance of Facebook\FacebookRequest, null given, called in /var/www/versions/1498736334/vendor/facebook/graph-sdk/src/Facebook/FacebookBatchResponse.php on line 108

This is part of my stacktrace:
screen shot 2017-06-29 at 15 05 03
screen shot 2017-06-29 at 15 08 16

It seems that facebook has responded with an error page, but sdk can't handle it correctly.

facebook/graph-sdk 5.5.0

@SammyK
Copy link
Contributor

SammyK commented Jun 29, 2017

Hum... I'm curious how we could reproduce this.

@AidasK
Copy link
Author

AidasK commented Jun 29, 2017

I don't think we can reproduce it, it is an error from facebook api, but sdk should handle it

@saulirajala
Copy link

I agree with @AidasK. The same issue happened today when Facebook API had some technical problems and was down for a while at least here in Finland. Because of unresponsiveness of API, the first argument for constructor of class FacebookResponse was null, which caused fatal error.

Event though these cases are edge cases, I think SDK should be able to survive from it without fatal error. I'm not that familiar with the architecture of SDK, but @SammyK solution in #636 seems fine or maybe even something like this could work:

$originalRequest = isset($this->batchRequest[$key]['request']) ? $this->batchRequest[$key]['request'] : null;

if ( ! $originalRequest ) {
	return;
}

@yguedidi
Copy link
Contributor

For me, the best fix is to allow null for the original request, but this is a BC break.. I'll then accept the solution proposed in #636 (with some changes)

@BinRoq
Copy link

BinRoq commented Aug 14, 2018

This error has appeared 4 times today:

Type error: Argument 1 passed to Facebook\FacebookResponse::__construct() must be an instance of Facebook\FacebookRequest, null given, called in /var/www/versions/1498736334/vendor/facebook/graph-sdk/src/Facebook/FacebookBatchResponse.php on line 108
This is part of my stacktrace:
screen shot 2017-06-29 at 15 05 03
screen shot 2017-06-29 at 15 08 16

It seems that facebook has responded with an error page, but sdk can't handle it correctly.

facebook/graph-sdk 5.5.0

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

5 participants