-
Notifications
You must be signed in to change notification settings - Fork 733
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
Implement an ErrorTransport (#1529) #1592
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -83,7 +83,8 @@ public function exec(Request $request, array $params) | |
throw new GuzzleException($ex, $request, new Response($ex->getMessage())); | ||
} | ||
|
||
$response = new Response((string) $res->getBody(), $res->getStatusCode()); | ||
$responseBody = (string) $res->getBody(); | ||
$response = new Response($responseBody, $res->getStatusCode()); | ||
$response->setQueryTime($end - $start); | ||
if ($connection->hasConfig('bigintConversion')) { | ||
$response->setJsonBigintConversion($connection->getConfig('bigintConversion')); | ||
|
@@ -93,6 +94,7 @@ public function exec(Request $request, array $params) | |
[ | ||
'request_header' => $request->getMethod(), | ||
'http_code' => $res->getStatusCode(), | ||
'body' => $responseBody, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if this could have some side effects? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No idea – it's my first contribution to this package. Don't think anything in FOSElastica is trying to read this (if i remember correctly) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ruflin it should break anything, i'm doing some tests on that and then I let u know. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i had a look a bit more deeply, and if you look the value of $info here (
[request_header] => GET
[http_code] => 200
[body] => {"took":4,"timed_out":false,"_shards":{"total":5,"successful":5,"skipped":0,"failed":0},"hits":{"total":1,"max_score":0.2876821,"hits":[{"_index":"dynamic_http_method_test","_type":"_doc","_id":"1","_score":0.2876821,"_source":{"test":"test"}}]}}
) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @p365labs Not sure I can follow. So this is good? In general it's just a gut feeling I have that the above could impact other things. If we don't find something I'm good with moving forward. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem I see here: the whole response will be stored twice, first as the response itself, and the second time in the TransferInfo object. When fetching huge responses from ES, this could be a significant overhead. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ha, you're right – i'm doing something completely stupid here! The use-case was to pass the body to have useful error messages (in case the AwsAuth is passed incorrect credentials) in FOSElasticaBundle Can totally factor this bit out There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mean we get a follow up PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jandom if I got it correctly, you're using the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
] | ||
); | ||
|
||
|
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.
Could you add a link to the PR at the end of the line?
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.
Done – let me know if that's what you had in mind
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.
Looks like the comment has been added twice, we already had a backport and usually we just mention the backport PR. Don't know if we have any "standard" here, but those 2 log entries are confusing IMHO
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.
In terms of chronology, first came the PR into master – then the PR into the backward compatibility branch. Happy to go either way here – this is super easy
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.
Thus, we did not add 2 new features, just backported something. In this case I'd like to have the
[backport] ...
log message, pointing to this PR. WDYT @ruflin ?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.
Both options work for me, whatever you prefer.
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.
Thanks for cleaning this up gents, super-fast work!