Skip to content

Conversation

@lohanidamodar
Copy link
Contributor

@lohanidamodar lohanidamodar commented Nov 17, 2025

  • Support setting base URL

Summary by CodeRabbit

  • New Features
    • Base URL configuration: set and retrieve a base URL so requests are automatically prefixed, with path appending, query parameter handling, and trailing-slash normalization; absolute URLs remain unaffected.
  • Tests
    • Added tests covering base URL get/set, relative/absolute paths, path appending, query handling, and trailing-slash normalization.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Nov 17, 2025

Walkthrough

Adds a protected string $baseUrl = '' property to Client. Adds public setBaseUrl(string $baseUrl): self (fluent) and getBaseUrl(): string methods. The fetch method is updated to prefix the supplied $url with $this->baseUrl before request processing so relative URLs are resolved against the configured base. Adds PHPUnit tests in tests/ClientTest.php covering: setting/getting base URL, relative-path resolution, appending paths, absolute-URL behavior when base is set, query parameter handling, and trailing-slash normalization.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Files changed: src/Client.php, tests/ClientTest.php
  • Types of changes: small API additions (property + accessor methods), moderate logic change in fetch, and multiple new unit tests

Areas to review:

  • Correct concatenation and normalization of baseUrl and request $url (leading/trailing slashes)
  • Behavior for absolute URLs when a base URL is set
  • Query string preservation and merging behavior
  • Test coverage correctness and assumptions in tests/ClientTest.php

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Feat: support base URL' directly and clearly describes the main change: adding base URL support to the HTTP client.
Docstring Coverage ✅ Passed Docstring coverage is 81.82% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat-support-base-url

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dd74515 and 5702098.

📒 Files selected for processing (1)
  • src/Client.php (2 hunks)

private array $retryStatusCodes = [500, 503];
private mixed $jsonEncodeFlags;

protected string $baseUrl = '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's update tests to try and set base URL

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
tests/ClientTest.php (1)

292-309: Consider adding a comment to clarify the empty string URL.

The test correctly validates that an empty string relative URL resolves to the base URL. However, a brief comment explaining this intent would improve readability.

Apply this diff:

     $client = new Client();
     $client->setBaseUrl('http://localhost:8000');
 
+    // Empty string should resolve to base URL
     $resp = $client->fetch(
         url: '',
         method: Client::METHOD_GET
     );
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 330fabd and 9ed8075.

📒 Files selected for processing (2)
  • src/Client.php (3 hunks)
  • tests/ClientTest.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/Client.php
🧰 Additional context used
🧬 Code graph analysis (1)
tests/ClientTest.php (2)
src/Client.php (4)
  • Client (9-441)
  • setBaseUrl (42-46)
  • getBaseUrl (48-51)
  • fetch (255-360)
src/Response.php (2)
  • getStatusCode (70-73)
  • json (88-95)
🔇 Additional comments (5)
tests/ClientTest.php (5)

274-286: LGTM! Clear and straightforward test.

The setter/getter test is well-structured and follows the established pattern for similar tests in this file.


315-331: LGTM! Effective test for path appending.

The test properly validates that relative paths are correctly appended to the base URL by using the existing redirect endpoint.


337-354: LGTM! Important test case for absolute URL handling.

This test correctly validates that absolute URLs bypass the base URL, which is essential behavior to prevent incorrect URL construction.


360-384: LGTM! Good coverage of query parameter handling.

The test properly validates that query parameters work correctly with base URLs, ensuring the URL construction logic doesn't interfere with query string handling.


390-407: LGTM! Essential test for slash normalization.

This test correctly validates the trailing/leading slash normalization logic, preventing double slashes in the constructed URL. The comment on line 394 is helpful.

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.

3 participants