Fix Name.com error handling for invalid domains and unsupported TLDs#60
Merged
ChiragAgg5k merged 10 commits intomasterfrom Mar 2, 2026
Merged
Fix Name.com error handling for invalid domains and unsupported TLDs#60ChiragAgg5k merged 10 commits intomasterfrom
ChiragAgg5k merged 10 commits intomasterfrom
Conversation
- Return false instead of throwing when checking availability for invalid domains - Add UnsupportedTldException for unsupported TLDs in purchase and transfer - Change error checks in transfer to use && instead of || for stricter matching - Add test for transferring unsupported TLD (.in)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR centralizes API error handling by introducing explicit error keys and an ERROR_MAP with a private matchError(Exception) helper used across adapters (NameCom, OpenSRS, Mock). Adapters now map API errors to specific registrar exceptions (including new InvalidAuthCodeException and UnsupportedTldException). The pricing API now uses a new Price value object (float price + bool premium); getPrice return types and cache formats updated accordingly. transfer signatures were simplified to transfer(string $domain, string $authCode, ?float $purchasePrice = null), removing contacts/nameservers. Tests and the Registrar/Adapter interfaces were updated to match these changes. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🧹 Nitpick comments (1)
src/Domains/Registrar/Adapter/NameCom.php (1)
181-185: Extract duplicate unsupported-TLD check and add HTTP code validation.This check is duplicated identically at lines 181–183 and 235–237. Additionally, the mapping is message-only while other error handlers in this file (lines 230, 241) validate HTTP status codes—this inconsistency risks misclassifying unrelated errors if message text overlaps.
Name.com API returns 422 Unprocessable Entity for unsupported TLDs; add this code check and extract a helper method:
♻️ Suggested refactor
+ private function isUnsupportedTldError(int $code, string $errorLower): bool + { + return $code === 422 && + ( + str_contains($errorLower, strtolower(self::ERROR_MESSAGE_UNSUPPORTED_TLD)) || + str_contains($errorLower, strtolower(self::ERROR_MESSAGE_UNSUPPORTED_TRANSFER)) + ); + } ... - if (str_contains($errorLower, strtolower(self::ERROR_MESSAGE_UNSUPPORTED_TLD)) || - str_contains($errorLower, strtolower(self::ERROR_MESSAGE_UNSUPPORTED_TRANSFER)) - ) { + if ($this->isUnsupportedTldError($code, $errorLower)) { throw new UnsupportedTldException($message, $e->getCode(), $e); }Also applies to: 235–239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Domains/Registrar/Adapter/NameCom.php` around lines 181 - 185, Extract the duplicated unsupported-TLD detection into a private helper on the NameCom adapter (e.g., private function isUnsupportedTldError(Throwable $e, string $message): bool) that checks both the error message substrings (self::ERROR_MESSAGE_UNSUPPORTED_TLD, self::ERROR_MESSAGE_UNSUPPORTED_TRANSFER) and the HTTP status code 422 from the exception/response; then replace both duplicated blocks with a call to this helper and throw UnsupportedTldException($message, $e->getCode(), $e) when it returns true so message+HTTP-code validation is consistent with other handlers like those around the existing 230/241 checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Registrar/NameComTest.php`:
- Around line 169-175: The test testTransferUnsupportedTldDotXYZ relies on live
Name.com behavior and should be made deterministic: in
tests/Registrar/NameComTest.php replace the external-dependent check by either
(A) using a TLD that your environment/contract guarantees is unsupported
(replace the '.xyz' generation in testTransferUnsupportedTldDotXYZ) or (B) mock
the provider client/adapter call so $this->registrar->transfer throws
UnsupportedTldException regardless of real API responses; to do option B,
stub/mock the Name.com API client or the registrar adapter method invoked by
transfer (the same code path used by $this->registrar->transfer and tested with
getPurchaseContact()/generateRandomString()) to throw UnsupportedTldException,
then assert the exception as before.
---
Nitpick comments:
In `@src/Domains/Registrar/Adapter/NameCom.php`:
- Around line 181-185: Extract the duplicated unsupported-TLD detection into a
private helper on the NameCom adapter (e.g., private function
isUnsupportedTldError(Throwable $e, string $message): bool) that checks both the
error message substrings (self::ERROR_MESSAGE_UNSUPPORTED_TLD,
self::ERROR_MESSAGE_UNSUPPORTED_TRANSFER) and the HTTP status code 422 from the
exception/response; then replace both duplicated blocks with a call to this
helper and throw UnsupportedTldException($message, $e->getCode(), $e) when it
returns true so message+HTTP-code validation is consistent with other handlers
like those around the existing 230/241 checks.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Domains/Registrar/Adapter/NameCom.phpsrc/Domains/Registrar/Exception/UnsupportedTldException.phptests/Registrar/NameComTest.php
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/Registrar/NameComTest.php (1)
161-175:⚠️ Potential issue | 🟠 MajorUnsupported-TLD transfer tests are non-deterministic against a live provider.
These assertions depend on current Name.com TLD support at runtime, so future provider-side changes can break tests for the wrong reason. Prefer stubbing/mocking the adapter error path for deterministic exception mapping tests, or gate provider-dependent cases behind capability checks.
#!/bin/bash # Verify this test class is integration-style (live endpoint + direct transfer calls) and lacks mocking for unsupported TLD paths. rg -n -C2 "api\.dev\.name\.com|testTransferUnsupportedTldDot(In|XYZ)|expectException\(UnsupportedTldException::class\)|->transfer\(" tests/Registrar/NameComTest.php rg -n -C2 "createMock|getMockBuilder|MockObject" tests/Registrar/NameComTest.php🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Registrar/NameComTest.php` around lines 161 - 175, These tests (testTransferUnsupportedTldDotIn, testTransferUnsupportedTldDotXYZ) call the live registrar->transfer and are non-deterministic; instead stub the registrar/adapter to deterministically exercise the UnsupportedTldException path by using createMock/getMockBuilder to return a mock adapter whose transfer(...) throws UnsupportedTldException, then inject that mock into the test subject and assert the exception; alternatively, if a capability method exists (e.g. supportsTld or isTldSupported), gate the live test with that check and skip when the provider actually supports the TLD.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Domains/Registrar/Adapter/NameCom.php`:
- Around line 117-122: The available() method in NameCom.php only treats
self::ERROR_INVALID_DOMAINS as unavailable but should also treat
unsupported/invalid single-domain errors as unavailable; update the switch in
the available() error handling (the switch on $this->matchError($e) inside the
available() method) to return false for the additional error codes
self::ERROR_UNSUPPORTED_TLD and self::ERROR_INVALID_DOMAIN (in addition to
self::ERROR_INVALID_DOMAINS) so those errors do not get re-thrown.
---
Duplicate comments:
In `@tests/Registrar/NameComTest.php`:
- Around line 161-175: These tests (testTransferUnsupportedTldDotIn,
testTransferUnsupportedTldDotXYZ) call the live registrar->transfer and are
non-deterministic; instead stub the registrar/adapter to deterministically
exercise the UnsupportedTldException path by using createMock/getMockBuilder to
return a mock adapter whose transfer(...) throws UnsupportedTldException, then
inject that mock into the test subject and assert the exception; alternatively,
if a capability method exists (e.g. supportsTld or isTldSupported), gate the
live test with that check and skip when the provider actually supports the TLD.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/Domains/Registrar.phpsrc/Domains/Registrar/Adapter.phpsrc/Domains/Registrar/Adapter/Mock.phpsrc/Domains/Registrar/Adapter/NameCom.phpsrc/Domains/Registrar/Adapter/OpenSRS.phpsrc/Domains/Registrar/Exception/InvalidAuthCodeException.phptests/Registrar/Base.phptests/Registrar/MockTest.phptests/Registrar/NameComTest.phptests/Registrar/OpenSRSTest.php
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Domains/Registrar/Adapter/OpenSRS.php (1)
140-150:⚠️ Potential issue | 🔴 CriticalOpenSRS will reject domain transfers due to empty
contact_set.The OpenSRS XCP API requires
contact_setin theSW_REGISTERrequest forreg_type=transfer. For most common TLDs (.com, .net, .org, .info, .biz),contact_setmust include owner, admin, billing, and tech contacts. However,transfer()at line 213 callsregister()with an empty contacts array, causing the API call to fail. There is no mechanism to pass contacts during a transfer, and no special handling exists for thechange_contactflag that OpenSRS supports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Domains/Registrar/Adapter/OpenSRS.php` around lines 140 - 150, The register() method is currently constructing a SW_REGISTER payload with an empty contact_set when transfer() calls it with no contacts, which causes OpenSRS to reject transfers; modify transfer() (the caller of register()) to accept and forward a complete contacts array (owner, admin, billing, tech) to register(), and update register() to set the appropriate OpenSRS transfer flags (e.g., include a populated 'contact_set' in 'attributes' for reg_type='transfer' or set the API's change_contact/change_contacts flag when using the caller's user data), ensuring register() uses the provided contacts array when regType === 'transfer' rather than leaving it empty.
♻️ Duplicate comments (1)
src/Domains/Registrar/Adapter/NameCom.php (1)
118-121:⚠️ Potential issue | 🟠 Major
available()still misses unsupported/invalid single-domain error paths.Right now only
ERROR_INVALID_DOMAINSmaps tofalse.ERROR_INVALID_DOMAINandERROR_UNSUPPORTED_TLDcan still throw, which conflicts with the invalid-domain availability behavior targeted by this PR.Proposed fix
} catch (Exception $e) { switch ($this->matchError($e)) { case self::ERROR_INVALID_DOMAINS: + case self::ERROR_INVALID_DOMAIN: + case self::ERROR_UNSUPPORTED_TLD: return false; default: throw $e; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Domains/Registrar/Adapter/NameCom.php` around lines 118 - 121, The available() method's error handling only maps self::ERROR_INVALID_DOMAINS to false, leaving self::ERROR_INVALID_DOMAIN and self::ERROR_UNSUPPORTED_TLD to bubble up; update the switch in available() (the switch over $this->matchError($e) in NameCom.php) to treat ERROR_INVALID_DOMAIN and ERROR_UNSUPPORTED_TLD the same as ERROR_INVALID_DOMAINS (return false) so unsupported/invalid single-domain errors are handled consistently.
🧹 Nitpick comments (1)
src/Domains/Registrar/Adapter/NameCom.php (1)
396-397: Consider mapping unsupported-TLD pricing failures toPriceNotFoundException.If Name.com returns unsupported-TLD for
:getPricing, this currently falls through toDomainsExceptioninstead ofPriceNotFoundException.Suggested adjustment
- case in_array($this->matchError($e), [self::ERROR_NOT_FOUND, self::ERROR_INVALID_DOMAIN]): + case in_array( + $this->matchError($e), + [self::ERROR_NOT_FOUND, self::ERROR_INVALID_DOMAIN, self::ERROR_UNSUPPORTED_TLD], + true + ): throw new PriceNotFoundException($message, $code, $e);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Domains/Registrar/Adapter/NameCom.php` around lines 396 - 397, The current error handling in NameCom.php maps matchError($e) values [self::ERROR_NOT_FOUND, self::ERROR_INVALID_DOMAIN] to PriceNotFoundException but misses the unsupported-TLD case; update the switch/case that checks matchError($e) (used by getPricing) to also include self::ERROR_UNSUPPORTED_TLD (or add a separate case for ERROR_UNSUPPORTED_TLD) so that when Name.com returns unsupported-TLD the code throws PriceNotFoundException($message, $code, $e) instead of falling through to DomainsException.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Domains/Registrar/Adapter/Mock.php`:
- Around line 264-267: The cache lookup in Mock:: (the block using
$this->cache->load($domain, $ttl)) keys only by domain and thus can return
prices that belong to a different regType or periodYears; update the caching
logic to include regType and periodYears in the cache key (or save and validate
those fields on the cached payload) before returning a new
Price($cached['price'], $cached['premium'] ?? false), and apply the same change
to the analogous block around lines 301-305 so cached entries are specific to
domain+regType+periodYears.
In `@src/Domains/Registrar/Adapter/OpenSRS.php`:
- Around line 455-458: The cache key in OpenSRS::getPrice is too coarse
(currently only uses $domain) and can return incorrect prices for different
pricing dimensions; update the cache key generation in getPrice (and the
corresponding save logic around the later block at 485-486) to include the
pricing dimensions such as $periodYears and $regType (or regType-equivalent
variable) so the key is unique per price variant (for example concatenate domain
+ periodYears + regType), and keep the existing shape handling (checking
['price'] and ['premium']) when loading and saving to ensure correct retrieval.
---
Outside diff comments:
In `@src/Domains/Registrar/Adapter/OpenSRS.php`:
- Around line 140-150: The register() method is currently constructing a
SW_REGISTER payload with an empty contact_set when transfer() calls it with no
contacts, which causes OpenSRS to reject transfers; modify transfer() (the
caller of register()) to accept and forward a complete contacts array (owner,
admin, billing, tech) to register(), and update register() to set the
appropriate OpenSRS transfer flags (e.g., include a populated 'contact_set' in
'attributes' for reg_type='transfer' or set the API's
change_contact/change_contacts flag when using the caller's user data), ensuring
register() uses the provided contacts array when regType === 'transfer' rather
than leaving it empty.
---
Duplicate comments:
In `@src/Domains/Registrar/Adapter/NameCom.php`:
- Around line 118-121: The available() method's error handling only maps
self::ERROR_INVALID_DOMAINS to false, leaving self::ERROR_INVALID_DOMAIN and
self::ERROR_UNSUPPORTED_TLD to bubble up; update the switch in available() (the
switch over $this->matchError($e) in NameCom.php) to treat ERROR_INVALID_DOMAIN
and ERROR_UNSUPPORTED_TLD the same as ERROR_INVALID_DOMAINS (return false) so
unsupported/invalid single-domain errors are handled consistently.
---
Nitpick comments:
In `@src/Domains/Registrar/Adapter/NameCom.php`:
- Around line 396-397: The current error handling in NameCom.php maps
matchError($e) values [self::ERROR_NOT_FOUND, self::ERROR_INVALID_DOMAIN] to
PriceNotFoundException but misses the unsupported-TLD case; update the
switch/case that checks matchError($e) (used by getPricing) to also include
self::ERROR_UNSUPPORTED_TLD (or add a separate case for ERROR_UNSUPPORTED_TLD)
so that when Name.com returns unsupported-TLD the code throws
PriceNotFoundException($message, $code, $e) instead of falling through to
DomainsException.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Domains/Registrar.phpsrc/Domains/Registrar/Adapter.phpsrc/Domains/Registrar/Adapter/Mock.phpsrc/Domains/Registrar/Adapter/NameCom.phpsrc/Domains/Registrar/Adapter/OpenSRS.phpsrc/Domains/Registrar/Price.phptests/Registrar/Base.php
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Domains/Registrar/Adapter/Mock.php (1)
264-267:⚠️ Potential issue | 🟠 MajorCache key is still missing
periodYearsandregType.
getPrice()prices vary by registration type and period, but cache entries are keyed only by domain. This can return incorrect cached values across different calls.Suggested fix
- if ($this->cache) { - $cached = $this->cache->load($domain, $ttl); + $cacheKey = $domain . '_' . $periodYears . '_' . $regType; + if ($this->cache) { + $cached = $this->cache->load($cacheKey, $ttl); if (is_array($cached) && isset($cached['price'])) { return new Price($cached['price'], $cached['premium'] ?? false); } } @@ $result = new Price($price, true); if ($this->cache) { - $this->cache->save($domain, ['price' => $result->price, 'premium' => $result->premium]); + $this->cache->save($cacheKey, ['price' => $result->price, 'premium' => $result->premium]); } @@ $result = new Price($price, false); if ($this->cache) { - $this->cache->save($domain, ['price' => $result->price, 'premium' => $result->premium]); + $this->cache->save($cacheKey, ['price' => $result->price, 'premium' => $result->premium]); }Also applies to: 277-277, 305-305
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Domains/Registrar/Adapter/Mock.php` around lines 264 - 267, The cache key for getPrice() currently uses only the domain, causing collisions across different registration types and periods; update the caching logic in getPrice() (and the related cache->load/cache->save usages around the same function) to include both $periodYears and $regType in the cache key (e.g., incorporate them into the key string or array used for cache->load and cache->save) so cached entries are unique per domain+periodYears+regType, and ensure the cached payload still stores ['price', 'premium'] as before.src/Domains/Registrar/Adapter/NameCom.php (1)
118-123:⚠️ Potential issue | 🟠 Major
available()still rethrows unsupported/invalid single-domain errors.To match the intended behavior, unsupported TLD and invalid single-domain errors should also return
falsehere.Suggested fix
} catch (Exception $e) { switch ($this->matchError($e)) { case self::ERROR_INVALID_DOMAINS: + case self::ERROR_INVALID_DOMAIN: + case self::ERROR_UNSUPPORTED_TLD: return false; default: throw $e; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Domains/Registrar/Adapter/NameCom.php` around lines 118 - 123, In the NameCom adapter's available() error handling block, currently only self::ERROR_INVALID_DOMAINS returns false while other errors are rethrown; add additional cases for the unsupported-TLD and single-invalid-domain error codes (e.g. self::ERROR_UNSUPPORTED_TLD and self::ERROR_INVALID_DOMAIN) so they also return false, by extending the switch on $this->matchError($e) to include those constants instead of falling through to the default throw $e.
🧹 Nitpick comments (1)
src/Domains/Registrar/Adapter/Mock.php (1)
365-365: Transfer parameters are currently unused in the mock implementation.If this is intentional, consider explicitly marking intent (e.g., parameter rename/doc note) so tests don’t imply auth/premium-transfer validation is happening here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Domains/Registrar/Adapter/Mock.php` at line 365, The mock transfer implementation in method transfer(string $domain, string $authCode, ?float $purchasePrice = null) accepts authCode and purchasePrice but never uses them; update the mock to explicitly mark intent by either renaming the parameters to _authCode and _purchasePrice (or adding a "// intentionally unused" comment) and adding a PHPDoc on transfer explaining this is a no-op/mock and does not perform auth/premium checks, or if tests expect validation, implement minimal handling (e.g., validate authCode format or compare to a fixture and handle purchasePrice). Ensure the change is applied in the Mock class's transfer method to avoid misleading tests and silence unused-parameter warnings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Domains/Registrar/Adapter/NameCom.php`:
- Around line 396-400: In getPrice() of NameCom.php the error mapping
incorrectly treats ERROR_INVALID_DOMAIN as a PriceNotFound case; change the
branch logic so that when $this->matchError($e) equals
self::ERROR_INVALID_DOMAIN it throws UnsupportedTldException (same handling as
self::ERROR_UNSUPPORTED_TLD) and remove ERROR_INVALID_DOMAIN from the in_array
that throws PriceNotFoundException; update the switch/case or conditional that
references matchError(), ERROR_INVALID_DOMAIN, ERROR_NOT_FOUND,
UnsupportedTldException, and PriceNotFoundException accordingly.
---
Duplicate comments:
In `@src/Domains/Registrar/Adapter/Mock.php`:
- Around line 264-267: The cache key for getPrice() currently uses only the
domain, causing collisions across different registration types and periods;
update the caching logic in getPrice() (and the related cache->load/cache->save
usages around the same function) to include both $periodYears and $regType in
the cache key (e.g., incorporate them into the key string or array used for
cache->load and cache->save) so cached entries are unique per
domain+periodYears+regType, and ensure the cached payload still stores ['price',
'premium'] as before.
In `@src/Domains/Registrar/Adapter/NameCom.php`:
- Around line 118-123: In the NameCom adapter's available() error handling
block, currently only self::ERROR_INVALID_DOMAINS returns false while other
errors are rethrown; add additional cases for the unsupported-TLD and
single-invalid-domain error codes (e.g. self::ERROR_UNSUPPORTED_TLD and
self::ERROR_INVALID_DOMAIN) so they also return false, by extending the switch
on $this->matchError($e) to include those constants instead of falling through
to the default throw $e.
---
Nitpick comments:
In `@src/Domains/Registrar/Adapter/Mock.php`:
- Line 365: The mock transfer implementation in method transfer(string $domain,
string $authCode, ?float $purchasePrice = null) accepts authCode and
purchasePrice but never uses them; update the mock to explicitly mark intent by
either renaming the parameters to _authCode and _purchasePrice (or adding a "//
intentionally unused" comment) and adding a PHPDoc on transfer explaining this
is a no-op/mock and does not perform auth/premium checks, or if tests expect
validation, implement minimal handling (e.g., validate authCode format or
compare to a fixture and handle purchasePrice). Ensure the change is applied in
the Mock class's transfer method to avoid misleading tests and silence
unused-parameter warnings.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Domains/Registrar/Adapter/Mock.phpsrc/Domains/Registrar/Adapter/NameCom.phptests/Registrar/Base.php
ChiragAgg5k
force-pushed
the
fix/namecom-error-handling
branch
from
a597cd9 to
a764f81
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/Domains/Registrar/Adapter/Mock.php (1)
264-267:⚠️ Potential issue | 🟠 MajorCache key is still too coarse for
getPrice().The cache is keyed only by domain, so values can be incorrectly reused across different
periodYearsandregTyperequests.Proposed fix
- $cached = $this->cache->load($domain, $ttl); + $cacheKey = $domain . '_' . $periodYears . '_' . $regType; + $cached = $this->cache->load($cacheKey, $ttl); if (is_array($cached) && isset($cached['price'])) { return new Price($cached['price'], $cached['premium'] ?? false); } @@ - $this->cache->save($domain, ['price' => $result->price, 'premium' => $result->premium]); + $cacheKey = $domain . '_' . $periodYears . '_' . $regType; + $this->cache->save($cacheKey, ['price' => $result->price, 'premium' => $result->premium]); @@ - $this->cache->save($domain, ['price' => $result->price, 'premium' => $result->premium]); + $cacheKey = $domain . '_' . $periodYears . '_' . $regType; + $this->cache->save($cacheKey, ['price' => $result->price, 'premium' => $result->premium]);Also applies to: 276-277, 304-305
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Domains/Registrar/Adapter/Mock.php` around lines 264 - 267, The cache key used in getPrice() is too coarse (currently just $domain) and ignores request parameters like $periodYears and $regType, causing incorrect cache hits; update the key generation for both cache->load and cache->save calls in the getPrice-related code paths (the blocks that create Price from $cached and that store price data) to include $periodYears and $regType (e.g., append or hash them into the key), and apply the same fix to the similar cache usages referenced around the other occurrences (the blocks at the locations flagged ~276-277 and ~304-305) so cached entries are unique per domain+periodYears+regType.src/Domains/Registrar/Adapter/NameCom.php (1)
120-122:⚠️ Potential issue | 🟠 Major
available()still misses unsupported/invalid single-domain cases.This currently returns
falseonly forERROR_INVALID_DOMAINS. Unsupported TLD and invalid single-domain responses can still throw, which conflicts with the PR behavior goal.Proposed fix
switch ($this->matchError($e)) { case self::ERROR_INVALID_DOMAINS: + case self::ERROR_INVALID_DOMAIN: + case self::ERROR_UNSUPPORTED_TLD: return false; default: throw $e; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Domains/Registrar/Adapter/NameCom.php` around lines 120 - 122, The available() method's exception handling currently only maps self::ERROR_INVALID_DOMAINS to false; extend the switch on $this->matchError($e) in the available() method to also return false for the constants representing unsupported TLDs and invalid single-domain responses (e.g., self::ERROR_UNSUPPORTED_TLD and self::ERROR_INVALID_DOMAIN) so those cases don't throw, and keep the existing behavior (rethrow or default) for other errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Domains/Registrar/Adapter/Mock.php`:
- Around line 264-267: The cache key used in getPrice() is too coarse (currently
just $domain) and ignores request parameters like $periodYears and $regType,
causing incorrect cache hits; update the key generation for both cache->load and
cache->save calls in the getPrice-related code paths (the blocks that create
Price from $cached and that store price data) to include $periodYears and
$regType (e.g., append or hash them into the key), and apply the same fix to the
similar cache usages referenced around the other occurrences (the blocks at the
locations flagged ~276-277 and ~304-305) so cached entries are unique per
domain+periodYears+regType.
In `@src/Domains/Registrar/Adapter/NameCom.php`:
- Around line 120-122: The available() method's exception handling currently
only maps self::ERROR_INVALID_DOMAINS to false; extend the switch on
$this->matchError($e) in the available() method to also return false for the
constants representing unsupported TLDs and invalid single-domain responses
(e.g., self::ERROR_UNSUPPORTED_TLD and self::ERROR_INVALID_DOMAIN) so those
cases don't throw, and keep the existing behavior (rethrow or default) for other
errors.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Domains/Registrar/Adapter/Mock.phpsrc/Domains/Registrar/Adapter/NameCom.phptests/Registrar/Base.php
loks0n
approved these changes
Mar 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
falseinstead of throwing when checking availability for invalid domains (e.g. unsupported TLDs)UnsupportedTldExceptionfor unsupported TLDs inpurchaseandtransfermethodstransferto use&&instead of||for stricter matching (require both error code and message to match).in)Test plan
testAvailablewith.indomain — should returnfalseinstead of throwingtestTransferUnsupportedTld— should throwUnsupportedTldExceptionSummary by CodeRabbit
New Features
Bug Fixes
Chores
Tests