function commerce_yotpo_create_purchase in commerce_yotpo.module (line 265) a response object is returned but not checked. Would it be an idea to check for a return code of '200' and if not '200' write a warning to watchdog?

CommentFileSizeAuthor
#6 check_response_code-2344269-6.patch544 bytesjoelpittet
#6 interdiff.txt504 bytesjoelpittet
#3 check_response-2344269.patch517 byteslanceh1412

Comments

lanceh1412’s picture

lanceh1412 commented

Something like

if ($response->code != 200) {
      watchdog('commerce_yotpo', 'Unexpected response code: ' . $response->code );
    }
lsolesen’s picture

lsolesen commented

Could you create a patch for this change?

lanceh1412’s picture

lanceh1412 commented
StatusFileSize
new check_response-2344269.patch517 bytes
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Active » Needs review 
+++ b/commerce_yotpo.module
@@ -263,6 +263,9 @@ function commerce_yotpo_create_purchase($commerce_order) {
+    if ($response->code != 200) {

Are there any other codes we should watch for, like 304?

Maybe $code >= 200 AND $code < 400 or maybe just the 2xx codes?

Either way I've no problem committing this, thank you for creating the patch @lanceh1412

lanceh1412’s picture

lanceh1412 commented

I don't know. Generally the response code should be 200. Anything else generates a notice in watchdog. If the return was generally not 200 would this indicate an issue e.g 301 would suggest that we need to update the url? I'm just thinking that maybe we should report on anything other than 200. I've been running this code for a while and never had noticed any entries in the log.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Fixed
StatusFileSize
new interdiff.txt504 bytes
new check_response_code-2344269-6.patch544 bytes

Well that makes sense, if we catch a bunch of these and they are in a different code that is reasonable we can adjust the condition. This doesn't affect the code path so I'm committing this.

Thanks for this @lanceh1412

Tweaked it slightly on commit because we can't be sure a code property exists and following how we've done this in commerce_yotpo_settings_form_send_orders

  • joelpittet committed f8b02ac on 7.x-1.x 
    Issue #2344269 by lanceh1412, joelpittet: check response code when...

  • joelpittet committed 223e6c8 on 7.x-1.x 
    Issue #2344269 by lanceh1412, joelpittet: check response code when...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.