chore: update suggestions method to return prices#43
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReworked OpenSRS registrar adapter: suggest logic expanded to support limit and price filters, multi-service calls, XML sanitization/parsing, and service_override; buildEnvelop accepts nullable domain; constructor/API credentials adjusted; Registrar public suggest signature simplified; composer allow-plugins added; tests updated and nameservers changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant SDK as OpenSRS SDK
participant API as OpenSRS API
Client->>SDK: suggest(query, tlds, limit?, priceMax?, priceMin?)
SDK->>API: name_suggest (services: ["suggestion","premium","lookup"], service_override?)
API-->>SDK: XML response
rect rgba(230,245,255,0.9)
note right of SDK: sanitize XML response
SDK->>SDK: sanitize(xml)
end
par Parse suggestions
SDK->>SDK: XPath -> suggestion nodes
SDK-->>SDK: suggestion entries (domain, available, type)
and Parse premiums
SDK->>SDK: XPath -> premium nodes
SDK-->>SDK: premium entries (domain, price, available, type="premium")
end
SDK->>SDK: merge entries by domain -> unified array
SDK-->>Client: consolidated suggestions array
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
composer.json(1 hunks)src/Domains/Registrar/OpenSRS.php(3 hunks)tests/OpenSRSTest.php(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.28.0)
tests/OpenSRSTest.php
[high] 17-17: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (1)
composer.json (1)
35-41: LGTM on scoped plugin allowancesThanks for tightening the Composer plugin policy—keeping the
allow-pluginslist narrowed to justphp-http/discoveryandtbachert/spiis the right move for Composer 2+.
ChiragAgg5k
force-pushed
the
fix-opensrs-issues
branch
from
9984a2d to
3858f87
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the OpenSRS domain suggestion functionality to include premium domains alongside regular suggestions. The changes merge premium domain suggestions with regular suggestions into a unified response format.
- Enhanced domain suggestion service to include premium domain lookups
- Updated response format to include domain type, availability status, and pricing information
- Added composer configuration to allow specific plugins
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Domains/Registrar/OpenSRS.php | Refactored suggest() method to process both regular and premium domain suggestions with enhanced data structure |
| composer.json | Added plugin configuration to allow php-http/discovery and tbachert/spi plugins |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
ChiragAgg5k
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/Domains/Registrar/OpenSRS.php (3)
326-331: Normalize availability status for premium entriesBe defensive against variants ('Available', 'true', '1').
- case 'status': - $available = $value === 'available'; + case 'status': + $available = in_array(strtolower($value), ['available', 'true', '1'], true); break;
257-260: Optional: If 'lookup' stays, consider parsing its resultsCurrently, 'lookup' is requested but ignored. Either remove it to save a round-trip or parse it into $items similarly to 'premium' (domain/status). I can draft the parsing if you confirm the expected XML shape.
44-44: Minor: Header formatting consistencyConsider adding a space after the colon in headers for consistency (e.g., 'Content-Type: text/xml', 'X-Signature: ').
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Domains/Registrar/OpenSRS.php(5 hunks)
🧰 Additional context used
🪛 GitHub Actions: Tests
src/Domains/Registrar/OpenSRS.php
[error] 97-97: Authentication Error.
[error] 375-375: Authentication Error.
[error] 419-419: Authentication Error.
🔇 Additional comments (3)
src/Domains/Registrar/OpenSRS.php (3)
309-342: LGTM on premium parsing and mergeParsing by keyed fields and merging into $items keyed by domain is solid. Price precision retained as float.
641-641: LGTM: buildEnvelop now tolerates null domainThe nullable domain param prevents emitting an empty domain tag. Looks correct.
Ensure all send() callers either pass domain explicitly when required by the action or include it under attributes as appropriate.
257-260: Use dt_array for services (no change needed)
OpenSRS name_suggest expects a dt_array of service names (lookup, suggestion, premium, personal_names, premium_brokered_transfer, premium_make_offer, etc.), not a dt_assoc of boolean flags.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/Domains/Registrar.php (1)
26-29: Confirm intentional narrowing of suggest() inputRegistrar::suggest now requires array, while Adapter allows array|string. If callers previously passed strings, this is a breaking change. Either keep array-only and document it clearly, or accept array|string for convenience.
- public function suggest(array $query, array $tlds = []): array + public function suggest(array|string $query, array $tlds = []): array { return $this->adapter->suggest($query, $tlds); }src/Domains/Registrar/Adapter.php (1)
24-29: Docblock-implementation mismatchDoc says array $query but signature allows array|string. Update docblock for accuracy.
- /** - * @param array $query - * @param array $tlds - * @return array - */ - abstract public function suggest(array|string $query, array $tlds = []): array; + /** + * @param array|string $query + * @param array $tlds + * @return array + */ + abstract public function suggest(array|string $query, array $tlds = []): array;src/Domains/Registrar/OpenSRS.php (2)
230-246: Docblock mentions 'lookup' but results aren’t parsedEither parse ‘lookup’ results or drop it from services and adjust docs. Suggest removing to ship safely now.
- * - `lookup`: Performs availability checks on exact matches and variations
255-258: Request only services you parseCurrently ‘lookup’ is requested but unused. Remove it or add parsing for it. Minimal fix: remove it.
'services' => [ - 'suggestion', // Algorithmic domain suggestions - 'premium', // Premium domain names with pricing - 'lookup' // Availability lookup for exact matches + 'suggestion', // Algorithmic domain suggestions + 'premium', // Premium domain names with pricing ],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(1 hunks)src/Domains/Registrar.php(1 hunks)src/Domains/Registrar/Adapter.php(1 hunks)src/Domains/Registrar/OpenSRS.php(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/Domains/Registrar/Adapter.php (2)
src/Domains/Registrar.php (2)
suggest(26-29)tlds(31-34)src/Domains/Registrar/OpenSRS.php (2)
suggest(246-343)tlds(345-349)
src/Domains/Registrar.php (2)
src/Domains/Registrar/Adapter.php (2)
suggest(28-28)tlds(33-33)src/Domains/Registrar/OpenSRS.php (2)
suggest(246-343)tlds(345-349)
src/Domains/Registrar/OpenSRS.php (2)
src/Domains/Registrar.php (2)
suggest(26-29)tlds(31-34)src/Domains/Registrar/Adapter.php (2)
suggest(28-28)tlds(33-33)
🪛 GitHub Actions: Tests
src/Domains/Registrar/OpenSRS.php
[error] 97-97: Authentication Error.
[error] 373-373: Authentication Error.
[error] 417-417: Authentication Error.
🔇 Additional comments (6)
README.md (1)
131-131: Docs synced with API changeSignature reflects array-only query; matches Registrar::suggest. If adapters accept string too, consider noting that only Registrar enforces array.
src/Domains/Registrar/OpenSRS.php (5)
281-292: Avoid positional indexing in XPath parsingRelying on item[0]/item[1] is brittle. Select by key.
- foreach ($suggestionElements as $element) { - $item = $element->xpath('dt_assoc/item'); - $domain = (string) $item[0]; - $available = (string) $item[1] === 'available'; - - $items[$domain] = [ - 'available' => $available, - 'price' => null, - 'type' => 'suggestion' - ]; - } + foreach ($suggestionElements as $element) { + $domainNode = $element->xpath('dt_assoc/item[@key="domain"]'); + $statusNode = $element->xpath('dt_assoc/item[@key="status"] | dt_assoc/item[@key="availability"]'); + $domain = isset($domainNode[0]) ? (string)$domainNode[0] : null; + $status = isset($statusNode[0]) ? strtolower((string)$statusNode[0]) : ''; + if ($domain) { + $items[$domain] = [ + 'available' => in_array($status, ['available', 'true', '1'], true), + 'price' => null, + 'type' => 'suggestion', + ]; + } + }
327-329: LGTM: price precision preservedCasting to float retains cents; fixes prior truncation issue.
639-639: LGTM: buildEnvelop domain is now nullableCleaner envelope construction; avoids emitting empty domain field.
42-45: Critical: X-Username must use reseller username, not apiSecretThis likely causes auth failures (matches CI “Authentication Error”). Use $this->user['username'] and keep header formatting consistent.
- $this->headers = [ - 'Content-Type:text/xml', - 'X-Username: '.$this->apiSecret, - ]; + $this->headers = [ + 'Content-Type: text/xml', + 'X-Username: '.$this->user['username'], + ];
264-266: Guard against empty/failed HTTP response before XML parsingcurl_exec may return false/empty; sanitizeResponse will fatal on xpath. Add a guard.
- $result = $this->send($message); - $result = $this->sanitizeResponse($result); + $result = $this->send($message); + if (!is_string($result) || $result === '') { + throw new Exception('Empty or invalid response from OpenSRS'); + } + $result = $this->sanitizeResponse($result);
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/OpenSRSTest.php (2)
44-53: Consider cleanup for purchased domains.The test purchases a real domain but doesn't clean it up afterward. Over time, repeated test runs could accumulate purchased domains in the test account, potentially causing issues with account limits or incurring costs.
Consider adding a
tearDown()method or an explicit cleanup step to delete or cancel test domains after use, if the OpenSRS test environment supports it.
123-137: Consider removing commented code.Explicitly skipping this test with a clear explanation is better than having a test that always fails. However, consider removing the commented-out line 125 to reduce noise, as the comments below already explain why this test cannot run.
Apply this diff to remove the commented code:
public function testTransfer(): void { - // $result = $this->client->transfer($domain, self::purchaseContact()); - // This will always fail mainly because it's a test env, // but also because: // - we use random domains to test
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/OpenSRSTest.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/OpenSRSTest.php (2)
src/Domains/Registrar/OpenSRS.php (6)
OpenSRS(8-720)available(70-85)getDomain(351-386)updateNameservers(103-132)updateDomain(388-421)renew(423-467)src/Domains/Registrar/Adapter.php (3)
available(13-13)getDomain(39-39)renew(46-46)
🔇 Additional comments (4)
tests/OpenSRSTest.php (4)
36-42: LGTM!The test correctly generates a random domain and verifies availability. Using dynamic domain generation ensures test reliability and isolation.
116-118: Good addition: graceful handling of insufficient funds.The conditional skip for accounts without sufficient funds is a practical improvement that prevents test failures due to test environment limitations.
23-23: Avoid hardcoded test domain
The hardcoded domainkffsfudlvc.netis only used intests/OpenSRSTest.phpand may cause flaky failures if it isn’t pre-registered or becomes unavailable. Consider one of the following:
- Document that
kffsfudlvc.netmust be pre-registered in the test OpenSRS account- Dynamically purchase a domain in
testPurchase()and reuse it for dependent tests- Mock the OpenSRS client to remove reliance on external state
63-68: Ensure testCancelPurchase is execution-order independent.
testCancelPurchase() calls cancel_pending_orders and asserts true; without a @Depends or creating a purchase here, it may be flaky if no pending orders exist. Consider either:
- Adding
@depends testPurchase- Invoking a purchase within this test
- Mocking the API response to guarantee success
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Domains/Registrar/Adapter.php (1)
23-31: Docblock should reflectarray|stringquery inputThe signature now accepts
array|string $query, but the docblock still documents@param array $query. Please update the annotation so consumers don’t miss the new scalar input support.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Domains/Registrar/Adapter.php(1 hunks)src/Domains/Registrar/OpenSRS.php(8 hunks)tests/OpenSRSTest.php(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/Domains/Registrar/Adapter.php (2)
src/Domains/Registrar/OpenSRS.php (2)
suggest(238-379)tlds(381-385)src/Domains/Registrar.php (2)
suggest(26-29)tlds(31-34)
tests/OpenSRSTest.php (1)
src/Domains/Registrar/OpenSRS.php (4)
OpenSRS(8-801)available(68-83)suggest(238-379)renew(459-503)
src/Domains/Registrar/OpenSRS.php (3)
src/Domains/Registrar.php (3)
__construct(11-14)suggest(26-29)tlds(31-34)src/Domains/Adapter.php (1)
__construct(28-39)src/Domains/Registrar/Adapter.php (2)
suggest(31-31)tlds(36-36)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/OpenSRSTest.php (1)
93-105: Loosen theassertCount(10, ...)expectation.OpenSRS only promises "up to" the requested maximum and may return fewer than 10 suggestions in practice. The strict
assertCount(10, $result)on Line 105 will fail intermittently.Apply this diff to make the assertion more robust:
- $this->assertCount(10, $result); + $this->assertLessThanOrEqual(10, count($result)); + $this->assertGreaterThan(0, count($result));
🧹 Nitpick comments (1)
tests/OpenSRSTest.php (1)
15-33: Consider the test domain dependency architecture.The refactoring creates a split approach where:
testAvailableandtestPurchasegenerate random domains (good isolation)testDomainInfo,testUpdateNameservers,testUpdateDomain, andtestRenewDomaindepend on the hard-coded'kffsfudlvc.net'fromsetUp()For consistency, consider one of these approaches:
- Purchase-based approach: Have
setUp()calltestPurchase()to create a domain and store it in$this->domain, so all tests use a freshly purchased domain- Fixture approach: Document that
'kffsfudlvc.net'must be pre-provisioned in the test account- Full isolation: Make each test independent by generating/purchasing domains as needed
The current mixed approach works but may confuse future maintainers about which domains exist and when.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(2 hunks)tests/OpenSRSTest.php(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🧬 Code graph analysis (1)
tests/OpenSRSTest.php (1)
src/Domains/Registrar/OpenSRS.php (3)
OpenSRS(8-801)suggest(238-379)renew(459-503)
🔇 Additional comments (10)
tests/OpenSRSTest.php (10)
35-41: LGTM!Generating a random domain for each availability check is good practice, reducing test interdependencies and improving test isolation.
43-52: LGTM!Generating a random domain for purchase tests improves test isolation and reduces conflicts between test runs.
54-60: Verify the hard-coded domain exists in the test environment.The test now depends on the hard-coded domain
'kffsfudlvc.net'fromsetUp(). UnliketestAvailableandtestPurchasewhich generate random domains, this test will fail if the domain doesn't exist or isn't properly configured in the test environment.Consider either:
- Documenting that this domain must be pre-provisioned in the OpenSRS test account
- Using the domain returned from
testPurchase()via test dependencies- Generating and purchasing a domain in
setUp()for use across tests
69-91: LGTM! Clear test coverage for basic suggestions.The test correctly validates that premium domains have prices and non-premium domains have
nullprices. Good separation of concerns.
107-128: LGTM! Premium price filtering test is well-structured.The test correctly uses
assertLessThanOrEqual(10, count($result))to avoid brittleness and validates both the domain type and price ranges. The defensive null check on Line 124 is appropriate.
131-139: LGTM!The change to use
$this->domainis consistent with other test method updates. Note that this test shares the same dependency on the hard-coded domain astestDomainInfo.
141-154: LGTM!The method signature change is consistent with the broader test refactoring. Shares the same domain dependency as other tests.
156-165: LGTM! Skip logic improves test robustness.The addition of skip logic for insufficient funds (Lines 160-162) is a good practice for handling test environment limitations without failing the entire suite.
167-181: LGTM! Well-documented test skip.The comprehensive comment clearly explains the technical limitations that prevent transfer testing in this environment. Good documentation practice.
208-219: LGTM! Static declaration resolves past issue.Making
generateRandomString()static aligns with its usage throughout the test class (self::generateRandomString()). This addresses the fatal error that would occur in PHP 8+ when calling a non-static method statically.
Summary by CodeRabbit
New Features
Documentation
Chores
Tests