helpscout / helpscout-api-php

PHP Wrapper for the Help Scout API
MIT License
98 stars 62 forks source link

RestClient::encodeEntity() should allow for errors #277

Closed miken32 closed 8 months ago

miken32 commented 3 years ago

The return type for this method is declared as string but in case of an error json_encode() will return false. The return type should be updated at the very least, or an exception thrown with contents of json_last_error_msg() included.

I came across this error recently, though I have no record of the arguments involved or why the Conversation object I passed couldn't be encoded.

[2021-06-18 20:11:56] production.ERROR: HelpScout\Api\Http\RestClient::encodeEntity(): Return value must be of type string, bool returned {"exception":"[object] (TypeError(code: 0): HelpScout\\Api\\Http\\RestClient::encodeEntity(): Return value must be of type string, bool returned at /xxxx/vendor/helpscout/api/src/Http/RestClient.php:161)
[stacktrace]
#0 /xxxx/vendor/helpscout/api/src/Http/RestClient.php(68): HelpScout\\Api\\Http\\RestClient->encodeEntity()
#1 /xxxx/vendor/helpscout/api/src/Conversations/ConversationsEndpoint.php(35): HelpScout\\Api\\Http\\RestClient->createResource()
#2 /xxxx/app/Helpscout.php(209): HelpScout\\Api\\Conversations\\ConversationsEndpoint->create()
miken32 commented 3 years ago

Also worth noting that since PHP 7.3, JSON functions can be passed the JSON_THROW_ON_ERROR flag so a JsonException can be thrown automatically. This could be caught by your code and handled appropriately.

bkuhl commented 3 years ago

🤔 Looking at the stack trace, it looks like this error was encountered when trying to create a conversation within Help Scout. The extract() that supplies json_encode() is strongly typed to return an array so it seems like a failure here and the other entities within Conversation::extract() also use the entity's extract() methods. It's unclear to me at the moment what could have caused such an error and we'd need some more details about how the convo was being created to understand what's going on.

That being said, you are correct that we should throw error if json can't be encoded.

miken32 commented 3 years ago

Nothing too interesting about the conversation creation:

$customer = (new Customer())->addEmail($email);
$thread = (new CustomerThread())
    ->setText($body)
    ->setCustomer($customer);

$convo = (new Conversation())
    ->setSubject($subject)
    ->setPreview($body)
    ->setStatus(Status::ACTIVE)
    ->setType("email")
    ->addThread($thread)
    ->setCustomer($customer);
$convo->setMailboxId($mbox);
$convo_id = $this->helpscout->conversations()->create($convo);

A couple of custom fields added as well, but nothing too complicated. Whatever was returned by Conversation::extract() couldn't be converted for some reason. This is the same code that's been used for hundreds of ticket creations and this is the first we've seen of this error.

bkuhl commented 3 years ago

v3.5.2 has been tagged with changes that'll throw an exception in that scenario. If you're able to update to this version, we can get a little more clarity as to what the underlying issue is next time this occurs.

miguelrs commented 8 months ago

Closing this issue due to inactivity. Please feel free to reopen if needed!