norkunas / onesignal-php-api

OneSignal API for PHP
MIT License
233 stars 82 forks source link

add status code to response content of send request method #165

Closed tcousin-ext closed 2 years ago

tcousin-ext commented 2 years ago

Hello @norkunas, I added the http status code in the sendRequest method for better handle of the api responses. Thank you :)

norkunas commented 2 years ago

Hello, thank you for your contribution.

I'm not sure that this would be the right way to expose status code. Because if OneSignal would return that field with some other data then you'd have dead code.

tcousin-ext commented 2 years ago

Hi @norkunas, thank you for your answer.

I can rename the key to 'http-code' or something more specific to prevent loosing data. What do you think ?

norkunas commented 2 years ago

Maybe _status_code would be good then. But why not just check if there is errors set and then assume that request failed?

tcousin-ext commented 2 years ago

Hi @norkunas it appears that the validation is blocked by php cs fixer and/or phpstan. Do you have any infos about that ? Thank you in advance

norkunas commented 2 years ago

Could you try to amend or force push to retrigger CI?

tcousin-ext commented 2 years ago

Hi again @norkunas, can you approve the merge ? Thank you in advance

norkunas commented 2 years ago

Hey, you didn't answer my one question yet :) also please squash your commits

tcousin-ext commented 2 years ago

Sorry, it slipped my mind. We need to call many onesignal apps and we want to retrieve the error status to analyze the cause of failure more easily

norkunas commented 2 years ago

Sorry, it slipped my mind. We need to call many onesignal apps and we want to retrieve the error status to analyze the cause of failure more easily

All right, so please squash your commits and I'll merge :wink:

tcousin-ext commented 2 years ago

Hello @norkunas i hope you're doing ok :) Is my MR ready to be approved ? Thank you

norkunas commented 2 years ago

Hi, you still got a merge commit, which should not be there :)

tcousin-ext commented 2 years ago

It should be ok now ;)

norkunas commented 2 years ago

Thank you

tcousin-ext commented 2 years ago

Thanks, you too