-
Notifications
You must be signed in to change notification settings - Fork 2k
Add fix for #619 #636
base: 5.x
Are you sure you want to change the base?
Add fix for #619 #636
Conversation
This looks weird to me... I'm 👎 on this solution The error means that we receive a response that does not have a corresponding request (or at least a corresponding response we do not found). I think it is better to decide what to do in this case:
Any preference? :) |
I'm cool with throwing an exception. But in what scenario would that happen? I tried to trigger the error by manually throwing invalid values at it but I couldn't trigger it! :) |
OMG!!! @SammyK I think I got it! Not tried yet (late night idea), but I think this can happen due to the limit of 50 requests per batch request. 😄 |
That's so awesome! Nice job @yguedidi! And thanks for knocking out some of these issues! I've been pretty swamped lately so I'm looking forward to getting some time to work on the SDK when things let up! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According my suggestion, I thing the doc should be updated, as well as the changelog and tests :)
@@ -98,7 +98,7 @@ public function setResponses(array $responses) | |||
public function addResponse($key, $response) | |||
{ | |||
$originalRequestName = isset($this->batchRequest[$key]['name']) ? $this->batchRequest[$key]['name'] : $key; | |||
$originalRequest = isset($this->batchRequest[$key]['request']) ? $this->batchRequest[$key]['request'] : null; | |||
$originalRequest = isset($this->batchRequest[$key]['request']) ? $this->batchRequest[$key]['request'] : new FacebookRequest; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of a FacebookRequest
, I'd use an UnknownFacebookRequest
which extends FacebookRequest
.
That way, we don't break BC, and the user can check with a simple instanceof
if the original request is there or not.
I've tried my darnedest to replicate the issue in #619, but I can't replicate it for the life of me. I was thinking it was because of timeouts where the response will be
null
, but it handlesnull
responses just fine.I'm thinking this has to be an extreme edge case. At any rate, I added this little patch to keep error messages from being generated if the edge case happens again.
@yguedidi - what do you think? :)