Skip to content
This repository was archived by the owner on Oct 29, 2020. It is now read-only.

Adds !empty check before json_decode#5616

Merged
chloealee merged 10 commits intoDoSomethingArchive:devfrom
chloealee:fix-reportback
Nov 4, 2015
Merged

Adds !empty check before json_decode#5616
chloealee merged 10 commits intoDoSomethingArchive:devfrom
chloealee:fix-reportback

Conversation

@chloealee
Copy link
Contributor

What does this PR do?

Adds a check to see if ns user is null and if so, do not try to json_decode to prevent error:

if (!empty($northstar_user_info->error)) {
        $error = sprintf("Error fetching Northstar user data, uid=%d: '%s'", $id, $northstar_user_info->error);
        watchdog_exception('northstar', new Exception($error));
      } else {
        cache_set('northstar_user_info_' . $id, $northstar_user_info, 'cache_northstar_user_info', REQUEST_TIME + 60*60*24);
}

How should this be manually tested?

endpoint: /api/v1/reportbacks.json?load_user=true should return user info. and not break. Works on local - tested by hard coding $northstar_user_info == NULL before new check and it by passed this correctly, return null for all user info in the api response. How do we test if this works with staging without merging?

Right now, the request to ns to get user information remains the same as what's in this code, so we know that it's successfully getting user info. on staging.

What are the relevant tickets?

#5408

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need some sort of logic too? So if $northstar_user ends up being null, how is the id, first_name, last_name etc being assigned that depends on getting an object from NS and not getting null?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use something like this:

$this->user['drupal_id'] = $data->uid;  // we by default pass the id to the new user property in the object

if(!empty($northstar_user)) {
  $this->user['id'] = $northstar_user->_id,
  $this->user['first_name'] = $northstar_user->first_name,
  $this->user['last_name'] = $northstar_user->last_name,
  $this->user['photo'] = $northstar_user->photo,
  $this->user['country'] = $northstar_user->country,
}

I think that could work...

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, scratch my suggestion above... it would make the returned data inconsistent and remove properties if null so wouldn't be as reliable...

instead, maybe just wrap the calls when assigning the properties on the user array to use the dosomething_helpers_isset():

...
  'first_name' => dosomething_helpers_isset($northstar_user->first_name),
...

@chloealee
Copy link
Contributor Author

@weerd I'm getting this error:

Notice: Undefined index: data in Reportback->build() (line 168 of /var/www/dev.dosomething.org/lib/modules/dosomething/dosomething_reportback/includes/Reportback.php).

which refers to this line:

$northstar_user = (object) @$northstar_user['data'][0];

I'm not sure why it's saying that it's an undefined index and I looked it up on google - if I add an isset() in front of it or null !== like many things suggest, it no longer returns the correct data. I know it's probably not great practice to use the @ operator but this is the only way I could get the notices to disappear in the logs. Is this the best solution to use?

@chloealee
Copy link
Contributor Author

wait - just checked to test again and looks like it isn't work now because of the rebase - hold off on checking this please and I'll keep you updated! Sorry! @weerd

@chloealee
Copy link
Contributor Author

@weerd okay now back in business! All should be working fine now. Just unsure about the comment above with the @ operator. Also, for some reason, dosomething_helpers_isset wasn't working correctly so changed it to a ternary operator. Let me know your thoughts!

@chloealee
Copy link
Contributor Author

@weerd for some reason, dosomething_helpers_isset(); was causing the notices in the logs. I swapped it out for just a regular isset(); check with a ternary operator and seemed to get rid of the notices! Now the logs are clear and correct data is returned!

@weerd
Copy link
Contributor

weerd commented Nov 4, 2015

I looked into it, and it's how the dosomething_helpers_isset() is used. If it's an object you are passing through to it, you need to also specify the $key for it :)

@chloealee
Copy link
Contributor Author

@weerd tested reportback-items endpoint as well and everything is working with no errors in the logs!

@weerd
Copy link
Contributor

weerd commented Nov 4, 2015

yaaaas!

Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't notice this before. No need for passing the ignore to the new static(). That's only needed for the Reportback class.

@chloealee
Copy link
Contributor Author

@weerd thank you! Made those two changes and tested again - still works! :)

@weerd
Copy link
Contributor

weerd commented Nov 4, 2015

Awesome-sauce! 💃

Merge it! 👍

@chloealee
Copy link
Contributor Author

🎉 🎉 🎉 BEST DAY OF MY LIFE!

chloealee added a commit that referenced this pull request Nov 4, 2015
Adds !empty check before json_decode
@chloealee chloealee merged commit cabf365 into DoSomethingArchive:dev Nov 4, 2015
@chloealee chloealee deleted the fix-reportback branch November 4, 2015 21:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants