Skip to content
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

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

Merged

Conversation

tcousin-ext
Copy link
Contributor

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

@norkunas
Copy link
Owner

norkunas commented Mar 1, 2022

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
Copy link
Contributor Author

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
Copy link
Owner

norkunas commented Mar 3, 2022

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
Copy link
Contributor Author

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
Copy link
Owner

norkunas commented Apr 8, 2022

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

@tcousin-ext tcousin-ext force-pushed the add_status_code_to_response_content branch from 651b0b4 to 456edec Compare April 8, 2022 14:12
@tcousin-ext
Copy link
Contributor Author

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

@norkunas
Copy link
Owner

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

@tcousin-ext
Copy link
Contributor Author

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
Copy link
Owner

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 😉

@tcousin-ext tcousin-ext force-pushed the add_status_code_to_response_content branch from 456edec to bae261b Compare May 17, 2022 08:07
@tcousin-ext
Copy link
Contributor Author

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

@norkunas
Copy link
Owner

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

@tcousin-ext tcousin-ext force-pushed the add_status_code_to_response_content branch from 05c4e5c to af7be4c Compare May 24, 2022 12:36
@tcousin-ext
Copy link
Contributor Author

It should be ok now ;)

@norkunas norkunas merged commit 0dddeca into norkunas:master May 24, 2022
@norkunas
Copy link
Owner

Thank you

@tcousin-ext
Copy link
Contributor Author

Thanks, you too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants