Skip to content

Add null response handling#148

Merged
wscourge merged 9 commits intomainfrom
wiktor/ome-394-pipeline-php-library-timing-out-due-to-null-response
Dec 17, 2025
Merged

Add null response handling#148
wscourge merged 9 commits intomainfrom
wiktor/ome-394-pipeline-php-library-timing-out-due-to-null-response

Conversation

@wscourge
Copy link
Contributor

@wscourge wscourge commented Dec 4, 2025

  1. Update customer methods to accept both positional and array arguments.
  2. Add null response check and dedicated exception.

@wscourge wscourge requested a review from Copilot December 4, 2025 09:03
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds null response handling to the ChartMogul SDK and enhances customer attribute/tag methods to accept both array and positional arguments.

Key Changes:

  • Added NetworkException for null response scenarios with validation in Client and Retry classes
  • Updated customer methods (addTags, removeTags, addCustomAttributes, removeCustomAttributes, updateCustomAttributes) to accept both array and positional arguments while maintaining backward compatibility

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/Exceptions/NetworkException.php New exception class for network-related failures
src/Http/Client.php Added null response checks in send() and handleResponse() methods
src/Http/Retry.php Added null result validation for both zero-retry and retry scenarios
src/Customer.php Updated tag and custom attribute methods to support both array and positional arguments
tests/Unit/Http/ClientNullResponseTest.php Tests for null response handling in Client
tests/Unit/Http/RetryNullHandlingTest.php Tests for null response handling in Retry
tests/Unit/CustomerArrayAttributesTest.php Comprehensive tests for new array argument support in customer methods

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

wscourge and others added 5 commits December 4, 2025 10:21
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@wscourge wscourge marked this pull request as ready for review December 4, 2025 10:15
@wscourge wscourge requested a review from Copilot December 4, 2025 10:15
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

public function handleResponse(?ResponseInterface $response)
{
// Handle null response
if ($response === null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

any particular reason why it should be handled like that here? It just looks a little bit weird to allow $response to be null just to raise an exception if it's null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@swember as I understand this is the exact underlying issue: right now, we don't recognise null responses and it leads to timeouts. Raising an exception solves this.

I am not an PHP virtuoso though, and I'm very open to different suggestions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After consulting with @swember I have removed this check. I'll be looking into all API responses to make sure none of them return null.

@swember please take another look at the PR, I'd like to proceed with releasing the rest of changes.

@wscourge wscourge requested a review from swember December 17, 2025 13:27
@wscourge wscourge merged commit a2b108a into main Dec 17, 2025
6 checks passed
@wscourge wscourge deleted the wiktor/ome-394-pipeline-php-library-timing-out-due-to-null-response branch December 17, 2025 15:20
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.

2 participants