Add Name.com domain registrar adapter #52
Conversation
WalkthroughThis pull request introduces a comprehensive refactoring of the registrar adapter system. A new NameCom domain registrar adapter is added with full API implementation including domain availability checks, purchase/transfer flows, nameserver management, pricing, and transfer status tracking. The registrar adapter base class is restructured with new protected fields for cache and timeouts, setter methods, and updated method signatures where purchase/transfer operations now return order IDs as strings instead of Registration objects. An abstract UpdateDetails class is introduced with adapter-specific implementations (Mock, OpenSRS, NameCom) for domain update operations. A new abstract Base test class provides reusable test implementations for adapter testing. Existing test classes (MockTest, OpenSRSTest) are refactored to extend this Base class, removing duplicated test logic. The Registrar facade is updated with expanded constructor parameters and new public methods (updateNameservers, cancelPurchase, checkTransferStatus). The Registration class is removed and Renewal is simplified. GitHub Actions secrets for NameCom credentials are configured, and composer autoload-dev is added for tests. Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
158-159: Incorrecttransfer()method signature in example.Line 158 shows
$reg->transfer($domain, [$contact])which is missing the required$authCodeparameter. Based on theRegistrar::transfer()signature, the second parameter should be the auth code.📝 Proposed fix
$renew = $reg->renew($domain, 1); -$transfer = $reg->transfer($domain, [$contact]); $transfer = $reg->transfer($domain, 'authcode', [$contact]);
🤖 Fix all issues with AI agents
In @.github/workflows/test.yml:
- Around line 31-32: Tests fail when repository secrets are not available for
PRs from forks; update the test suite to detect missing NameCom credentials
(environment variables NAMECOM_USERNAME and NAMECOM_TOKEN) and skip tests
instead of asserting. In the NameComTest class (e.g., in setUp or a helper like
ensureCredentialsAvailable used by tests such as
testCreateDomain/testUpdateDomain), check getenv('NAMECOM_USERNAME') and
getenv('NAMECOM_TOKEN') (or $_ENV/$_SERVER equivalents) and call
$this->markTestSkipped('NameCom credentials not available') when either is empty
so CI forks gracefully skip these tests.
In @src/Domains/Registrar/Adapter/NameCom.php:
- Around line 586-594: The request lacks curl timeouts so calls can hang; when
configuring the curl handle ($ch) in NameCom.php (the block that calls curl_init
and sets options like CURLOPT_HTTPHEADER, CURLOPT_USERPWD and
CURLOPT_CUSTOMREQUEST), add timeout options via curl_setopt — at minimum set
CURLOPT_CONNECTTIMEOUT (e.g., 5 seconds) and CURLOPT_TIMEOUT (e.g., 10 seconds)
so connection and total request time are bounded; optionally add millisecond
variants (CURLOPT_CONNECTTIMEOUT_MS / CURLOPT_TIMEOUT_MS) if you need sub-second
precision, and ensure these options are set before executing curl_exec.
- Around line 623-648: The sanitizeContacts function drops contacts when
$contacts is a numerically indexed array because the loop checks string keys
against $types; fix sanitizeContacts by detecting numeric keys (e.g.,
array_keys($contacts) are sequential integers) and in that case iterate the
predefined $types and assign contacts by index (use $contacts[$i] if set)
mapping the first contact to registrant/admin/tech/billing as appropriate;
retain the existing branch for associative keys and keep special-case mapping of
'owner' to 'registrant', calling $this->formatContact(...) for every assigned
contact.
- Around line 474-481: The renewal parsing is using wrong response keys so
renewals are marked failed; update the parsing logic that constructs the Renewal
to read $orderId from $result['order'] (not 'order_id') and read the expiry from
the nested domain object ($result['domain']['expireDate']) while safely checking
existence before creating the DateTime (e.g.,
isset($result['domain']['expireDate']) or null fallback) so $expiresAt is set
correctly and $orderId reflects the API's 'order' field.
- Around line 656-672: The formatContact method is missing the organization ->
company mapping for Name.com; update formatContact (in NameCom.php) to include
'companyName' mapped from the Contact object's 'org' field by adding
"companyName" => $data['org'] ?? '' into the returned array so the Name.com v4
API receives the organization value.
In @tests/Registrar/NameComTest.php:
- Around line 25-54: The setUp() test currently uses $this->assertNotEmpty on
NAMECOM_USERNAME and NAMECOM_TOKEN which fails tests when env vars are absent;
change this to detect empty getenv('NAMECOM_USERNAME') or
getenv('NAMECOM_TOKEN') in setUp() and call $this->markTestSkipped('NAMECOM
credentials not set') so tests are skipped instead of failing; update the
setUp() method in NameComTest to perform the skip check before initializing
$this->client and $this->clientWithCache and remove or replace the
assertNotEmpty assertions.
- Around line 237-243: The test testGetPriceWithInvalidDomain asserts the wrong
exception/message for getPrice; inspect the getPrice method behavior and fix the
test to match it: if getPrice throws PriceNotFoundException use
expectException(PriceNotFoundException::class) and change expectExceptionMessage
to "Price not found for domain:"; if an invalid TLD path throws DomainsException
(caught at the logic around lines 370-372), update the test to expect
DomainsException and the appropriate message instead; ensure you reference the
same exception class (PriceNotFoundException or DomainsException) and exact
message string used by getPrice.
🧹 Nitpick comments (4)
src/Domains/Registrar/Adapter/NameCom.php (4)
520-520: Unused parameters$checkStatusand$getRequestAddress.As flagged by static analysis, these parameters are not used in the method body. If Name.com API doesn't support these options, consider adding a docblock comment explaining the limitation, or prefix with underscore to indicate intentional non-use.
♻️ Proposed fix
- public function checkTransferStatus(string $domain, bool $checkStatus = true, bool $getRequestAddress = false): TransferStatus + /** + * Check transfer status for a domain + * + * @param string $domain The domain name + * @param bool $_checkStatus Flag to check status (not used by Name.com API) + * @param bool $_getRequestAddress Flag to get request address (not used by Name.com API) + * @return TransferStatus Transfer status information + */ + public function checkTransferStatus(string $domain, bool $_checkStatus = true, bool $_getRequestAddress = false): TransferStatus
337-361: Cache key construction is duplicated.The cache key pattern is defined twice (Lines 338 and 360). Consider extracting to a local variable to avoid inconsistencies.
♻️ Proposed fix
public function getPrice(string $domain, int $periodYears = 1, string $regType = self::REG_TYPE_NEW, int $ttl = 3600): float { + $cacheKey = $domain . '_' . $regType . '_' . $periodYears; + if ($this->cache) { - $cacheKey = $domain . '_' . $regType . '_' . $periodYears; $cached = $this->cache->load($cacheKey, $ttl); if ($cached !== null && is_array($cached)) { return $cached['price']; } } try { // ... existing code ... if ($this->cache) { - $cacheKey = $domain . '_' . $regType . '_' . $periodYears; $this->cache->save($cacheKey, ['price' => $price]); }
321-323: Silent exception handling insuggest()andtlds().Both methods silently return empty arrays on failure. While this may be acceptable for non-critical operations, consider logging the exception for debugging purposes or documenting this behavior in the method docblocks.
Also applies to: 391-393
428-456: Non-atomic updates may leave domain in partial state.When both contacts and autorenew are updated, two separate API calls are made. If the second fails, the first change persists. Consider documenting this limitation or returning more detailed error information.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/test.ymlREADME.mdsrc/Domains/Registrar/Adapter/NameCom.phptests/Registrar/NameComTest.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/Domains/Registrar/Adapter/NameCom.php (12)
src/Domains/Registrar.php (1)
Registrar(11-163)src/Domains/Exception.php (1)
Exception(5-7)src/Domains/Registrar/Contact.php (2)
Contact(5-44)toArray(24-43)src/Domains/Registrar/Exception/DomainTakenException.php (1)
DomainTakenException(7-9)src/Domains/Registrar/Exception/DomainNotTransferableException.php (1)
DomainNotTransferableException(7-9)src/Domains/Registrar/Exception/InvalidContactException.php (1)
InvalidContactException(7-9)src/Domains/Registrar/Exception/AuthException.php (1)
AuthException(7-9)src/Domains/Registrar/Exception/PriceNotFoundException.php (1)
PriceNotFoundException(7-9)src/Domains/Cache.php (3)
Cache(7-35)load(21-24)save(26-29)src/Domains/Registrar/Registration.php (1)
Registration(5-17)src/Domains/Registrar/Renewal.php (1)
Renewal(7-15)src/Domains/Registrar/TransferStatus.php (1)
TransferStatus(19-27)
🪛 GitHub Actions: Tests
tests/Registrar/NameComTest.php
[error] 32-32: NAMECOM_USERNAME environment variable must be set
[error] 32-32: NAMECOM_USERNAME environment variable must be set
[error] 32-32: NAMECOM_USERNAME environment variable must be set
[error] 32-32: NAMECOM_USERNAME environment variable must be set
[error] 32-32: NAMECOM_USERNAME environment variable must be set
[error] 32-32: NAMECOM_USERNAME environment variable must be set
[error] 32-32: NAMECOM_USERNAME environment variable must be set
[error] 32-32: NAMECOM_USERNAME environment variable must be set
[error] 32-32: NAMECOM_USERNAME environment variable must be set
[error] 32-32: NAMECOM_USERNAME environment variable must be set
[error] 32-32: NAMECOM_USERNAME environment variable must be set
[error] 32-32: NAMECOM_USERNAME environment variable must be set
[error] 32-32: NAMECOM_USERNAME environment variable must be set
[error] 32-32: NAMECOM_USERNAME environment variable must be set
[error] 32-32: NAMECOM_USERNAME environment variable must be set
[error] 32-32: NAMECOM_USERNAME environment variable must be set
[error] 32-32: NAMECOM_USERNAME environment variable must be set
[error] 32-32: NAMECOM_USERNAME environment variable must be set
[error] 32-32: NAMECOM_USERNAME environment variable must be set
[error] 32-32: NAMECOM_USERNAME environment variable must be set
[error] 32-32: NAMECOM_USERNAME environment variable must be set
[error] 32-32: NAMECOM_USERNAME environment variable must be set
[error] 32-32: NAMECOM_USERNAME environment variable must be set
[error] 32-32: NAMECOM_USERNAME environment variable must be set
🪛 PHPMD (2.15.0)
src/Domains/Registrar/Adapter/NameCom.php
520-520: Avoid unused parameters such as '$checkStatus'. (undefined)
(UnusedFormalParameter)
520-520: Avoid unused parameters such as '$getRequestAddress'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (8)
src/Domains/Registrar/Adapter/NameCom.php (4)
47-66: LGTM on constructor and endpoint normalization.The HTTPS enforcement and header initialization are appropriate for a production API adapter.
402-418: LGTM ongetDomain()implementation.The method correctly maps API response fields to the Domain object. Letting transport exceptions propagate is consistent with other adapters.
562-572: LGTM on transfer status mapping.The conservative default of
NotTransferrablefor unknown statuses is appropriate - safer than assuming transferability.
133-176: LGTM on purchase implementation.Exception mapping based on message content is reasonable given that many REST APIs don't provide structured error codes.
README.md (1)
107-126: LGTM on NameCom adapter documentation.Clear and consistent with the OpenSRS example format. The endpoint configuration examples are helpful.
tests/Registrar/NameComTest.php (3)
77-85: Real domain purchase in test could incur costs.This test makes actual purchase API calls. Ensure that the CI secrets (
NAMECOM_USERNAME,NAMECOM_TOKEN) are configured for Name.com's sandbox environment (https://api.dev.name.com) rather than production, to avoid unintended domain purchases and charges.
408-444: LGTM on test helper methods.The
purchaseContact()andgenerateRandomString()helpers are well-structured, and usingrandom_int()provides cryptographically secure randomness for test domain generation.
35-35: Test depends on pre-existing domain in account.
testDomainInfo()uses'example-test-domain.com'which must exist in the Name.com account for the test to pass. Consider documenting this requirement or using a domain purchased in an earlier test (with appropriate test ordering/dependencies).Also applies to: 140-149
There was a problem hiding this comment.
Pull request overview
This pull request adds a complete Name.com domain registrar adapter to the Utopia Domains library, expanding the library's registrar support beyond OpenSRS. The adapter implements all required abstract methods from the Adapter interface and uses Name.com's REST API v4 with JSON and HTTP Basic Authentication.
Changes:
- Added new NameCom adapter class implementing all 13 required methods (getName, available, purchase, transfer, renew, getDomain, updateDomain, updateNameservers, suggest, getPrice, tlds, getAuthCode, checkTransferStatus, cancelPurchase)
- Added comprehensive test suite with 20+ test methods covering success cases, exception handling, caching, and edge cases
- Updated GitHub Actions workflow to include Name.com API credentials for CI/CD testing
- Updated README with usage examples for both OpenSRS and NameCom adapters
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
src/Domains/Registrar/Adapter/NameCom.php |
New adapter implementing Name.com API v4 with REST/JSON, HTTP Basic Auth, comprehensive error handling, contact formatting, and caching support |
tests/Registrar/NameComTest.php |
Comprehensive test suite covering all adapter methods, exception scenarios, caching behavior, and endpoint configuration |
.github/workflows/test.yml |
Added NAMECOM_USERNAME and NAMECOM_TOKEN environment variables for CI/CD test execution |
README.md |
Added documentation for NameCom adapter usage with code examples alongside existing OpenSRS documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ChiragAgg5k
force-pushed
the
feat-add-namecom-adapter
branch
3 times, most recently
from
f239249 to
bc398da
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/Domains/Registrar/Adapter/NameCom.php:
- Around line 637-662: sanitizeContacts currently ignores numerically-indexed
contact arrays because it only checks string keys against $types; update
sanitizeContacts to detect numeric-indexed input (array_keys($contacts) ===
range(0, count($contacts)-1)) and, in that case, map contacts by position to the
Name.com types (e.g., index 0→'registrant',1→'admin',2→'tech',3→'billing'),
falling back to reusing the first contact for missing positions; keep the
existing associative handling (including 'owner' → 'registrant') and continue to
call $this->formatContact(...) for each assigned type.
In @tests/Registrar/NameComTest.php:
- Around line 25-55: The test setup currently uses assertNotEmpty for $username
and $token which causes hard failures; change setUp() so it checks
getenv('NAMECOM_USERNAME') and getenv('NAMECOM_TOKEN') and if either is empty
calls $this->markTestSkipped('NAMECOM credentials not set') instead of
assertNotEmpty, then proceed to set $this->domain and instantiate $this->client
and $this->clientWithCache (NameCom constructor) only when credentials exist.
🧹 Nitpick comments (5)
src/Domains/Registrar/Adapter/NameCom.php (4)
169-177: Consider tightening error message matching to avoid false positives.The check
str_contains($e->getMessage(), 'contact')on line 172 is overly broad and could match unintended messages like "Please contact support" or "contact us for help", incorrectly throwingInvalidContactException.♻️ Suggested refinement
- if (str_contains($e->getMessage(), 'Invalid contact') || str_contains($e->getMessage(), 'contact')) { + if (str_contains($e->getMessage(), 'Invalid contact') || str_contains($e->getMessage(), 'contact data') || str_contains($e->getMessage(), 'contact information')) { throw new InvalidContactException($message, self::RESPONSE_CODE_INVALID_CONTACT, $e); }
621-628: Handle JSON decode failures explicitly.If the API returns malformed JSON,
json_decodereturnsnulland the method silently returns an empty array. This could mask API errors or connectivity issues.♻️ Proposed fix
$response = json_decode($result, true); + if ($response === null && $result !== 'null' && $result !== '') { + throw new Exception('Failed to parse response from Name.com: Invalid JSON'); + } + if ($httpCode >= 400) { $message = $response['message'] ?? $response['details'] ?? 'Unknown error'; throw new Exception($message, $httpCode); } return $response ?? [];
530-534: Unused parameters$checkStatusand$getRequestAddressshould be documented or removed.These parameters are defined but never used. If they're required for interface compliance, add a docblock note explaining they're not applicable for the Name.com API. Otherwise, consider removing them if the adapter interface allows.
♻️ Suggested documentation update
/** * Check transfer status for a domain * * @param string $domain The domain name - * @param bool $checkStatus Flag to check status - * @param bool $getRequestAddress Flag to get request address + * @param bool $checkStatus Flag to check status (not used by Name.com API) + * @param bool $getRequestAddress Flag to get request address (not used by Name.com API) * @return TransferStatus Transfer status information */
473-497: Consider surfacing renewal errors for debugging.When renewal fails, the method returns a
Renewalwithsuccessful: falsebut discards the exception details. This makes debugging difficult in production.♻️ Suggested improvement
Consider logging the error or including the error message in a new
errorproperty on the Renewal object if the model supports it, or throw the exception for truly unexpected errors while only returning unsuccessful for expected failures.tests/Registrar/NameComTest.php (1)
78-86: Consider isolating state-mutating tests.This test actually purchases a random domain on the sandbox API. While this is appropriate for integration testing, consider adding a
@group integrationannotation or similar to allow running quick unit tests separately from slower, state-mutating integration tests.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/test.ymlREADME.mdsrc/Domains/Registrar/Adapter/NameCom.phptests/Registrar/NameComTest.php
✅ Files skipped from review due to trivial changes (1)
- README.md
🧰 Additional context used
🧬 Code graph analysis (1)
src/Domains/Registrar/Adapter/NameCom.php (12)
src/Domains/Registrar.php (1)
Registrar(11-163)src/Domains/Exception.php (1)
Exception(5-7)src/Domains/Registrar/Contact.php (2)
Contact(5-44)toArray(24-43)src/Domains/Registrar/Exception/DomainTakenException.php (1)
DomainTakenException(7-9)src/Domains/Registrar/Exception/DomainNotTransferableException.php (1)
DomainNotTransferableException(7-9)src/Domains/Registrar/Exception/InvalidContactException.php (1)
InvalidContactException(7-9)src/Domains/Registrar/Exception/AuthException.php (1)
AuthException(7-9)src/Domains/Registrar/Exception/PriceNotFoundException.php (1)
PriceNotFoundException(7-9)src/Domains/Cache.php (3)
Cache(7-35)load(21-24)save(26-29)src/Domains/Registrar/Registration.php (1)
Registration(5-17)src/Domains/Registrar/Renewal.php (1)
Renewal(7-15)src/Domains/Registrar/TransferStatus.php (1)
TransferStatus(19-27)
🪛 GitHub Actions: Tests
tests/Registrar/NameComTest.php
[error] 32-32: NAMECOM_USERNAME environment variable must be set.
[error] 32-32: NAMECOM_USERNAME environment variable must be set.
[error] 32-32: NAMECOM_USERNAME environment variable must be set.
[error] 32-32: NAMECOM_USERNAME environment variable must be set.
[error] 32-32: NAMECOM_USERNAME environment variable must be set.
[error] 32-32: NAMECOM_USERNAME environment variable must be set.
[error] 32-32: NAMECOM_USERNAME environment variable must be set.
[error] 32-32: NAMECOM_USERNAME environment variable must be set.
[error] 32-32: NAMECOM_USERNAME environment variable must be set.
[error] 32-32: NAMECOM_USERNAME environment variable must be set.
[error] 32-32: NAMECOM_USERNAME environment variable must be set.
[error] 32-32: NAMECOM_USERNAME environment variable must be set.
[error] 32-32: NAMECOM_USERNAME environment variable must be set.
[error] 32-32: NAMECOM_USERNAME environment variable must be set.
[error] 32-32: NAMECOM_USERNAME environment variable must be set.
[error] 32-32: NAMECOM_USERNAME environment variable must be set.
[error] 32-32: NAMECOM_USERNAME environment variable must be set.
[error] 32-32: NAMECOM_USERNAME environment variable must be set.
[error] 32-32: NAMECOM_USERNAME environment variable must be set.
[error] 32-32: NAMECOM_USERNAME environment variable must be set.
[error] 32-32: NAMECOM_USERNAME environment variable must be set.
[error] 32-32: NAMECOM_USERNAME environment variable must be set.
[error] 32-32: NAMECOM_USERNAME environment variable must be set.
[error] 32-32: NAMECOM_USERNAME environment variable must be set.
[error] 32-32: NAMECOM_USERNAME environment variable must be set.
🪛 PHPMD (2.15.0)
src/Domains/Registrar/Adapter/NameCom.php
530-530: Avoid unused parameters such as '$checkStatus'. (undefined)
(UnusedFormalParameter)
530-530: Avoid unused parameters such as '$getRequestAddress'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (6)
src/Domains/Registrar/Adapter/NameCom.php (3)
1-70: LGTM on adapter setup and constructor.The constructor properly initializes credentials, normalizes the endpoint URL to HTTPS, and sets reasonable defaults for timeouts. The endpoint normalization logic correctly handles both
http://and bare hostnames.
77-100: LGTM on core domain availability check.The
available()method correctly queries the Name.com API and safely handles the response structure with proper null checks.
381-426: LGTM on TLD listing and domain info retrieval.Both
tlds()andgetDomain()methods are well-implemented with proper response parsing and type handling..github/workflows/test.yml (1)
31-32: Ensure GitHub secrets are configured to fix pipeline failures.The workflow correctly exports the environment variables, but the pipeline is failing with "NAMECOM_USERNAME environment variable must be set" because the secrets
NAMECOM_USERNAMEandNAMECOM_TOKENhaven't been added to the repository's GitHub Actions secrets.Please add these secrets in the repository settings under Settings → Secrets and variables → Actions before merging.
tests/Registrar/NameComTest.php (2)
379-415: LGTM on helper methods.The
purchaseContact()andgenerateRandomString()helpers are well-implemented and follow secure random generation practices usingrandom_int().
57-169: Good test coverage for core adapter functionality.The tests comprehensively cover the adapter's API surface including success paths, error handling with specific exception types, and edge cases like taken domains and invalid credentials.
ChiragAgg5k
force-pushed
the
feat-add-namecom-adapter
branch
2 times, most recently
from
42b804e to
8b0463b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/Domains/Registrar/Adapter/NameCom.php:
- Around line 343-349: The cached array read via $this->cache->load($cacheKey,
$ttl) may lack a 'price' key causing an undefined index; update the block in the
method that builds $cacheKey and reads $cached to defensively verify that
$cached is an array and array_key_exists('price', $cached) (or
isset($cached['price'])) before returning $cached['price'], and fall through to
the normal price-fetch logic if the key is missing or invalid.
- Around line 602-604: When encoding request body for POST/PUT/PATCH, don't pass
json_encode($data) directly to CURLOPT_POSTFIELDS because json_encode can return
false; instead first call $body = json_encode($data) and check ($body !==
false), and if encoding failed use json_last_error_msg() to throw/log a clear
exception (or attempt to sanitize/utf8-encode the payload) so you never send the
literal "false" string; update the block that sets CURLOPT_POSTFIELDS (the code
using $data, json_encode and CURLOPT_POSTFIELDS in NameCom.php) to perform this
check and handle the error path before calling curl_setopt.
🧹 Nitpick comments (5)
src/Domains/Registrar/Adapter/NameCom.php (5)
526-527: Unused parameters$checkStatusand$getRequestAddress.These parameters are declared but never used. If required by the
Adapterinterface, consider adding a docblock note that they're not supported by Name.com's API. If not required, remove them.♻️ Suggested documentation
/** * Check transfer status for a domain * * @param string $domain The domain name - * @param bool $checkStatus Flag to check status - * @param bool $getRequestAddress Flag to get request address + * @param bool $checkStatus Flag to check status (not used by Name.com API) + * @param bool $getRequestAddress Flag to get request address (not used by Name.com API) * @return TransferStatus Transfer status information */ + #[\SuppressWarnings(PHPMD.UnusedFormalParameter)] public function checkTransferStatus(string $domain, bool $checkStatus = true, bool $getRequestAddress = false): TransferStatus
666-682:address3field from Contact is not mapped.The
Contactclass includes anaddress3property, but it's not included in the formatted output. If Name.com's API doesn't support a third address line, consider concatenating it withaddress2to preserve the data.♻️ Proposed fix to preserve address3
return [ 'firstName' => $data['firstname'] ?? '', 'lastName' => $data['lastname'] ?? '', 'email' => $data['email'] ?? '', 'phone' => $data['phone'] ?? '', 'address1' => $data['address1'] ?? '', - 'address2' => $data['address2'] ?? '', + 'address2' => trim(($data['address2'] ?? '') . ' ' . ($data['address3'] ?? '')), 'city' => $data['city'] ?? '', 'state' => $data['state'] ?? '', 'zip' => $data['postalcode'] ?? '', 'country' => $data['country'] ?? '', ];
568-578: Default to a more appropriate status for unknown values.Mapping unknown transfer statuses to
NotTransferrablemay be overly restrictive. Consider using a more neutral default or logging unexpected values to catch API changes.♻️ Alternative approach
return match (strtolower($status)) { 'pending' => TransferStatusEnum::PendingRegistry, 'approved', 'complete', 'completed' => TransferStatusEnum::Completed, 'cancelled', 'rejected' => TransferStatusEnum::Cancelled, 'pending_owner' => TransferStatusEnum::PendingOwner, 'pending_admin' => TransferStatusEnum::PendingAdmin, - default => TransferStatusEnum::NotTransferrable, + default => TransferStatusEnum::PendingRegistry, // Unknown status - treat as pending };
390-399: Document empty return in method docblock.The method returns an empty array intentionally, but the docblock says "Get list of available TLDs" which may mislead callers. Consider updating the docblock to clarify this limitation.
♻️ Suggested documentation improvement
/** - * Get list of available TLDs + * Get list of available TLDs. + * + * Note: Name.com supports too many TLDs to return efficiently, + * so this method returns an empty array. Use the Name.com web + * interface or API directly for TLD listings. * * @return array List of TLD strings */
511-512: Redundant exception re-throw.Catching
DomainsExceptiononly to re-throw it is unnecessary since the genericExceptioncatch on line 513 wouldn't match it anyway (due to the order of catches).♻️ Simplified exception handling
- } catch (DomainsException $e) { - throw $e; } catch (Exception $e) { + if ($e instanceof DomainsException) { + throw $e; + } throw new DomainsException('Failed to get auth code: ' . $e->getMessage(), $e->getCode(), $e); }Or simply remove lines 511-512 if
DomainsExceptionextendsExceptionsince it will be caught by the generic handler and re-wrapped (which may be intentional for message consistency).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/test.ymlREADME.mdsrc/Domains/Registrar/Adapter/NameCom.phptests/Registrar/NameComTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- tests/Registrar/NameComTest.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/Domains/Registrar/Adapter/NameCom.php (10)
src/Domains/Registrar/Contact.php (2)
Contact(5-44)toArray(24-43)src/Domains/Registrar/Exception/DomainTakenException.php (1)
DomainTakenException(7-9)src/Domains/Registrar/Exception/DomainNotTransferableException.php (1)
DomainNotTransferableException(7-9)src/Domains/Registrar/Exception/InvalidContactException.php (1)
InvalidContactException(7-9)src/Domains/Registrar/Exception/AuthException.php (1)
AuthException(7-9)src/Domains/Registrar/Exception/PriceNotFoundException.php (1)
PriceNotFoundException(7-9)src/Domains/Cache.php (3)
Cache(7-35)load(21-24)save(26-29)src/Domains/Registrar/Registration.php (1)
Registration(5-17)src/Domains/Registrar/Renewal.php (1)
Renewal(7-15)src/Domains/Registrar/TransferStatus.php (1)
TransferStatus(19-27)
🪛 PHPMD (2.15.0)
src/Domains/Registrar/Adapter/NameCom.php
526-526: Avoid unused parameters such as '$checkStatus'. (undefined)
(UnusedFormalParameter)
526-526: Avoid unused parameters such as '$getRequestAddress'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (4)
.github/workflows/test.yml (1)
31-32: LGTM!The environment variable exports follow the same pattern as the existing OpenSRS credentials. Secrets are properly sourced from the repository secrets context.
Ensure
NAMECOM_USERNAMEandNAMECOM_TOKENsecrets are configured in the repository settings for the CI tests to authenticate successfully.src/Domains/Registrar/Adapter/NameCom.php (3)
88-100: LGTM on core domain operations.The
available(),updateNameservers(), andgetDomain()methods are well-implemented with appropriate error handling and correct API endpoint usage.Also applies to: 109-126, 407-423
67-69: No action needed. The$headersproperty is properly declared in the parentUtopia\Domains\Adapterclass (line 16 with type annotation/** @var array<mixed> */), so there is no dynamic property creation issue or PHP 8.2+ deprecation risk. The assignment in NameCom's constructor is valid and correctly inherits from the parent class.Likely an incorrect or invalid review comment.
478-483: Therenew()method correctly uses$result['order']because the Name.com API v4 RenewDomain endpoint returns an"order"field, not"order_id". Thepurchase()andtransfer()methods useorder_idbecause their respective endpoints return that field. This is not an inconsistency but proper alignment with each endpoint's response format.Likely an incorrect or invalid review comment.
ChiragAgg5k
force-pushed
the
feat-add-namecom-adapter
branch
2 times, most recently
from
de8d2d4 to
8c55ae0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/Domains/Registrar/Adapter/NameCom.php:
- Around line 369-372: The catch for PriceNotFoundException is prepending
"Failed to get price for domain:" to an exception whose message may already
contain that text, causing double-prefixing; fix by not adding the prefix—either
rethrow the original exception (throw $e;) or rethrow a new
PriceNotFoundException using only $e->getMessage() (e.g., new
PriceNotFoundException($e->getMessage(), $e->getCode(), $e)) so the message is
not duplicated; update the catch block around PriceNotFoundException
accordingly.
- Around line 661-677: The formatContact function is missing the organization
mapping required by Name.com; update formatContact (which uses
Contact::toArray()) to include a 'companyName' entry set from the contact's
'org' value (e.g., add a 'companyName' => $data['org'] ?? '' mapping alongside
the other fields) so the Name.com API receives the organization via companyName.
🧹 Nitpick comments (5)
tests/Registrar/NameComTest.php (1)
249-264: Cache test doesn't verify actual caching behavior.The
clientWithCacheusesNoneAdapterwhich doesn't persist data. Both calls togetPrice()will hit the API, and the test passes because the API returns consistent prices—not because caching works. Consider using a memory-backed cache adapter (e.g.,Memoryadapter) to verify that the second call returns cached data without an API request.src/Domains/Registrar/Adapter/NameCom.php (4)
160-175: String-based error detection is fragile but acceptable.The error classification via
str_containson lowercased error messages (lines 165-173) could break if the API changes message wording. Consider documenting this as a known limitation. If Name.com provides structured error codes in responses, those would be more reliable.
322-324: Silent exception swallowing hides API errors.Returning an empty array on exception makes it impossible for callers to distinguish between "no suggestions found" and "API error/authentication failure." Consider at least logging the exception, or allowing certain exceptions (like
AuthException) to propagate.♻️ Suggested improvement
} catch (Exception $e) { - return []; + // Log the error for debugging purposes + // Consider propagating auth exceptions to callers + error_log('Name.com suggest API error: ' . $e->getMessage()); + return []; }
452-454: Silent failure loses error context.Returning
falseon exception doesn't indicate what went wrong (network error, auth failure, domain not found, etc.). Consider logging the error or throwing specific exceptions for recoverable vs. non-recoverable failures.
521-521: Unused parameters$checkStatusand$getRequestAddress.Per static analysis, these parameters are declared but never used. If they're required for interface compatibility with other adapters, add a comment explaining this. Otherwise, consider whether they should influence the API call behavior.
♻️ Suggested fix - document unused parameters
/** * Check transfer status for a domain * * @param string $domain The domain name - * @param bool $checkStatus Flag to check status - * @param bool $getRequestAddress Flag to get request address + * @param bool $checkStatus Flag to check status (unused - Name.com API always returns status) + * @param bool $getRequestAddress Flag to get request address (unused - not supported by Name.com API) * @return TransferStatus Transfer status information */ + // phpcs:ignore Generic.CodeAnalysis.UnusedFunctionParameter public function checkTransferStatus(string $domain, bool $checkStatus = true, bool $getRequestAddress = false): TransferStatus
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/test.ymlREADME.mdsrc/Domains/Registrar/Adapter/NameCom.phptests/Registrar/NameComTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/test.yml
- README.md
🧰 Additional context used
🧬 Code graph analysis (1)
src/Domains/Registrar/Adapter/NameCom.php (11)
src/Domains/Exception.php (1)
Exception(5-7)src/Domains/Registrar/Contact.php (2)
Contact(5-44)toArray(24-43)src/Domains/Registrar/Exception/DomainTakenException.php (1)
DomainTakenException(7-9)src/Domains/Registrar/Exception/DomainNotTransferableException.php (1)
DomainNotTransferableException(7-9)src/Domains/Registrar/Exception/InvalidContactException.php (1)
InvalidContactException(7-9)src/Domains/Registrar/Exception/AuthException.php (1)
AuthException(7-9)src/Domains/Registrar/Exception/PriceNotFoundException.php (1)
PriceNotFoundException(7-9)src/Domains/Cache.php (3)
Cache(7-35)load(21-24)save(26-29)src/Domains/Registrar/Registration.php (1)
Registration(5-17)src/Domains/Registrar/Renewal.php (1)
Renewal(7-15)src/Domains/Registrar/TransferStatus.php (1)
TransferStatus(19-27)
🪛 PHPMD (2.15.0)
src/Domains/Registrar/Adapter/NameCom.php
521-521: Avoid unused parameters such as '$checkStatus'. (undefined)
(UnusedFormalParameter)
521-521: Avoid unused parameters such as '$getRequestAddress'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (7)
tests/Registrar/NameComTest.php (4)
25-55: LGTM!Setup correctly initializes both clients with sandbox endpoint and validates required credentials.
57-171: LGTM!Test methods cover basic functionality, exception handling, and edge cases appropriately. The use of
generateRandomString()for domain names ensures test isolation.
315-364: LGTM!Transfer and auth code tests appropriately handle the expected failures when testing with random/unowned domains. The broad exception handling is reasonable for integration tests where multiple failure modes are acceptable.
405-441: LGTM!Helper methods are well-structured. The
purchaseContactmethod correctly creates the contact structure expected by the adapter, andgenerateRandomStringuses cryptographically securerandom_int().src/Domains/Registrar/Adapter/NameCom.php (3)
49-70: LGTM!Constructor is well-structured with sensible defaults. The HTTPS enforcement (lines 61-65) is a good security practice for API communications.
88-95: LGTM!The availability check correctly uses the Name.com checkAvailability endpoint and safely defaults to
falseif the response structure is unexpected.
583-620: LGTM!The HTTP client implementation follows security best practices with SSL verification, configurable timeouts, and proper error handling. HTTP Basic Authentication is correctly implemented using
CURLOPT_USERPWD.
ChiragAgg5k
force-pushed
the
feat-add-namecom-adapter
branch
3 times, most recently
from
d2ad528 to
6978421
Compare
ChiragAgg5k
force-pushed
the
feat-add-namecom-adapter
branch
from
6978421 to
e185c76
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Implement NameCom adapter with full API v4 REST support - Add comprehensive test suite for NameCom adapter - Update GitHub Actions workflow to include NameCom credentials - Update README with documentation for both OpenSRS and NameCom adapters Features: - Domain availability checking - Domain registration and purchase - Domain transfers with auth codes - Domain renewal and management - Nameserver updates - Domain suggestions and search - Price lookups with caching support - Transfer status checking - Full exception handling The adapter supports both production (api.name.com) and sandbox (api.dev.name.com) endpoints.
ChiragAgg5k
force-pushed
the
feat-add-namecom-adapter
branch
from
e185c76 to
f46f669
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @tests/Registrar/BaseRegistrarTest.php:
- Around line 300-313: The test calls $this->getAdapter()->updateNameservers()
but getAdapter() returns a Registrar instance which lacks updateNameservers(),
so either make Registrar expose that method or change the test to call the real
adapter; add a public method updateNameservers(array $nameservers) (or with the
same signature used in test) to the Registrar class that delegates to the
underlying adapter's updateNameservers(...) (or adjust getAdapter() so it
returns the adapter implementation instead of Registrar), ensuring the method
name and signature match the test's usage (refer to testUpdateNameservers,
getAdapter(), updateNameservers()) so the call succeeds.
- Around line 198-206: The Registrar facade is missing delegation for
cancelPurchase(): add an abstract cancelPurchase(): bool (or appropriate
signature) to the Adapter base class so concrete adapters (NameCom, OpenSRS)
formally implement it, then update the Registrar facade to expose a
cancelPurchase() method that delegates to $this->getAdapter()->cancelPurchase(),
mirroring how purchase(), renew(), and transfer() are forwarded.
In @tests/Registrar/MockTest.php:
- Around line 33-41: The test methods getAdapter and getAdapterWithCache declare
a Registrar return type but actually return the Mock adapter, causing
TypeErrors; update their return type declarations to the concrete Mock type used
in this test (the Mock adapter class name referenced by
$this->adapter/$this->adapterWithCache) or change the returned objects to
implement Registrar—most straightforward is to change the methods' return type
from Registrar to the Mock class name (e.g., Mock) to match the actual returned
instances.
In @tests/Registrar/NameComTest.php:
- Around line 48-56: getAdapter() and getAdapterWithCache() currently return a
NameCom adapter instance while their declared return type is Registrar; wrap the
adapter in a Registrar before returning to match the signature. Replace
returning $this->client and $this->clientWithCache with returning a new
Registrar constructed with those adapter instances (e.g., new
Registrar($this->client) and new Registrar($this->clientWithCache)) so the
methods return Registrar objects consistent with the base class and other tests.
In @tests/Registrar/OpenSRSTest.php:
- Around line 51-59: The test methods getAdapter() and getAdapterWithCache()
declare a Registrar return type but actually return an OpenSRS adapter; change
their declared return type from Registrar to OpenSRS (or a common interface that
OpenSRS implements) so the signatures match their returned values, and add or
adjust the use/import for OpenSRS if needed so the class name resolves
correctly.
🧹 Nitpick comments (5)
tests/Registrar/NameComTest.php (2)
118-124: Consider using the loop variable or suppressing the warning.The
$domainvariable in the foreach loop is unused. If you only need the$datavalues, you can use$_as the key placeholder to signal intent.♻️ Proposed fix
-foreach ($result as $domain => $data) { +foreach ($result as $_ => $data) { $this->assertEquals('premium', $data['type']);
140-142: Same unused variable pattern.Apply the same fix here for consistency.
♻️ Proposed fix
-foreach ($result as $domain => $data) { +foreach ($result as $_ => $data) { $this->assertEquals('suggestion', $data['type']);src/Domains/Registrar/Adapter/NameCom.php (3)
104-121: Consider throwing exception on failure for consistency.
updateNameservers()returns an array withsuccessful: falseon error, while other methods likegetDomain()andupdateDomain()throw exceptions. This inconsistency could lead to caller confusion.♻️ Proposed fix for consistent error handling
public function updateNameservers(string $domain, array $nameservers): array { - try { - $result = $this->send('POST', '/core/v1/domains/' . $domain . ':setNameservers', [ - 'nameservers' => $nameservers, - ]); - - return [ - 'successful' => true, - 'nameservers' => $result['nameservers'] ?? $nameservers, - ]; - } catch (Exception $e) { - return [ - 'successful' => false, - 'error' => $e->getMessage(), - ]; - } + $result = $this->send('POST', '/core/v1/domains/' . $domain . ':setNameservers', [ + 'nameservers' => $nameservers, + ]); + + return [ + 'successful' => true, + 'nameservers' => $result['nameservers'] ?? $nameservers, + ]; }
385-389: Consider documenting TLD retrieval limitation.Returning an empty array is acceptable, but consumers might expect a list. Consider adding
@seeor documentation pointing to Name.com's TLD list for reference.
516-550: Unused parameters and potentially misleading fallback.The
$checkStatusand$getRequestAddressparameters are unused. If they're required by the adapter interface, consider adding a comment explaining why they're not applicable for Name.com.Also, returning
TransferStatusEnum::Transferrablewhen no transfer is found may be misleading - it could mean the domain was never submitted for transfer or isn't owned by the account.♻️ Suggested documentation
public function checkTransferStatus(string $domain, bool $checkStatus = true, bool $getRequestAddress = false): TransferStatus { + // Note: $checkStatus and $getRequestAddress are not used by Name.com API + // but are kept for interface compatibility with other adapters try {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/test.ymlREADME.mdsrc/Domains/Registrar/Adapter/NameCom.phptests/Registrar/BaseRegistrarTest.phptests/Registrar/MockTest.phptests/Registrar/NameComTest.phptests/Registrar/OpenSRSTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-05T13:46:00.588Z
Learnt from: ChiragAgg5k
Repo: utopia-php/domains PR: 44
File: src/Domains/Registrar/OpenSRS.php:292-438
Timestamp: 2025-10-05T13:46:00.588Z
Learning: In the OpenSRS `suggest()` method, non-premium (suggestion) domains should be prioritized and shown first. When a limit is applied, the limit should be filled with suggestion domains first, and premium domains should only be included if there's remaining capacity after processing suggestions.
Applied to files:
tests/Registrar/OpenSRSTest.php
🧬 Code graph analysis (4)
tests/Registrar/NameComTest.php (5)
src/Domains/Cache.php (1)
Cache(7-35)src/Domains/Exception.php (1)
Exception(5-7)src/Domains/Registrar/Exception/AuthException.php (1)
AuthException(7-9)tests/Registrar/BaseRegistrarTest.php (8)
BaseRegistrarTest(13-442)getAdapter(18-18)getAdapterWithCache(23-23)getTestDomain(29-29)generateRandomString(75-86)getPurchaseContact(47-70)getExpectedAdapterName(34-34)getDefaultNameservers(435-441)tests/Registrar/MockTest.php (6)
setUp(19-26)getAdapter(33-36)getAdapterWithCache(38-41)getTestDomain(43-49)getExpectedAdapterName(51-54)getDefaultNameservers(56-62)
tests/Registrar/BaseRegistrarTest.php (6)
src/Domains/Exception.php (1)
Exception(5-7)src/Domains/Registrar/Exception/DomainTakenException.php (1)
DomainTakenException(7-9)src/Domains/Registrar/Exception/InvalidContactException.php (1)
InvalidContactException(7-9)src/Domains/Registrar/Exception/PriceNotFoundException.php (1)
PriceNotFoundException(7-9)tests/Registrar/OpenSRSTest.php (6)
getAdapter(51-54)getAdapterWithCache(56-59)getTestDomain(61-64)getExpectedAdapterName(66-69)getDefaultTld(71-74)getDefaultNameservers(76-82)src/Domains/Registrar/Adapter/NameCom.php (2)
getName(77-80)available(88-95)
tests/Registrar/MockTest.php (4)
src/Domains/Registrar/Exception/DomainTakenException.php (1)
DomainTakenException(7-9)src/Domains/Registrar/Exception/InvalidContactException.php (1)
InvalidContactException(7-9)src/Domains/Registrar/Adapter.php (3)
Adapter(7-110)purchase(35-35)transfer(91-91)tests/Registrar/BaseRegistrarTest.php (8)
BaseRegistrarTest(13-442)getAdapter(18-18)getAdapterWithCache(23-23)getTestDomain(29-29)generateRandomString(75-86)getPurchaseContact(47-70)getExpectedAdapterName(34-34)getDefaultNameservers(435-441)
tests/Registrar/OpenSRSTest.php (4)
src/Domains/Exception.php (1)
Exception(5-7)src/Domains/Registrar/Exception/AuthException.php (1)
AuthException(7-9)src/Domains/Registrar/Exception/DomainNotTransferableException.php (1)
DomainNotTransferableException(7-9)tests/Registrar/BaseRegistrarTest.php (5)
BaseRegistrarTest(13-442)getAdapter(18-18)getAdapterWithCache(23-23)getTestDomain(29-29)getExpectedAdapterName(34-34)
🪛 GitHub Actions: CodeQL
tests/Registrar/NameComTest.php
[error] 50-50: Method Utopia\Tests\Registrar\NameComTest::getAdapter() should return Utopia\Domains\Registrar but returns Utopia\Domains\Registrar\Adapter\NameCom.
[error] 55-55: Method Utopia\Tests\Registrar\NameComTest::getAdapterWithCache() should return Utopia\Domains\Registrar but returns Utopia\Domains\Registrar\Adapter\NameCom.
tests/Registrar/BaseRegistrarTest.php
[error] 204-204: Call to an undefined method Utopia\Domains\Registrar::cancelPurchase().
[error] 309-309: Call to an undefined method Utopia\Domains\Registrar::updateNameservers().
[error] 397-397: Call to an undefined method Utopia\Domains\Registrar::checkTransferStatus().
[error] 426-426: Call to an undefined method Utopia\Domains\Registrar::checkTransferStatus().
tests/Registrar/MockTest.php
[error] 35-35: Method Utopia\Tests\Registrar\MockTest::getAdapter() should return Utopia\Domains\Registrar but returns Utopia\Domains\Registrar\Adapter\Mock.
[error] 40-40: Method Utopia\Tests\Registrar\MockTest::getAdapterWithCache() should return Utopia\Domains\Registrar but returns Utopia\Domains\Registrar\Adapter\Mock.
tests/Registrar/OpenSRSTest.php
[error] 53-53: Method Utopia\Tests\Registrar\OpenSRSTest::getAdapter() should return Utopia\Domains\Registrar but returns Utopia\Domains\Registrar\Adapter\OpenSRS.
[error] 58-58: Method Utopia\Tests\Registrar\OpenSRSTest::getAdapterWithCache() should return Utopia\Domains\Registrar but returns Utopia\Domains\Registrar\Adapter\OpenSRS.
🪛 GitHub Actions: Tests
tests/Registrar/NameComTest.php
[error] 50-50: TypeError: Utopia\Tests\Registrar\NameComTest::getAdapter(): Return value must be of type Utopia\Domains\Registrar, Utopia\Domains\Registrar\Adapter\NameCom returned
[error] 50-50: Utopia\Tests\Registrar\NameComTest::getAdapter(): Return value must be of type Utopia\Domains\Registrar, Utopia\Domains\Registrar\Adapter\NameCom returned
[error] 50-50: Script vendor/bin/phpunit handling the test event returned with error code 2 (TypeError due to invalid adapter return type in NameComTest)
tests/Registrar/MockTest.php
[error] 35-35: TypeError: Utopia\Tests\Registrar\MockTest::getAdapter(): Return value must be of type Utopia\Domains\Registrar, Utopia\Domains\Registrar\Adapter\Mock returned
[error] 35-35: Utopia\Tests\Registrar\MockTest::getAdapter(): Return value must be of type Utopia\Domains\Registrar, Utopia\Domains\Registrar\Adapter\Mock returned
[error] 35-35: Script vendor/bin/phpunit handling the test event returned with error code 2 (TypeError due to invalid adapter return type in Mock adapter tests)
tests/Registrar/OpenSRSTest.php
[error] 53-53: TypeError: Utopia\Tests\Registrar\OpenSRSTest::getAdapter(): Return value must be of type Utopia\Domains\Registrar, Utopia\Domains\Registrar\Adapter\OpenSRS returned
[error] 53-53: Utopia\Tests\Registrar\OpenSRSTest::getAdapter(): Return value must be of type Utopia\Domains\Registrar, Utopia\Domains\Registrar\Adapter\OpenSRS returned
[error] 53-53: Script vendor/bin/phpunit handling the test event returned with error code 2 (TypeError due to invalid adapter return type in OpenSRSTest)
🪛 PHPMD (2.15.0)
tests/Registrar/NameComTest.php
118-118: Avoid unused local variables such as '$domain'. (undefined)
(UnusedLocalVariable)
140-140: Avoid unused local variables such as '$domain'. (undefined)
(UnusedLocalVariable)
tests/Registrar/BaseRegistrarTest.php
39-39: Avoid unused parameters such as '$testName'. (undefined)
(UnusedFormalParameter)
src/Domains/Registrar/Adapter/NameCom.php
516-516: Avoid unused parameters such as '$checkStatus'. (undefined)
(UnusedFormalParameter)
516-516: Avoid unused parameters such as '$getRequestAddress'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (9)
README.md (1)
79-129: LGTM! Clear multi-adapter documentation.The documentation structure is well-organized with separate sections for each adapter, clear constructor examples, and appropriate endpoint comments distinguishing production from sandbox/testing URLs.
tests/Registrar/BaseRegistrarTest.php (1)
36-42: Unused$testNameparameter is acceptable for extensibility.The static analysis flags
$testNameas unused, but this is intentional - subclasses can overrideshouldSkipTest()and use the parameter to conditionally skip specific tests. This is a valid extensibility pattern.tests/Registrar/MockTest.php (1)
66-163: Mock-specific tests look good.The test coverage for nameservers, transfers, invalid contacts, and transfer status is comprehensive and follows good testing patterns.
tests/Registrar/OpenSRSTest.php (1)
84-184: OpenSRS-specific tests provide good coverage.The tests for invalid password, multiple keyword suggestions, premium filtering, and transfer scenarios are well-structured and cover important edge cases.
src/Domains/Registrar/Adapter/NameCom.php (5)
49-70: Constructor implementation looks solid.Good practices: enforces HTTPS, configurable timeouts, optional cache, proper header setup for JSON content type.
132-176: Purchase implementation is well-structured.Good error handling with specific exception mapping. Note that string-based error detection (e.g.,
str_contains($errorLower, 'unavailable')) is somewhat fragile if Name.com changes their error messages, but this is a reasonable approach given API constraints.
259-321: Suggest implementation handles filtering correctly.Client-side price and type filtering is implemented properly with early termination when the limit is reached.
578-625: HTTP transport implementation is secure and robust.Good practices: SSL verification enabled, configurable timeouts, proper error handling for curl failures, JSON encoding errors, and HTTP error responses.
633-692: Contact sanitization handles multiple input formats well.The logic correctly handles single contacts, numeric arrays, and associative arrays with proper mapping to Name.com's expected format including the
owner→registranttranslation.
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)
tests/Registrar/MockTest.php (1)
103-124: Inconsistent contact format intestTransferWithInvalidContact.Line 123 passes
[$invalidContact](indexed array with single Contact), while other tests use$this->getPurchaseContact()which returns a keyed array. This inconsistency mirrors the issue inBase.php::testPurchaseWithInvalidContact.🐛 Suggested fix
- $this->adapter->transfer('transfer.com', 'auth-code', [$invalidContact]); + $this->adapter->transfer('transfer.com', 'auth-code', [ + 'owner' => $invalidContact, + 'admin' => $invalidContact, + 'tech' => $invalidContact, + 'billing' => $invalidContact, + ]);
🤖 Fix all issues with AI agents
In @tests/Registrar/Base.php:
- Around line 173-198: testPurchaseWithInvalidContact constructs contacts as a
plain indexed array with a single Contact, which is inconsistent with other
tests (like testPurchase) that pass a keyed contacts array from
getPurchaseContact() containing owner/admin/tech/billing; update
testPurchaseWithInvalidContact to use the same keyed structure (call
getPurchaseContact() to get the standard array, then replace one role's Contact
with an invalid-email Contact) before calling getAdapter()->purchase so adapters
that expect role keys validate the same shape as other tests.
- Around line 319-333: The test testUpdateNameservers expects updateNameservers
to return an array with keys 'successful' and 'nameservers', but some adapters
(e.g., OpenSRS implementation of updateNameservers) currently return only
['code','text','successful']; fix by normalizing the adapter responses: update
the OpenSRS adapter (and any others missing it) so its updateNameservers method
returns the same structure including a 'nameservers' key (preferably the actual
nameserver list or null/empty array on failure) or alternatively make the test
in Base::testUpdateNameservers check for 'nameservers' only if provided (e.g.,
assertArrayHasKey('nameservers', $result) only when the adapter declares
support) — locate the method updateNameservers in each adapter class and the
testUpdateNameservers in tests/Registrar/Base.php and apply one consistent
approach across all adapters.
🧹 Nitpick comments (5)
tests/Registrar/NameComTest.php (3)
57-64: Test domain purchased on every call could lead to resource accumulation.
getTestDomain()purchases a new domain each time it's invoked. Since the base class tests liketestDomainInfo,testUpdateDomain,testRenewDomain, etc. each callgetTestDomain(), this will purchase multiple domains per test run, potentially accumulating costs and domains in the sandbox account.Consider caching the purchased domain for the test class instance:
♻️ Suggested refactor
class NameComTest extends Base { private NameCom $client; private NameCom $clientWithCache; + private ?string $testDomain = null; // ... setUp() ... protected function getTestDomain(): string { - // For tests that need an existing domain, we'll purchase one on the fly - // or return a domain we know exists - $testDomain = $this->generateRandomString() . '.com'; - $this->client->purchase($testDomain, $this->getPurchaseContact(), 1); - return $testDomain; + if ($this->testDomain === null) { + $this->testDomain = $this->generateRandomString() . '.com'; + $this->client->purchase($this->testDomain, $this->getPurchaseContact(), 1); + } + return $this->testDomain; }
106-106: Consider removing debugechostatements.The
echostatements (lines 106, 116-117, 127, 140, 149) add noise to test output. PHPUnit's--verboseor--debugflags provide this functionality when needed.
129-135: Unused loop variable$domain.The
$domainkey variable is declared but unused in the loop body. Consider using an underscore prefix to indicate intentionally unused variable, or omit the key.♻️ Suggested fix
- foreach ($result as $domain => $data) { + foreach ($result as $data) { $this->assertEquals('premium', $data['type']);Apply the same pattern to line 151.
tests/Registrar/Base.php (1)
362-369: Weak exception handling intestRenewDomain.Catching generic
\Exceptionand only asserting the message is not empty allows any exception to pass the test, potentially masking real bugs. Consider catching more specific exception types or verifying error codes.tests/Registrar/MockTest.php (1)
42-48: Same concern as NameComTest: domain purchased on everygetTestDomain()call.This creates a new domain each time it's called within a test run. Since
MockTestusestearDown()to reset state, this is less problematic here, but caching could still improve efficiency.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
composer.jsontests/Registrar/Base.phptests/Registrar/MockTest.phptests/Registrar/NameComTest.phptests/Registrar/OpenSRSTest.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-05T13:46:00.588Z
Learnt from: ChiragAgg5k
Repo: utopia-php/domains PR: 44
File: src/Domains/Registrar/OpenSRS.php:292-438
Timestamp: 2025-10-05T13:46:00.588Z
Learning: In the OpenSRS `suggest()` method, non-premium (suggestion) domains should be prioritized and shown first. When a limit is applied, the limit should be filled with suggestion domains first, and premium domains should only be included if there's remaining capacity after processing suggestions.
Applied to files:
tests/Registrar/OpenSRSTest.php
🧬 Code graph analysis (3)
tests/Registrar/MockTest.php (4)
tests/Registrar/NameComTest.php (5)
getAdapter(47-50)getAdapterWithCache(52-55)getTestDomain(57-64)getExpectedAdapterName(66-69)getDefaultNameservers(71-77)tests/Registrar/OpenSRSTest.php (5)
getAdapter(50-53)getAdapterWithCache(55-58)getTestDomain(60-63)getExpectedAdapterName(65-68)getDefaultNameservers(75-81)src/Domains/Registrar/Adapter/Mock.php (3)
Mock(19-551)purchase(144-163)transfer(386-406)src/Domains/Registrar/Adapter.php (2)
purchase(35-35)transfer(91-91)
tests/Registrar/Base.php (5)
src/Domains/Registrar/Contact.php (1)
Contact(5-44)src/Domains/Exception.php (1)
Exception(5-7)src/Domains/Registrar/Exception/DomainTakenException.php (1)
DomainTakenException(7-9)src/Domains/Registrar/Exception/InvalidContactException.php (1)
InvalidContactException(7-9)src/Domains/Registrar/Exception/PriceNotFoundException.php (1)
PriceNotFoundException(7-9)
tests/Registrar/OpenSRSTest.php (5)
src/Domains/Exception.php (1)
Exception(5-7)src/Domains/Registrar/Exception/DomainNotTransferableException.php (1)
DomainNotTransferableException(7-9)tests/Registrar/Base.php (9)
Base(13-462)generateRandomString(84-95)getAdapter(18-18)getAdapterWithCache(23-23)getTestDomain(29-29)getExpectedAdapterName(34-34)getDefaultNameservers(455-461)shouldSkipTest(43-51)getPurchaseContact(56-79)tests/Registrar/MockTest.php (5)
getAdapter(32-35)getAdapterWithCache(37-40)getTestDomain(42-48)getExpectedAdapterName(50-53)getDefaultNameservers(55-61)src/Domains/Registrar/Exception/AuthException.php (1)
AuthException(7-9)
🪛 GitHub Actions: Tests
tests/Registrar/Base.php
[error] 332-332: Test failure: array does not contain required key 'nameservers'
🪛 PHPMD (2.15.0)
tests/Registrar/OpenSRSTest.php
83-83: Avoid unused parameters such as '$testName'. (undefined)
(UnusedFormalParameter)
tests/Registrar/NameComTest.php
79-79: Avoid unused parameters such as '$testName'. (undefined)
(UnusedFormalParameter)
129-129: Avoid unused local variables such as '$domain'. (undefined)
(UnusedLocalVariable)
151-151: Avoid unused local variables such as '$domain'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (8)
composer.json (1)
20-22: LGTM!The
autoload-devPSR-4 namespace mapping correctly enables the new test class hierarchy underUtopia\Tests\.tests/Registrar/Base.php (2)
13-51: Good abstract test base design.The abstract base class with
shouldSkipTest()pattern provides flexible test scaffolding that accommodates adapters with varying feature support. The helper methods (getPurchaseContact,generateRandomString) reduce duplication across test classes.
216-225: No action needed. ThecancelPurchase()method is intentionally designed without parameters. Unlike domain-specific operations (purchase, renew, transfer),cancelPurchase()is a batch operation that cancels pending orders based on date and status criteria. The @phpstan-ignore comment is correct since this is an optional method not part of the base Adapter interface.tests/Registrar/OpenSRSTest.php (3)
16-16: Hardcoded test domain assumes pre-existing state.The
testDomainis hardcoded to'kffsfudlvc.net'. UnlikeNameComTestandMockTestwhich purchase domains on-the-fly, this assumes the domain already exists in the test account. This could cause test failures if the domain doesn't exist or expires.Consider documenting this dependency or purchasing the domain in
setUp()similar to other adapters.
50-87: LGTM!The abstract method implementations correctly integrate with the base test class. The
getDefaultTld()override to'net'is appropriate for OpenSRS testing, andshouldSkipTestreturningfalseindicates full feature support.
164-188: Good exception handling pattern for transfer tests.The transfer tests properly catch
DomainNotTransferableExceptionwith specific code and message assertions, allowing tests to pass whether the transfer succeeds or fails with an expected error.tests/Registrar/MockTest.php (2)
27-30: Good practice: resetting adapter state intearDown.Calling
reset()ensures test isolation by cleaning up purchased/transferred domains between tests.
65-88: LGTM!The Mock-specific tests for purchase and transfer with nameservers are well-structured and verify the expected behavior of the Mock adapter.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/Domains/Registrar/Adapter/NameCom.php:
- Around line 466-473: The success check uses !empty($orderId) which treats the
string '0' as empty and will incorrectly mark a valid orderId of '0' as
unsuccessful; change the check used when constructing the Renewal (successful:
...) to an explicit non-empty-string test such as $orderId !== '' or
strlen($orderId) > 0 (given $orderId is cast to string above) so that '0' is
considered a successful result; update the successful: expression in the
Renewal(...) call accordingly.
- Around line 632-666: sanitizeContacts currently returns an empty array for an
explicitly empty contacts array (allowing invalid data through); add a
validation guard to fail fast: in sanitizeContacts(array $contacts) check if
empty($contacts) and throw an InvalidArgumentException (or Domain-specific
exception) with a clear message like "contacts must be a non-empty array", and
also ensure purchase() and transfer() (which normalize contacts via
is_array($contacts) ? $contacts : [$contacts]) either rely on that guard or
perform the same non-empty validation before calling sanitizeContacts to provide
earlier, contextual errors.
- Around line 143-151: The Registration being returned uses the wrong field name
for the Name.com v4 response; update the references to use the camelCase field
returned by the API by replacing occurrences of $result['order_id'] with
$result['orderId'] in the Registration construction (the code that builds the
new Registration with code and id) so the actual order ID is captured instead of
falling back to the defaults.
🧹 Nitpick comments (14)
tests/Registrar/NameComTest.php (3)
62-69: Consider the cost implications of purchasing domains in getTestDomain.This implementation purchases a new domain every time
getTestDomain()is called, which could incur costs and slow down tests if called multiple times within the test suite. Consider caching the purchased domain or using a static property to prevent multiple purchases.♻️ Suggested improvement to cache the test domain
+ private ?string $cachedTestDomain = null; + protected function getTestDomain(): string { + if ($this->cachedTestDomain !== null) { + return $this->cachedTestDomain; + } + // For tests that need an existing domain, we'll purchase one on the fly // or return a domain we know exists $testDomain = $this->generateRandomString() . '.com'; $this->registrar->purchase($testDomain, $this->getPurchaseContact(), 1); + $this->cachedTestDomain = $testDomain; return $testDomain; }
131-137: Use underscore prefix for intentionally unused loop variables.The static analysis correctly identifies that
$domainis unused in these foreach loops. Using an underscore prefix is a common convention to indicate intentionally unused variables.♻️ Proposed fix for unused loop variables
- foreach ($result as $domain => $data) { + foreach ($result as $_domain => $data) { $this->assertEquals('premium', $data['type']); if ($data['price'] !== null) { $this->assertGreaterThanOrEqual(100, $data['price']); $this->assertLessThanOrEqual(10000, $data['price']); } }- foreach ($result as $domain => $data) { + foreach ($result as $_domain => $data) { $this->assertEquals('suggestion', $data['type']); }Also applies to: 151-153
118-138: Premium domain suggestion test may return empty results.The test iterates over results but doesn't assert that results are non-empty. If the Name.com API returns no premium domains matching the criteria, the test would pass without validating anything meaningful.
♻️ Add assertion for non-empty results
$this->assertIsArray($result); + // Ensure we got at least some results to validate + $this->assertNotEmpty($result, 'Expected at least one premium domain suggestion'); foreach ($result as $domain => $data) {src/Domains/Registrar/Adapter/Mock.php (1)
504-517: Consider validating domain ownership in updateNameservers.The
updateNameserversmethod ignores the$domainparameter entirely and doesn't verify that the domain exists inpurchasedDomains. For consistency with other Mock methods likegetDomain,renew, andupdateDomain, consider adding validation.♻️ Add domain validation for consistency
public function updateNameservers(string $domain, array $nameservers): array { + if (!in_array($domain, $this->purchasedDomains)) { + return [ + 'successful' => false, + 'error' => "Domain {$domain} not found in mock registry", + ]; + } + return [ 'successful' => true, 'nameservers' => $nameservers, ]; }tests/Registrar/Base.php (2)
273-284: Consider more specific exception handling in testRenewDomain.Catching generic
\Exceptionand only asserting the message is non-empty doesn't verify the expected behavior. If renewal failure is expected for certain scenarios, consider documenting the expected exception types.♻️ Add comment documenting expected exception types
public function testRenewDomain(): void { $testDomain = $this->getTestDomain(); try { $result = $this->getRegistrar()->renew($testDomain, 1); $this->assertIsBool($result->successful); } catch (\Exception $e) { - // Renewal may fail for various reasons depending on the registrar + // Renewal may fail for various reasons: + // - Domain not in renewable period + // - Insufficient account balance (sandbox) + // - Domain locked $this->assertNotEmpty($e->getMessage()); } }
286-300: Exception assertion may be too strict for transfer test.The test expects only
DomainNotTransferableExceptionwhen the transfer fails, but other exceptions may be valid (e.g.,AuthException,DomainsException). Consider broadening the assertion or documenting the expected behavior.♻️ Broaden exception handling
} catch (\Exception $e) { - $this->assertInstanceOf(DomainNotTransferableException::class, $e); + // Transfer failures can occur for various reasons + $this->assertTrue( + $e instanceof DomainNotTransferableException || + $e instanceof \Utopia\Domains\Exception, + 'Expected a domain-related exception, got: ' . get_class($e) + ); }src/Domains/Registrar/Adapter/NameCom.php (4)
194-196: Redundant check for nameservers.The
if (!empty($nameservers))check on line 194 is redundant because line 183 already ensures$nameserversis populated with defaults if empty.♻️ Suggested simplification
- if (!empty($nameservers)) { - $data['nameservers'] = $nameservers; - } + $data['nameservers'] = $nameservers;
326-332: Cache key is computed twice; extract to local variable.The cache key
$domain . '_' . $regType . '_' . $periodYearsis constructed identically on lines 328 and 350. Consider extracting to a local variable at the start of the method.♻️ Suggested refactor
public function getPrice(string $domain, int $periodYears = 1, string $regType = Registrar::REG_TYPE_NEW, int $ttl = 3600): float { + $cacheKey = $domain . '_' . $regType . '_' . $periodYears; + if ($this->cache) { - $cacheKey = $domain . '_' . $regType . '_' . $periodYears; $cached = $this->cache->load($cacheKey, $ttl); if ($cached !== null && is_array($cached)) { return $cached['price']; } } // ... later in the method ... if ($this->cache) { - $cacheKey = $domain . '_' . $regType . '_' . $periodYears; $this->cache->save($cacheKey, ['price' => $price]); }Also applies to: 349-352
510-544: Unused parameters$checkStatusand$getRequestAddress.Per static analysis, these parameters are declared but never used in the method body. If they're part of an interface contract that this adapter doesn't need, consider documenting this explicitly in the docblock or using a
@SuppressWarningsannotation.📝 Documentation suggestion
/** * Check transfer status for a domain * * @param string $domain The domain name - * @param bool $checkStatus Flag to check status - * @param bool $getRequestAddress Flag to get request address + * @param bool $checkStatus Flag to check status (not used by Name.com adapter) + * @param bool $getRequestAddress Flag to get request address (not used by Name.com adapter) * @return TransferStatus Transfer status information */
552-562: Mapping unknown transfer statuses toNotTransferrablemay be misleading.The default case maps any unrecognized status to
NotTransferrable, which could mask unexpected API responses or new status values. Consider logging unknown statuses or mapping to a more neutral state if available.tests/Registrar/OpenSRSTest.php (2)
64-67: Test domainkffsfudlvc.netis hardcoded; consider documenting its purpose.The hardcoded test domain is used for transfer tests. A brief comment explaining why this specific domain was chosen (e.g., known to exist, sandbox domain) would improve test maintainability.
87-88: Extraneous blank lines.Lines 87-88 contain consecutive blank lines before the OpenSRS-specific tests section comment.
tests/Registrar/MockTest.php (2)
45-51:getTestDomain()creates a new domain on every call; consider caching.Unlike OpenSRSTest which returns a fixed test domain, this implementation purchases a new domain each time
getTestDomain()is called. If the Base class or any test calls this multiple times, it could lead to unexpected behavior or test pollution.♻️ Consider caching the test domain
+ private ?string $testDomain = null; + protected function getTestDomain(): string { + if ($this->testDomain !== null) { + return $this->testDomain; + } // For mock, we purchase a domain on the fly - $testDomain = $this->generateRandomString() . '.com'; - $this->registrar->purchase($testDomain, $this->getPurchaseContact(), 1); - return $testDomain; + $this->testDomain = $this->generateRandomString() . '.com'; + $this->registrar->purchase($this->testDomain, $this->getPurchaseContact(), 1); + return $this->testDomain; }
159-165: Consider asserting specific status value, not just type.The test only verifies that
$result->statusis an instance ofTransferStatusEnum, but doesn't assert a specific expected value. This reduces the test's ability to catch regressions.💡 Suggestion
public function testCheckTransferStatusWithRequestAddress(): void { $domain = 'example.com'; $result = $this->registrar->checkTransferStatus($domain, false, true); $this->assertInstanceOf(\Utopia\Domains\Registrar\TransferStatusEnum::class, $result->status); + // Consider adding: $this->assertEquals(TransferStatusEnum::ExpectedValue, $result->status); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/Domains/Registrar.phpsrc/Domains/Registrar/Adapter.phpsrc/Domains/Registrar/Adapter/Mock.phpsrc/Domains/Registrar/Adapter/NameCom.phpsrc/Domains/Registrar/Adapter/OpenSRS.phptests/Registrar/Base.phptests/Registrar/MockTest.phptests/Registrar/NameComTest.phptests/Registrar/OpenSRSTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Domains/Registrar/Adapter/OpenSRS.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-05T13:46:00.588Z
Learnt from: ChiragAgg5k
Repo: utopia-php/domains PR: 44
File: src/Domains/Registrar/OpenSRS.php:292-438
Timestamp: 2025-10-05T13:46:00.588Z
Learning: In the OpenSRS `suggest()` method, non-premium (suggestion) domains should be prioritized and shown first. When a limit is applied, the limit should be filled with suggestion domains first, and premium domains should only be included if there's remaining capacity after processing suggestions.
Applied to files:
tests/Registrar/OpenSRSTest.php
🧬 Code graph analysis (5)
src/Domains/Registrar.php (3)
src/Domains/Registrar/TransferStatus.php (2)
TransferStatus(19-27)__construct(21-26)src/Domains/Registrar/Adapter/OpenSRS.php (4)
__construct(54-75)updateNameservers(100-130)cancelPurchase(253-277)checkTransferStatus(750-818)src/Domains/Registrar/Adapter.php (7)
setDefaultNameservers(37-40)setCache(48-51)setConnectTimeout(59-62)setTimeout(70-73)updateNameservers(147-150)cancelPurchase(207-207)checkTransferStatus(200-200)
tests/Registrar/OpenSRSTest.php (3)
src/Domains/Exception.php (1)
Exception(5-7)src/Domains/Registrar/Exception/AuthException.php (1)
AuthException(7-9)src/Domains/Registrar/Exception/DomainNotTransferableException.php (1)
DomainNotTransferableException(7-9)
src/Domains/Registrar/Adapter.php (3)
src/Domains/Cache.php (1)
Cache(7-35)src/Domains/Registrar.php (4)
Registrar(12-224)getName(59-62)getPrice(160-163)renew(172-175)src/Domains/Registrar/Adapter/OpenSRS.php (3)
getName(39-42)getPrice(469-516)renew(659-704)
src/Domains/Registrar/Adapter/Mock.php (4)
src/Domains/Registrar.php (4)
Registrar(12-224)getPrice(160-163)updateNameservers(146-149)cancelPurchase(207-210)src/Domains/Registrar/Adapter.php (3)
getPrice(161-161)updateNameservers(147-150)cancelPurchase(207-207)src/Domains/Registrar/Adapter/NameCom.php (3)
getPrice(325-372)updateNameservers(96-113)cancelPurchase(234-239)src/Domains/Registrar/Adapter/OpenSRS.php (3)
getPrice(469-516)updateNameservers(100-130)cancelPurchase(253-277)
tests/Registrar/NameComTest.php (2)
src/Domains/Registrar/Exception/AuthException.php (1)
AuthException(7-9)src/Domains/Registrar/Adapter/NameCom.php (2)
NameCom(23-692)purchase(124-167)
🪛 PHPMD (2.15.0)
src/Domains/Registrar/Adapter.php
147-147: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
147-147: Avoid unused parameters such as '$nameservers'. (undefined)
(UnusedFormalParameter)
src/Domains/Registrar/Adapter/NameCom.php
510-510: Avoid unused parameters such as '$checkStatus'. (undefined)
(UnusedFormalParameter)
510-510: Avoid unused parameters such as '$getRequestAddress'. (undefined)
(UnusedFormalParameter)
src/Domains/Registrar/Adapter/Mock.php
511-511: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
tests/Registrar/NameComTest.php
131-131: Avoid unused local variables such as '$domain'. (undefined)
(UnusedLocalVariable)
151-151: Avoid unused local variables such as '$domain'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (27)
tests/Registrar/NameComTest.php (1)
18-50: Well-structured test setup with proper environment validation.The setUp method properly validates environment variables, creates both cached and non-cached registrar instances, and uses the sandbox API endpoint for testing. The use of assertions for environment variable checks is appropriate for integration tests.
src/Domains/Registrar/Adapter/Mock.php (4)
18-18: LGTM!The import of the
Registrarclass is correctly added to support the use ofRegistrar::REG_TYPE_*constants.
91-105: LGTM!The constructor signature change correctly removes the
Cacheparameter since cache is now set viasetCache()from the baseAdapterclass, as configured by theRegistrarconstructor. This aligns with the dependency injection pattern established across all adapters.
270-317: LGTM!The
getPricemethod correctly usesRegistrar::REG_TYPE_*constants for the multiplier mapping, aligning with the centralized constant definitions in theRegistrarclass. The caching logic is properly implemented.
519-527: LGTM!The
cancelPurchasemethod correctly implements the abstract method from the baseAdapterclass. For a mock adapter, returningtrueis appropriate as it simulates successful cancellation.src/Domains/Registrar/Adapter.php (5)
11-29: Well-designed adapter configuration properties.The addition of
defaultNameservers,cache,connectTimeout, andtimeoutproperties with sensible defaults (5s connect, 10s request) provides good configuration flexibility while maintaining reasonable defaults for production use.
31-73: LGTM!The setter methods are clean and provide proper dependency injection points. This design allows the
Registrarclass to configure adapters uniformly while keeping adapter construction simple.
139-150: Default implementation with exception is intentional for optional override.The static analysis flags unused parameters, but this is intentional for a base class method that throws "not implemented". This pattern allows adapters to optionally override the method. The PHPDoc correctly documents the
@throws \Exception.
161-161: LGTM!Using
Registrar::REG_TYPE_NEWas the default aligns the abstract method signature with the centralized constants in theRegistrarclass, ensuring consistency across all adapter implementations.
201-207: LGTM!The abstract
cancelPurchasemethod correctly enforces that all adapters must implement this functionality, aligning with the expanded public API surface of theRegistrarclass.src/Domains/Registrar.php (5)
10-10: LGTM!The
TransferStatusimport is correctly added to support the return type of the newcheckTransferStatusmethod.
14-20: LGTM!Moving the registration type constants to literal strings in the
Registrarclass is cleaner than delegating to adapter constants. This centralizes the domain vocabulary in one place.
24-52: Well-designed constructor with sensible defaults.The expanded constructor properly configures the adapter through setter methods, maintaining clean separation of concerns. The conditional checks for
defaultNameserversandcacheavoid unnecessary calls when defaults are used.
139-149: LGTM!The
updateNameserversmethod correctly delegates to the adapter and follows the established pattern of other Registrar methods.
201-223: LGTM!Both
cancelPurchaseandcheckTransferStatusmethods are properly implemented as simple delegations to the adapter, maintaining consistency with the rest of the class. The docblocks accurately describe the parameters and return types.tests/Registrar/Base.php (5)
14-35: Well-designed abstract test base class.The abstract methods provide a clean contract for adapter-specific test classes to implement. This design promotes test consistency across different registrar adapters (NameCom, OpenSRS, Mock).
40-63: LGTM!The
getPurchaseContacthelper creates comprehensive contact data with all required fields. The suffix parameter allows creating distinct contacts for update tests.
68-79: LGTM!Using
random_int()for generating random strings is cryptographically secure and appropriate for generating unique test domain names.
316-339: Comprehensive transfer status validation.The test properly validates the
TransferStatusobject structure and handles the various possible status values. The assertion for the status enum values is thorough.
349-359: Default nameservers may not be valid for all adapters.The default
ns1.example.comandns2.example.comnameservers may not be accepted by all registrars (e.g., some require registered nameservers). The method is correctly marked as overridable, which NameComTest demonstrates by providing Name.com's actual nameservers.src/Domains/Registrar/Adapter/NameCom.php (2)
45-62: LGTM! Secure endpoint normalization.The constructor properly enforces HTTPS by normalizing HTTP to HTTPS and prepending the scheme when missing. This is a good security practice for API communication.
572-624: LGTM! Solid HTTP transport implementation.The
send()method properly handles:
- SSL verification enabled (lines 583-584)
- JSON encoding error handling (lines 588-592)
- cURL error handling with resource cleanup (lines 600-604)
- HTTP error response mapping with specific exception types (lines 613-621)
- Timeout configuration via inherited properties (lines 581-582)
tests/Registrar/OpenSRSTest.php (2)
13-52: LGTM! Clean migration to Base class pattern.The test class properly extends
Baseand implements all required abstract methods. The setup correctly initializes both registrar instances with and without cache, following the established pattern.
113-138: Test aligns with learned preference for suggestion domain prioritization.The test correctly verifies that when
filterTypeis'suggestion', only non-premium domains are returned. This aligns with the documented preference for prioritizing suggestion domains. Based on learnings.tests/Registrar/MockTest.php (3)
14-28: LGTM! Proper setup with registrar instances.The class correctly initializes both registrar instances and maintains the adapter reference for the
reset()call intearDown().
58-64: Default nameservers differ from those used in tests.
getDefaultNameservers()returnsns1.example.comandns2.example.com, which matches the nameservers used intestPurchaseWithNameserversandtestTransferWithNameservers. This is good consistency.
93-104: Good test coverage for domain-already-exists transfer scenario.The test properly sets up the precondition by purchasing the domain first, then verifies the expected exception when attempting to transfer an already-owned domain.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @src/Domains/Registrar.php:
- Around line 32-51: The constructor Registrar::__construct currently type-hints
?Cache $cache but the Cache class is not imported; add the correct use statement
for the Cache class used by your project (or replace the type-hint with a
fully-qualified class name) so the ?Cache $cache parameter resolves (ensure the
import matches the actual Cache class/namespace used elsewhere in the codebase).
In @src/Domains/Registrar/Adapter/NameCom.php:
- Around line 140-143: The code returns $result['order'] directly which can
cause a notice if 'order' is missing; in the method that calls
$this->send('POST', '/core/v1/domains', $data) (in class NameCom.php), update
the return to safely handle missing keys by checking isset($result['order']) or
using the null coalescing operator (e.g. return $result['order'] ?? null) or
throw a clear exception if an order is required, ensuring callers get a defined
value or an explicit error instead of a PHP notice.
- Around line 190-193: The transfer handling returns $result['order'] without
checking it, risking a null/undefined access; update the code around the
send('POST', '/core/v1/transfers', $data) call in NameCom.php to defensively
verify the response contains an 'order' key (e.g., use isset or
array_key_exists) and handle the missing case (throw a descriptive exception or
return null/error), and ensure any callers of the method get a consistent error
path.
- Around line 538-548: The mapTransferStatus function currently misses several
Name.com API transfer statuses and misclassifies some (e.g., 'canceled' vs
'cancelled'); update mapTransferStatus to include 'pending_transfer',
'pending_unlock', and 'pending_insert' mapping to
TransferStatusEnum::PendingRegistry, add 'failed' mapping to
TransferStatusEnum::Cancelled, and accept both 'canceled' and 'cancelled'
mapping to TransferStatusEnum::Cancelled (in addition to existing mappings for
'pending', 'approved', 'complete', 'completed', 'pending_owner',
'pending_admin', and default behavior). Ensure you reference the
mapTransferStatus method and use the TransferStatusEnum values exactly as named.
In @tests/Registrar/Base.php:
- Around line 273-285: The test testRenewDomain incorrectly expects renew() to
return a string; renew() returns a Renewal object per the Adapter contract, so
update the assertions to assert the result is a Renewal instance and check its
orderId property is a non-empty string (e.g., use assertInstanceOf for Renewal
and assertNotEmpty/assertIsString on $result->orderId), and add the import use
Utopia\Domains\Registrar\Renewal; at the top of the file.
🧹 Nitpick comments (8)
tests/Registrar/Base.php (1)
68-79: Consider usingrandom_bytes()for more robust random string generation.The current implementation is fine for testing, but
random_bytes()with base conversion would be more cryptographically sound if these strings are ever used in contexts requiring unpredictability.src/Domains/Registrar/Adapter/OpenSRS.php (2)
16-21: Remove unused importRegistration.The
Registrationclass is imported but no longer used after thepurchase()andtransfer()methods were changed to returnstringinstead ofRegistration.Proposed fix
use Utopia\Domains\Registrar\Adapter; -use Utopia\Domains\Registrar\Registration; use Utopia\Domains\Registrar\Renewal;
803-833: Consider adding timeout configuration to curl requests.The
send()method doesn't use$this->connectTimeoutand$this->timeoutproperties that are now available in the base adapter. This could lead to indefinite hangs if the OpenSRS API is unresponsive.Proposed fix
$ch = curl_init($this->endpoint); curl_setopt($ch, CURLOPT_RETURNTRANSFER, true); curl_setopt($ch, CURLOPT_HTTPHEADER, $headers); curl_setopt($ch, CURLOPT_POST, 1); curl_setopt($ch, CURLOPT_POSTFIELDS, $xml); + curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, $this->connectTimeout); + curl_setopt($ch, CURLOPT_TIMEOUT, $this->timeout);src/Domains/Registrar/Adapter/NameCom.php (3)
1-22: Remove unused importRegistration.The
Registrationclass is imported on line 16 but is never used in this file.Proposed fix
use Utopia\Domains\Registrar\Adapter; -use Utopia\Domains\Registrar\Registration; use Utopia\Domains\Registrar\Renewal;
215-225: Document thatcancelPurchase()is a no-op for Name.com.The method always returns
truewithout performing any action. While documented in the docblock, consider logging a warning or returning a more informative response for debugging purposes.
496-530: Unused parameters$checkStatusand$getRequestAddressshould be utilized or documented.The method signature includes these parameters for interface compliance, but they're not used in the implementation. If Name.com's API supports different query modes based on these flags, implement them. Otherwise, add a docblock note explaining they're not applicable to this registrar.
Proposed documentation update
/** * Check transfer status for a domain * * @param string $domain The domain name - * @param bool $checkStatus Flag to check status - * @param bool $getRequestAddress Flag to get request address + * @param bool $checkStatus Flag to check status (not used by Name.com API) + * @param bool $getRequestAddress Flag to get request address (not used by Name.com API) * @return TransferStatus Transfer status information */tests/Registrar/OpenSRSTest.php (2)
87-89: Minor: Extra blank lines.Lines 87-88 have unnecessary blank lines before the OpenSRS-specific tests comment.
Suggested cleanup
} - - // OpenSRS-specific tests public function testPurchaseWithInvalidPassword(): void
168-180: Consider adding explicit assertions for the success path.The try-catch pattern is appropriate for sandbox testing where outcomes vary. However, when
transfer()succeeds without exception, the test silently passes without verifying meaningful behavior. Consider adding a comment explaining this is expected sandbox behavior.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
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/Registration.phptests/Registrar/Base.phptests/Registrar/MockTest.phptests/Registrar/OpenSRSTest.php
💤 Files with no reviewable changes (1)
- src/Domains/Registrar/Registration.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-05T13:46:00.588Z
Learnt from: ChiragAgg5k
Repo: utopia-php/domains PR: 44
File: src/Domains/Registrar/OpenSRS.php:292-438
Timestamp: 2025-10-05T13:46:00.588Z
Learning: In the OpenSRS `suggest()` method, non-premium (suggestion) domains should be prioritized and shown first. When a limit is applied, the limit should be filled with suggestion domains first, and premium domains should only be included if there's remaining capacity after processing suggestions.
Applied to files:
tests/Registrar/OpenSRSTest.php
🧬 Code graph analysis (5)
tests/Registrar/Base.php (5)
src/Domains/Exception.php (1)
Exception(5-7)src/Domains/Registrar/Exception/DomainTakenException.php (1)
DomainTakenException(7-9)src/Domains/Registrar/Exception/DomainNotTransferableException.php (1)
DomainNotTransferableException(7-9)src/Domains/Registrar/Exception/InvalidContactException.php (1)
InvalidContactException(7-9)src/Domains/Registrar/Exception/PriceNotFoundException.php (1)
PriceNotFoundException(7-9)
src/Domains/Registrar/Adapter.php (6)
src/Domains/Cache.php (1)
Cache(7-35)src/Domains/Registrar.php (3)
Registrar(11-223)renew(171-174)transfer(185-188)src/Domains/Registrar/Contact.php (1)
Contact(5-44)src/Domains/Registrar/Domain.php (1)
Domain(7-17)src/Domains/Exception.php (1)
Exception(5-7)src/Domains/Registrar/Renewal.php (1)
Renewal(7-15)
src/Domains/Registrar/Adapter/NameCom.php (11)
src/Domains/Registrar.php (16)
Registrar(11-223)__construct(32-51)getName(58-61)available(69-72)updateNameservers(145-148)purchase(83-86)transfer(185-188)cancelPurchase(206-209)suggest(99-102)tlds(109-112)getPrice(159-162)getDomain(120-123)updateDomain(133-136)renew(171-174)getAuthCode(196-199)checkTransferStatus(219-222)src/Domains/Registrar/Adapter.php (15)
Adapter(9-208)getName(80-80)available(88-88)updateNameservers(147-150)purchase(99-99)transfer(182-182)cancelPurchase(207-207)suggest(112-112)tlds(119-119)getPrice(161-161)getDomain(127-127)updateDomain(137-137)renew(170-170)getAuthCode(190-190)checkTransferStatus(200-200)src/Domains/Registrar/Contact.php (2)
Contact(5-44)toArray(24-43)src/Domains/Registrar/Exception/DomainTakenException.php (1)
DomainTakenException(7-9)src/Domains/Registrar/Exception/DomainNotTransferableException.php (1)
DomainNotTransferableException(7-9)src/Domains/Registrar/Exception/InvalidContactException.php (1)
InvalidContactException(7-9)src/Domains/Registrar/Exception/AuthException.php (1)
AuthException(7-9)src/Domains/Registrar/Exception/PriceNotFoundException.php (1)
PriceNotFoundException(7-9)src/Domains/Cache.php (3)
Cache(7-35)load(21-24)save(26-29)src/Domains/Registrar/Renewal.php (1)
Renewal(7-15)src/Domains/Registrar/TransferStatus.php (1)
TransferStatus(19-27)
tests/Registrar/OpenSRSTest.php (4)
src/Domains/Exception.php (1)
Exception(5-7)src/Domains/Registrar/Exception/AuthException.php (1)
AuthException(7-9)src/Domains/Registrar/Exception/DomainNotTransferableException.php (1)
DomainNotTransferableException(7-9)src/Domains/Registrar/Adapter/OpenSRS.php (1)
OpenSRS(23-1153)
src/Domains/Registrar/Adapter/Mock.php (2)
src/Domains/Registrar/Adapter.php (5)
purchase(99-99)getPrice(161-161)transfer(182-182)updateNameservers(147-150)cancelPurchase(207-207)src/Domains/Registrar/Adapter/OpenSRS.php (5)
purchase(167-199)getPrice(452-499)transfer(201-234)updateNameservers(100-130)cancelPurchase(236-260)
🪛 PHPMD (2.15.0)
src/Domains/Registrar/Adapter.php
147-147: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
147-147: Avoid unused parameters such as '$nameservers'. (undefined)
(UnusedFormalParameter)
src/Domains/Registrar/Adapter/NameCom.php
496-496: Avoid unused parameters such as '$checkStatus'. (undefined)
(UnusedFormalParameter)
496-496: Avoid unused parameters such as '$getRequestAddress'. (undefined)
(UnusedFormalParameter)
src/Domains/Registrar/Adapter/Mock.php
134-134: Avoid unused parameters such as '$periodYears'. (undefined)
(UnusedFormalParameter)
134-134: Avoid unused parameters such as '$nameservers'. (undefined)
(UnusedFormalParameter)
368-368: Avoid unused parameters such as '$authCode'. (undefined)
(UnusedFormalParameter)
368-368: Avoid unused parameters such as '$periodYears'. (undefined)
(UnusedFormalParameter)
368-368: Avoid unused parameters such as '$nameservers'. (undefined)
(UnusedFormalParameter)
492-492: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (29)
src/Domains/Registrar/Adapter.php (5)
6-7: LGTM! Import additions align with the updated adapter contract.The new imports for
CacheandRegistrarare required for the dependency injection pattern and constant references throughout the class.
11-73: LGTM! Well-structured dependency injection properties and setters.The new properties (
defaultNameservers,cache,connectTimeout,timeout) with sensible defaults and corresponding setters enable flexible adapter configuration. This aligns well with theRegistrarconstructor that wires these through to the adapter.
147-150: Default implementation appropriately throws for unimplemented behavior.The static analysis warning about unused parameters is a false positive here—this is an intentional stub that concrete adapters can override. The signature must match the contract even if this default throws.
202-207: LGTM! New abstract method completes the adapter contract.The
cancelPurchase()method is properly declared as abstract, requiring all concrete adapters to implement it.
161-161: All adapter implementations are consistent. Each concrete adapter (OpenSRS, NameCom, Mock) uses theRegistrar::REG_TYPE_NEWdefault value in theirgetPricemethod signature, matching the abstract method definition.tests/Registrar/Base.php (2)
1-15: LGTM! Well-structured abstract test base for registrar adapters.The class provides a clean abstraction with appropriate abstract methods for adapter-specific setup and comprehensive test coverage across all registrar operations.
137-158: All adapters correctly handle unkeyed single-contact arrays—no action needed.The test properly exercises the intended code path. Both OpenSRS and NameCom have explicit logic to detect single-element unkeyed arrays and map them to required keyed formats (owner/admin/tech/billing). The Mock adapter's
validateContacts()validates the invalid email format and throwsInvalidContactExceptionas expected.src/Domains/Registrar/Adapter/OpenSRS.php (3)
54-58: LGTM! Constructor simplified to use setter-based dependency injection.The removal of
defaultNameserversandcacheparameters from the constructor aligns with the new pattern where these are injected via theRegistrarclass using setter methods.
167-183: LGTM! Purchase method returns order ID string as per updated contract.The method now correctly returns
$result['id']directly instead of constructing aRegistrationobject, aligning with the simplified API surface.
201-217: LGTM! Transfer method returns order ID string as per updated contract.Consistent with the
purchase()changes, the method now returns the order ID directly.src/Domains/Registrar/Adapter/Mock.php (6)
16-16: LGTM! Import added for Registrar constants.
88-102: LGTM! Constructor simplified, cache now injected via setter.The removal of the
cacheparameter aligns with the updated dependency injection pattern.
134-145: LGTM! Purchase returns order ID string.The static analysis warnings about unused
$periodYearsand$nameserversare expected for a mock implementation that doesn't need to use all parameters.
259-306: LGTM! getPrice properly uses Registrar constants for multiplier mapping.The switch from
self::REG_TYPE_*toRegistrar::REG_TYPE_*is correct and maintains consistent behavior.
368-380: LGTM! Transfer returns order ID string.Static analysis warnings about unused parameters are expected for a mock.
485-508: LGTM! New methods implement the updated adapter contract.Both
updateNameservers()andcancelPurchase()provide appropriate mock implementations. The unused$domainparameter inupdateNameserversis expected since the mock doesn't perform actual domain operations.src/Domains/Registrar/Adapter/NameCom.php (4)
45-62: LGTM! Constructor properly initializes credentials and normalizes endpoint.The endpoint normalization ensures HTTPS is always used, and the HTTP Basic Auth credentials are stored for use in API requests.
311-340: Cache key mismatch with Cache class implementation.The code builds a composite cache key (
$domain . '_' . $regType . '_' . $periodYears) but theCacheclass internally prefixes withdomain:and uses the provided key directly. This works, but differs from how other adapters (OpenSRS, Mock) use just the domain as the key. This could lead to cache fragmentation where the same domain has multiple cache entries for different regTypes/periods.Consider whether this granular caching is intentional or if it should match the simpler approach used by other adapters.
558-610: LGTM! Robust HTTP transport implementation.The
send()method properly:
- Uses configured timeouts (
connectTimeout,timeout)- Enables SSL verification
- Handles JSON encoding/decoding errors
- Maps HTTP error codes to appropriate exceptions (including
AuthExceptionfor 401)
618-652: LGTM! Contact sanitization handles multiple input formats.The method correctly handles:
- Single contact (applied to all types)
- Numerically-indexed arrays (mapped by position)
- Associative arrays (mapped by key, with 'owner' → 'registrant' translation)
src/Domains/Registrar.php (3)
16-19: LGTM on constant value changes.The registration type constants are now self-contained literal strings instead of delegating to the adapter. This simplifies the dependency chain and makes the
Registrarclass more independent.
138-148: LGTM!The new
updateNameserversmethod correctly delegates to the adapter and follows the established pattern in this class.
200-222: LGTM on new methods.Both
cancelPurchase()andcheckTransferStatus()follow the established delegation pattern and are properly documented.tests/Registrar/OpenSRSTest.php (1)
13-18: LGTM on class structure refactoring.The test class properly extends
Baseand maintains both the direct adapter reference andRegistrarwrapper instances, enabling comprehensive testing at both levels.tests/Registrar/MockTest.php (5)
14-18: LGTM on class structure.The test class properly extends
Baseand maintains appropriate property declarations for theRegistrarwrapper pattern.
45-51: Side effect ingetTestDomain()may affect test isolation.This method purchases a domain as a side effect each time it's called. If
getTestDomain()is called multiple times within the same test or across tests (beforetearDownresets the adapter), it could lead to unexpected state or domain conflicts.Consider documenting this behavior or ensuring the base class only calls this method once per test.
68-78: LGTM!The test properly verifies that
purchase()returns a non-empty string order ID when nameservers are provided.
80-91: LGTM!The test properly verifies that
transfer()returns a non-empty string order ID when nameservers are provided.
159-165: No issues found with the assertion.The
TransferStatusclass has astatusproperty explicitly typed asTransferStatusEnum(as shown in the constructor at line 22). The assertionassertInstanceOf(\Utopia\Domains\Registrar\TransferStatusEnum::class, $result->status)correctly verifies this type.Likely an incorrect or invalid review comment.
ChiragAgg5k
force-pushed
the
feat-add-namecom-adapter
branch
from
bf2efb8 to
68d836e
Compare
ChiragAgg5k
force-pushed
the
feat-add-namecom-adapter
branch
from
68d836e to
a9e9a82
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/Domains/Registrar/Adapter/OpenSRS.php (3)
167-184:purchase()returning$result['id']can silently return empty string—fail fast if missing.
If the API returns success but omitsid, the caller gets''with no signal. Consider throwing whensuccessful=truebutidis empty (or always require it).Proposed fix
- $result = $this->register($domain, $regType, $this->user, $contacts, $nameservers, $periodYears); - $result = $this->response($result); - return $result['id']; + $result = $this->register($domain, $regType, $this->user, $contacts, $nameservers, $periodYears); + $result = $this->response($result); + if (empty($result['id'])) { + throw new DomainsException('Failed to purchase domain: missing order id in response'); + } + return $result['id'];
201-218: Same concern as purchase:transfer()should not return empty order id.
Mirror the same “require id” guard to avoid silent failures.
452-499: Cache key ingetPrice()ignoresregTypeandperiodYears(can return wrong prices).
If cache is enabled, a prior call fornew/1ycan be incorrectly returned forrenewal/2y, etc. Consider including those in the cache key.Proposed fix
- if ($this->cache) { - $cached = $this->cache->load($domain, $ttl); + $cacheKey = $domain . '_' . $regType . '_' . $periodYears; + if ($this->cache) { + $cached = $this->cache->load($cacheKey, $ttl); if ($cached !== null && is_array($cached)) { return $cached['price']; } } ... - if ($this->cache) { - $this->cache->save($domain, [ + if ($this->cache) { + $this->cache->save($cacheKey, [ 'price' => $result, ]); }src/Domains/Registrar/Adapter/Mock.php (3)
134-145: PHPMD unused params inpurchase()are expected (interface), but add an explicit “intentional” note.
This keeps the mock aligned with the interface while making static analysis noise easier to justify.
259-306:getPrice()cache key ignoresregTypeandperiodYears(wrong cached results).
Same fix pattern as OpenSRS: include both in the cache key.
367-379: PHPMD unused params intransfer()are expected, but consider documenting as interface-required.
🤖 Fix all issues with AI agents
In @src/Domains/Registrar/Adapter/Mock.php:
- Around line 484-507: updateNameservers currently always returns success;
change it to mirror the other mock methods by first checking the mock registry
for the domain (use the same existence check used by
getDomain()/renew()/updateDomain()), return a failure response if the domain is
not present (e.g. ['successful' => false, 'error' => 'domain not purchased' or
similar]), and when present update the stored nameservers in the registry entry
before returning ['successful' => true, 'nameservers' => $nameservers].
In @src/Domains/Registrar/Adapter/NameCom.php:
- Around line 617-655: sanitizeContacts can raise a TypeError because
formatContact requires a Contact; validate inputs up-front by (1) checking the
$contacts array is non-empty (keep the existing InvalidContactException), (2)
iterating every item and throwing InvalidContactException if an item is not an
instance of Contact (include which key/index in the message), (3) after mapping
(single, numeric or associative) ensure all required types
['registrant','admin','tech','billing'] are present and non-null and throw
InvalidContactException if any are missing; update sanitizeContacts to perform
these validations before calling formatContact and reference sanitizeContacts,
formatContact, and InvalidContactException in your changes.
- Around line 124-160: In the NameCom::purchase method, cast the API response
order to a string before returning it: replace the raw return of
$result['order'] with an explicit string cast so the method honors its declared
string return type; locate this in the purchase(...) function and change the
final return to return (string)$result['order'] (ensuring you reference
$result['order'] in the NameCom class).
- Around line 311-358: getPrice() in NameCom::getPrice currently always uses the
checkAvailability response (single-year purchasePrice) and ignores the
periodYears and regType parameters; update the method to call Name.com's Get
Pricing endpoint (instead of or in addition to
/core/v1/domains:checkAvailability) and pass the years query/body parameter set
to $periodYears and include/translate $regType (new/renew/transfer) into the
appropriate endpoint/parameter per Name.com API (use different endpoint or
request flag when regType != Registrar::REG_TYPE_NEW). Keep existing cacheKey
logic, store the price returned for the specific period/regType, and retain the
same exception handling (throw PriceNotFoundException when the pricing response
lacks the expected price).
🧹 Nitpick comments (4)
src/Domains/Registrar/Adapter/OpenSRS.php (2)
54-75: Endpoint normalization looks correct, but confirm property promotion matches intended visibility/overrides.
You’re promoting$endpointtoprotected, so subclasses can mutate it; if that’s not intended, considerprivate+ accessor.
100-130:updateNameservers()response now echoes requested nameservers—ensure callers don’t treat it as “applied”.
Right now it returns the input$nameserverseven if OpenSRS normalizes/partially applies; consider returning “requestedNameservers” vs “appliedNameservers” (when available from response).src/Domains/Registrar/Adapter/NameCom.php (2)
96-113:updateNameservers()swallows exceptions and returns{successful:false}; ensure this matches adapter contract expectations.
If other adapters are expected to throw on failure, this will be inconsistent for callers (and can hide auth/validation failures).
220-225:cancelPurchase()always returning true can mislead callers into thinking cancellation happened.
Consider returningfalse(not supported) or throwing a dedicated “unsupported” exception if the broader API tolerates it.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Domains/Registrar/Adapter/Mock.phpsrc/Domains/Registrar/Adapter/NameCom.phpsrc/Domains/Registrar/Adapter/OpenSRS.phpsrc/Domains/Registrar/Renewal.phptests/Registrar/Base.php
💤 Files with no reviewable changes (1)
- src/Domains/Registrar/Renewal.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Registrar/Base.php
🧰 Additional context used
🪛 PHPMD (2.15.0)
src/Domains/Registrar/Adapter/NameCom.php
495-495: Avoid unused parameters such as '$checkStatus'. (undefined)
(UnusedFormalParameter)
495-495: Avoid unused parameters such as '$getRequestAddress'. (undefined)
(UnusedFormalParameter)
src/Domains/Registrar/Adapter/Mock.php
134-134: Avoid unused parameters such as '$periodYears'. (undefined)
(UnusedFormalParameter)
134-134: Avoid unused parameters such as '$nameservers'. (undefined)
(UnusedFormalParameter)
367-367: Avoid unused parameters such as '$authCode'. (undefined)
(UnusedFormalParameter)
367-367: Avoid unused parameters such as '$periodYears'. (undefined)
(UnusedFormalParameter)
367-367: Avoid unused parameters such as '$nameservers'. (undefined)
(UnusedFormalParameter)
491-491: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (10)
src/Domains/Registrar/Adapter/OpenSRS.php (1)
21-21: Good move: centralize reg-type constants viaUtopia\Domains\Registrar.
Helps keep adapters consistent as the API evolves.src/Domains/Registrar/Adapter/Mock.php (2)
16-16: Registrar constant usage is consistent with the refactor.
88-102: Constructor change looks fine (defaultPrice still configurable).src/Domains/Registrar/Adapter/NameCom.php (7)
45-63: Constructor + endpoint normalization is correct. The default endpoint (https://api.name.com) matches Name.com's v4 production API URL, and the code correctly accepts custom endpoints (e.g.,https://api.dev.name.comfor sandbox). HTTP Basic authentication with username and token is properly parameterized in the constructor.
557-609: Error parsing is complete per Name.com API v4 specification.The Name.com API v4 official documentation confirms error responses contain only
message(required) anddetails(optional) fields—noerrorsarray format. The current code correctly handles both fields with the fallback pattern at line 575, and the HTTP code is already included in the exception being thrown.Likely an incorrect or invalid review comment.
377-397: The field names (createDate,expireDate,autorenewEnabled,nameservers) correctly match the Name.com API v4 get domain endpoint response schema. No changes needed.
443-462:renew()response shape is correct.Verification of the Name.com API v4 renew domain endpoint confirms that the response includes
order(numeric) anddomain.expireDate(ISO datetime string) fields as accessed in the code.
470-485: The response keyauthCodeis correct per Name.com API documentation. Verify the exact core/v1 endpoint path/core/v1/domains/{domain}:getAuthCodewith Name.com's current API documentation, as search results only document the legacy v4 endpoint format.
80-87:available()implementation correctly accesses the Name.com API response structure.The implementation properly uses
results[0].purchasableto determine domain availability, which matches the CheckAvailability endpoint response schema that returns an array of SearchResult objects with a purchasable boolean field.
238-300: Fix field mapping and remove unsupportedlimitparameter; add suggestion prioritization.Response field names are incorrect:
purchasableshould beavailablepurchasePriceshould bepriceThe
limitparameter is not supported by the Core API endpoint; remove it from the request payload. The local limit logic (breaking whencount >= $limit) remains valid for client-side limiting.Additionally, when a limit is applied, suggestion domains should be prioritized and returned first, with premium domains only included if remaining capacity exists (consistent with the
suggest()implementation pattern).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/Domains/Registrar/Adapter/NameCom.php:
- Around line 507-541: In checkTransferStatus, document why the interface
parameters $checkStatus and $getRequestAddress are unused (add a comment in the
method and/or mark them with /** @param bool $checkStatus unused for NameCom API
*/ and/or use (void)$checkStatus; (void)$getRequestAddress;) so static analysis
warnings are addressed; and change the fallback behavior that currently returns
TransferStatusEnum::Transferrable when no transfer is found—either return a more
neutral enum value like TransferStatusEnum::Unknown/NotInitiated (add that enum
member if missing) or return TransferStatus with status set to a new
Neutral/NotInitiated value and null reason/timestamp, updating any callers or
mapping logic accordingly (refer to checkTransferStatus, TransferStatus, and
TransferStatusEnum).
🧹 Nitpick comments (3)
src/Domains/Registrar/Adapter/NameCom.php (3)
95-112: Consider throwing exceptions instead of returning error arrays.The method catches exceptions and returns an error object instead of propagating the exception. This pattern can lead to silent failures if callers don't check the
successfulfield. Consider either:
- Throwing the exception to maintain consistency with other methods like
getDomain,updateDomain, etc.- Documenting this behavior clearly in the return type.
♻️ Suggested refactor to throw exception
public function updateNameservers(string $domain, array $nameservers): array { - try { - $result = $this->send('POST', '/core/v1/domains/' . $domain . ':setNameservers', [ - 'nameservers' => $nameservers, - ]); - - return [ - 'successful' => true, - 'nameservers' => $result['nameservers'] ?? $nameservers, - ]; - } catch (Exception $e) { - return [ - 'successful' => false, - 'error' => $e->getMessage(), - ]; - } + $result = $this->send('POST', '/core/v1/domains/' . $domain . ':setNameservers', [ + 'nameservers' => $nameservers, + ]); + + return [ + 'successful' => true, + 'nameservers' => $result['nameservers'] ?? $nameservers, + ]; }
175-175: Redundant conditional check for nameservers.Line 175 already ensures
$nameserversis non-empty by falling back to$this->defaultNameservers. The check on line 186 will always be true (assumingdefaultNameserversis non-empty), making the conditional redundant.♻️ Simplify by removing redundant check
$data = [ 'domainName' => $domain, 'authCode' => $authCode, 'years' => $periodYears, 'contacts' => $contactData, + 'nameservers' => $nameservers, ]; - if (!empty($nameservers)) { - $data['nameservers'] = $nameservers; - } - $result = $this->send('POST', '/core/v1/transfers', $data);Also applies to: 186-188
377-381: Stub returns empty array - document behavior.The method returns an empty array with a comment explaining why. Consider adding
@return array<empty>or noting in the PHPDoc that this adapter doesn't support TLD enumeration, so callers know to expect an empty result.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Domains/Registrar/Adapter/NameCom.phpsrc/Domains/Registrar/Exception/DomainNotAvailableException.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Domains/Registrar/Exception/DomainNotAvailableException.php (1)
src/Domains/Exception.php (1)
Exception(5-7)
src/Domains/Registrar/Adapter/NameCom.php (10)
src/Domains/Registrar.php (16)
Registrar(11-223)__construct(32-51)getName(58-61)available(69-72)updateNameservers(145-148)purchase(83-86)transfer(185-188)cancelPurchase(206-209)suggest(99-102)tlds(109-112)getPrice(159-162)getDomain(120-123)updateDomain(133-136)renew(171-174)getAuthCode(196-199)checkTransferStatus(219-222)src/Domains/Registrar/Adapter.php (15)
Adapter(9-208)getName(80-80)available(88-88)updateNameservers(147-150)purchase(99-99)transfer(182-182)cancelPurchase(207-207)suggest(112-112)tlds(119-119)getPrice(161-161)getDomain(127-127)updateDomain(137-137)renew(170-170)getAuthCode(190-190)checkTransferStatus(200-200)src/Domains/Registrar/Contact.php (2)
Contact(5-44)toArray(24-43)src/Domains/Registrar/Exception/DomainTakenException.php (1)
DomainTakenException(7-9)src/Domains/Registrar/Exception/DomainNotTransferableException.php (1)
DomainNotTransferableException(7-9)src/Domains/Registrar/Exception/InvalidContactException.php (1)
InvalidContactException(7-9)src/Domains/Registrar/Exception/AuthException.php (1)
AuthException(7-9)src/Domains/Registrar/Exception/PriceNotFoundException.php (1)
PriceNotFoundException(7-9)src/Domains/Registrar/Exception/DomainNotAvailableException.php (1)
DomainNotAvailableException(7-9)src/Domains/Registrar/TransferStatus.php (1)
TransferStatus(19-27)
🪛 GitHub Actions: Tests
src/Domains/Registrar/Adapter/NameCom.php
[error] 368-368: Utopia\Domains\Exception: Failed to get price for domain: Not Found
[error] 617-617: Caused by: Exception: Not Found
[error] 81-81: Test assertion failed: Expected PriceNotFoundException but got Exception"None of the submitted domains are valid" (NameCom.php:81)
🪛 PHPMD (2.15.0)
src/Domains/Registrar/Adapter/NameCom.php
507-507: Avoid unused parameters such as '$checkStatus'. (undefined)
(UnusedFormalParameter)
507-507: Avoid unused parameters such as '$getRequestAddress'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (10)
src/Domains/Registrar/Exception/DomainNotAvailableException.php (1)
1-9: LGTM!Clean implementation of a marker exception following PSR-4 namespace conventions. Extending the project's base
Utopia\Domains\Exceptionmaintains consistency with the exception hierarchy.src/Domains/Registrar/Adapter/NameCom.php (9)
44-61: LGTM - Constructor with endpoint normalization.The constructor properly normalizes the endpoint to HTTPS, which is a good security practice. The configuration inherits
$this->headers,$this->connectTimeout,$this->timeout, and$this->defaultNameserversfrom the baseAdapterclass.
79-86: LGTM!The availability check correctly extracts the
purchasableflag with a safe fallback tofalse.
123-159: LGTM - Purchase implementation with comprehensive error handling.The method properly handles contact normalization, uses default nameservers as fallback, and maps API errors to domain-specific exceptions (DomainTakenException, InvalidContactException).
225-230: No-op implementation - verify this meets requirements.The method always returns
truewithout performing any action. The comment explains Name.com doesn't have a direct equivalent. Consider whether aNotImplementedExceptionwould be more appropriate to signal unsupported functionality, or document this behavior in the class-level PHPDoc.
243-305: LGTM - Domain suggestion with client-side filtering.The implementation properly handles array-to-string query conversion, TLD normalization (stripping leading dots), and applies price/type filters client-side. The double limit check (API + local) ensures exact result count.
389-446: LGTM - Domain retrieval and update methods.Both methods properly construct objects from API responses and handle exceptions appropriately. The
updateDomainmethod efficiently skips the API call when there's nothing to update.
455-497: LGTM - Renewal and auth code retrieval.Both methods have appropriate implementations with proper exception handling. The
getAuthCodemethod correctly distinguishes between domain-specific exceptions and general errors.
569-621: LGTM - HTTP transport with proper security settings.The
sendmethod properly:
- Enables SSL verification (
CURLOPT_SSL_VERIFYPEERandCURLOPT_SSL_VERIFYHOST)- Uses HTTP Basic Auth
- Handles JSON encoding/decoding errors
- Maps 401 responses to
AuthException- Cleans up curl resources in all code paths
629-667: LGTM - Contact sanitization with flexible input handling.The method handles three input formats gracefully:
- Single contact → applied to all roles
- Numeric array → positional mapping to roles
- Associative array → direct role mapping with
owner→registrantaliasThe empty check on line 631 prevents edge cases with the numeric array detection.
ChiragAgg5k
force-pushed
the
feat-add-namecom-adapter
branch
from
377b4a0 to
123dbc1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/Domains/Registrar/Adapter/NameCom.php:
- Around line 316-370: In getPrice(), remove the preflight available() call (so
callers can fetch prices for unavailable/owned domains) and change the error
handling so HTTP 404 responses map to PriceNotFoundException: in the
catch(Exception $e) block inspect the HTTP status/code or the exception message
from send() (check for status 404 or "Not Found" in
$e->getCode()/$e->getMessage()) and throw new PriceNotFoundException(...,
$e->getCode(), $e) instead of wrapping as DomainsException; also update the
check that currently compares against ERROR_MESSAGE_PRICE_NOT_FOUND to recognize
"Not Found" or a 404 code so PriceNotFoundException is consistently thrown by
getPrice().
🧹 Nitpick comments (4)
src/Domains/Registrar/Adapter/OpenSRS.php (1)
16-21: Unused import:Registrationclass is no longer used.The
Registrationimport on line 16 is now dead code sincepurchase()andtransfer()methods were refactored to returnstringorder IDs instead ofRegistrationobjects.🧹 Suggested cleanup
use Utopia\Domains\Registrar\Adapter; -use Utopia\Domains\Registrar\Registration; use Utopia\Domains\Registrar\Renewal;src/Domains/Registrar/Adapter.php (1)
147-150: Default implementation throws generic\Exception- consider using a more specific exception type.The base implementation throws a generic
\Exception. For consistency with the rest of the codebase's exception hierarchy, consider usingDomainsExceptionor a dedicatedNotImplementedException.The static analysis warning about unused parameters is a false positive here — the parameters are intentionally present to define the method contract for subclasses.
🧹 Suggested improvement
+use Utopia\Domains\Exception as DomainsException; + public function updateNameservers(string $domain, array $nameservers): array { - throw new \Exception('Method not implemented'); + throw new DomainsException('Method not implemented', 501); }src/Domains/Registrar/Adapter/NameCom.php (2)
220-230: Consider documenting the behavioral difference more explicitly.The
cancelPurchase()always returnstruesince Name.com doesn't have an equivalent bulk cancel endpoint. While the docblock mentions this, callers might assume the operation actually did something. Consider whether this should throw a "not supported" exception instead, or at minimum, log a warning.
346-355: Missing return statement for unhandled$regTypevalues.The switch statement handles
REG_TYPE_NEW,REG_TYPE_RENEWAL, andREG_TYPE_TRANSFER, but falls through to throwPriceNotFoundExceptionfor other values likeREG_TYPE_TRADE. This is correct behavior, but consider adding adefaultcase for clarity.🧹 Suggested improvement
switch ($regType) { case Registrar::REG_TYPE_NEW: return $purchasePrice; case Registrar::REG_TYPE_RENEWAL: return $renewalPrice; case Registrar::REG_TYPE_TRANSFER: return $transferPrice; + default: + throw new PriceNotFoundException('Unsupported registration type: ' . $regType, 400); } - - throw new PriceNotFoundException('Price not found for domain: ' . $domain, 400);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Domains/Registrar/Adapter.phpsrc/Domains/Registrar/Adapter/NameCom.phpsrc/Domains/Registrar/Adapter/OpenSRS.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Domains/Registrar/Adapter/OpenSRS.php (3)
src/Domains/Registrar/Adapter.php (4)
purchase(99-99)transfer(182-182)getPrice(161-161)checkTransferStatus(198-198)src/Domains/Registrar/Adapter/NameCom.php (4)
purchase(123-159)transfer(171-218)getPrice(316-370)checkTransferStatus(509-535)src/Domains/Registrar/TransferStatus.php (1)
TransferStatus(19-27)
src/Domains/Registrar/Adapter.php (3)
src/Domains/Cache.php (1)
Cache(7-35)src/Domains/Registrar.php (10)
Registrar(11-223)getName(58-61)available(69-72)purchase(83-86)tlds(109-112)getDomain(120-123)updateNameservers(145-148)getPrice(159-162)renew(171-174)transfer(185-188)src/Domains/Registrar/Adapter/Mock.php (9)
getName(76-79)available(110-121)purchase(134-145)tlds(219-222)getDomain(231-247)updateNameservers(491-497)getPrice(259-306)renew(316-330)transfer(367-379)
🪛 GitHub Actions: Linter
src/Domains/Registrar/Adapter/NameCom.php
[error] 1-1: PSR-12 linting failed: no_trailing_whitespace_in_comments (Pint). 1 style issue found in 27 files. Run './vendor/bin/pint' to fix.
🪛 GitHub Actions: Tests
src/Domains/Registrar/Adapter/NameCom.php
[error] 368-368: Utopia\Domains\Exception: Failed to get price for domain: Not Found
[error] 618-618: Caused by: Exception: Not Found
[error] 332-332: Caused by: Exception: Not Found
[error] 368-368: Utopia\Domains\Exception: Failed to get price for domain: Not Found
[error] 618-618: Caused by: Exception: Not Found
[error] 332-332: Caused by: Exception: Not Found
[error] 618-618: Failed asserting that exception of type 'Exception' matches expected 'Utopia\Domains\Registrar\Exception\PriceNotFoundException'. Message was: 'None of the submitted domains are valid'
[error] 81-81: Invalid domain price lookup path during invalid-domain test
[error] 326-326: Invalid-domain test path: expected PriceNotFoundException but got None of the submitted domains are valid
🪛 PHPMD (2.15.0)
src/Domains/Registrar/Adapter.php
147-147: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
147-147: Avoid unused parameters such as '$nameservers'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (26)
src/Domains/Registrar/Adapter/OpenSRS.php (6)
54-75: LGTM!Constructor simplification looks good. The endpoint normalization logic correctly handles various input formats (http://, https://, or bare hostname), and the default endpoint is appropriate for the OpenSRS sandbox/testing environment.
100-130: LGTM!The
updateNameserversmethod now includesnameserversin the return array, aligning with the Mock adapter's return structure and providing the caller with confirmation of the nameservers that were set.
167-199: LGTM!The
purchase()method correctly:
- Uses
Registrar::REG_TYPE_NEWconstant for consistency- Returns the order ID string from
$result['id']- Maintains comprehensive exception handling for various error codes
201-234: LGTM!The
transfer()method correctly:
- Uses
Registrar::REG_TYPE_TRANSFERconstant- Returns the order ID string
- Properly handles various transfer-related exceptions
452-499: LGTM!The
getPrice()signature update to useRegistrar::REG_TYPE_NEWas the default aligns with the centralized constant definitions in the Registrar class.
730-798: LGTM!The
checkTransferStatus()simplification is cleaner:
- Signature now accepts only the domain (consistent with base Adapter contract)
- Hard-coded
check_status => 1andget_request_address => 0simplify usage- Status mapping to
TransferStatusEnumis comprehensivesrc/Domains/Registrar/Adapter.php (5)
6-7: LGTM!New imports for
CacheandRegistrarare required for the new properties and constant references.
11-73: LGTM!The new configuration properties and setters provide a clean pattern for adapter configuration:
- Properties have sensible defaults (
$connectTimeout = 5,$timeout = 10)- Setters allow the
Registrarfacade to configure adapters post-construction- This aligns well with the
Registrarconstructor pattern shown in the relevant snippets
161-161: LGTM!The
getPrice()signature correctly usesRegistrar::REG_TYPE_NEWas the default, centralizing the constant definition.
182-182: LGTM!The
transfer()return type changed tostringfor order ID, consistent withpurchase().
198-205: LGTM!
checkTransferStatus()simplified to domain-only parameter- New
cancelPurchase()abstract method completes the domain lifecycle operationssrc/Domains/Registrar/Adapter/NameCom.php (15)
1-21: LGTM!Clean namespace setup and comprehensive imports covering all required exception types and domain objects.
22-61: LGTM!Well-structured constructor with:
- Clear parameter documentation
- Endpoint normalization consistent with OpenSRS adapter
- Appropriate default endpoint for Name.com API
63-86: LGTM!The
getName()andavailable()implementations are clean and properly use the Name.com checkAvailability endpoint.
88-112: LGTM!The
updateNameservers()implementation properly:
- Catches exceptions and returns error details instead of throwing
- Returns consistent structure with
successfulboolean andnameserversarray- Aligns with the Mock adapter's return format
114-159: LGTM!The
purchase()method correctly:
- Handles both single Contact and array of contacts
- Falls back to defaultNameservers
- Maps exceptions to domain-specific exception types
- Re-throws AuthException without wrapping
161-218: LGTM!The
transfer()method has comprehensive error handling covering:
- Domain not transferable (by message or HTTP 409)
- Invalid contact (by message or HTTP 422)
- Domain already in account
- AuthException passthrough
232-305: LGTM!The
suggest()method properly:
- Handles both string and array query inputs
- Applies TLD, limit, price range, and filter type filters client-side
- Returns consistent structure matching OpenSRS adapter
372-381: LGTM!The
tlds()returning an empty array is consistent with the OpenSRS adapter which also lacks a TLD listing endpoint.
383-409: LGTM!The
getDomain()method properly extracts domain information from the API response and constructs the Domain object with appropriate null handling for optional fields.
411-446: LGTM!The
updateDomain()method correctly:
- Combines contacts and autorenew updates into a single PATCH request
- Skips the API call if there's nothing to update
- Properly sanitizes contact data
448-476: LGTM!The
renew()method correctly returns aRenewalobject with order ID and new expiration date.
478-501: LGTM!The
getAuthCode()method properly handles the auth code retrieval and throws appropriate exceptions.
503-560: LGTM!The
checkTransferStatus()method:
- Handles 404 as "transferrable" (no transfer in progress) — sensible interpretation
- Maps Name.com status strings to
TransferStatusEnumcomprehensively- The status mapping covers all documented Name.com statuses
562-622: LGTM!The
send()method implements robust HTTP transport:
- Uses HTTP Basic Auth correctly
- Configures SSL verification properly
- Uses the configurable timeout values from the base adapter
- Handles JSON encoding/decoding errors
- Properly distinguishes 401 Unauthorized for
AuthException
624-693: LGTM!The contact sanitization logic handles three input patterns well:
- Single contact → replicate for all types
- Numerically-indexed array → map by position with fallback
- Associative array → direct mapping with 'owner' → 'registrant' translation
The
formatContact()method correctly maps the internal Contact fields to Name.com's expected API field names.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @src/Domains/Registrar/Adapter/NameCom.php:
- Around line 357-369: The catch block currently only re-throws
PriceNotFoundException but omits DomainNotAvailableException; if you move the
available() check inside the try in NameCom.php (e.g., within the method that
calls available()), detect when $e is a DomainNotAvailableException and re-throw
it similarly to PriceNotFoundException before mapping other exceptions: add an
instanceof check for DomainNotAvailableException and throw a new
DomainNotAvailableException($message, $e->getCode(), $e) (or re-throw $e
directly), keeping the existing string-based check for
ERROR_MESSAGE_PRICE_NOT_FOUND and the final throw of DomainsException for all
other exceptions.
- Around line 346-355: The switch on $regType in NameCom.php omits
Registrar::REG_TYPE_TRADE so trade requests fall through to the generic
PriceNotFoundException; add an explicit case for Registrar::REG_TYPE_TRADE
inside the switch (e.g., case Registrar::REG_TYPE_TRADE:) that either returns
the appropriate trade price variable if supported or throws a
PriceNotFoundException with a clear message like "Trade pricing not supported
for domain: {domain}", referencing $domain, and keep the existing final throw as
a safety net.
- Around line 316-370: The availability check call to available() in getPrice()
can throw exceptions that bypass the method's try/catch logic; move the call to
available($domain) inside the existing try block (or wrap it in its own
try/catch) so any errors from available() are handled the same way as send()
errors, preserve the existing DomainNotAvailableException behavior (throw when
available() returns false), and ensure caching logic and subsequent error-type
checks (e.g., comparing against self::ERROR_MESSAGE_PRICE_NOT_FOUND) still run
inside the same try/catch scope in getPrice().
- Around line 79-86: The available() call can throw from send() and currently
runs before getPrice()'s try-catch; move the available($domain) invocation into
the existing try block inside getPrice() (or alternatively wrap the call in a
local try-catch) so any Exception from send() is caught by getPrice()'s catch
handler; ensure you reference the available() method and the getPrice() method
and do not allow raw Exceptions from send() to propagate uncaught.
🧹 Nitpick comments (2)
src/Domains/Registrar/Adapter.php (1)
37-73: Consider adding validation for timeout setters.The setter methods are straightforward, but the timeout setters could benefit from validation to prevent invalid values (e.g., negative timeouts).
♻️ Optional: Add validation for timeout values
public function setConnectTimeout(int $connectTimeout): void { + if ($connectTimeout < 0) { + throw new \InvalidArgumentException('Connect timeout must be non-negative'); + } $this->connectTimeout = $connectTimeout; } public function setTimeout(int $timeout): void { + if ($timeout < 0) { + throw new \InvalidArgumentException('Timeout must be non-negative'); + } $this->timeout = $timeout; }src/Domains/Registrar/Adapter/NameCom.php (1)
225-230: Document the limitation more explicitly.The method always returns
truebut doesn't actually cancel anything. Consider throwing\BadMethodCallExceptionor documenting this limitation more prominently if the Name.com API truly doesn't support this operation.♻️ Optional: Consider throwing or logging for unsupported operation
public function cancelPurchase(): bool { // Name.com doesn't have a direct equivalent to OpenSRS's cancel pending orders // Transfers can be cancelled individually using the CancelTransfer endpoint + // Note: This is a no-op. Callers should be aware that pending orders are not cancelled. return true; }Alternatively, if this operation should fail explicitly:
public function cancelPurchase(): bool { throw new \BadMethodCallException('Name.com does not support bulk cancellation of pending orders'); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Domains/Registrar/Adapter.phpsrc/Domains/Registrar/Adapter/NameCom.phpsrc/Domains/Registrar/Adapter/OpenSRS.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Domains/Registrar/Adapter/NameCom.php (4)
src/Domains/Registrar/Adapter.php (15)
Adapter(9-206)getName(80-80)available(88-88)updateNameservers(147-150)purchase(99-99)transfer(182-182)cancelPurchase(205-205)suggest(112-112)tlds(119-119)getPrice(161-161)getDomain(127-127)updateDomain(137-137)renew(170-170)getAuthCode(190-190)checkTransferStatus(198-198)src/Domains/Registrar/Contact.php (2)
Contact(5-44)toArray(24-43)src/Domains/Registrar/TransferStatus.php (1)
TransferStatus(19-27)src/Domains/Cache.php (2)
load(21-24)save(26-29)
src/Domains/Registrar/Adapter.php (6)
src/Domains/Cache.php (1)
Cache(7-35)src/Domains/Registrar.php (1)
Registrar(11-223)src/Domains/Registrar/Contact.php (1)
Contact(5-44)src/Domains/Registrar/Domain.php (1)
Domain(7-17)src/Domains/Registrar/Renewal.php (1)
Renewal(7-14)src/Domains/Registrar/TransferStatus.php (1)
TransferStatus(19-27)
🪛 GitHub Actions: Linter
src/Domains/Registrar/Adapter/NameCom.php
[error] 1-1: PSR-12: no_trailing_whitespace_in_comme… (lint issue reported by PHP Pint).
🪛 GitHub Actions: Tests
src/Domains/Registrar/Adapter/NameCom.php
[error] 365-365: Utopia\Domains\Registrar\Exception\PriceNotFoundException: Failed to get price for domain: Not Found
[error] 618-618: Caused by: Exception: Not Found
[error] 332-332: Caused by: Not Found (Price retrieval path encountered an exception)
[error] 81-81: Failed asserting that exception of type "Exception" matches expected exception "Utopia\Domains\Registrar\Exception\PriceNotFoundException". Message was: "None of the submitted domains are valid"
🪛 PHPMD (2.15.0)
src/Domains/Registrar/Adapter.php
147-147: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
147-147: Avoid unused parameters such as '$nameservers'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (21)
src/Domains/Registrar/Adapter/OpenSRS.php (7)
21-21: LGTM!The
Registrarimport is correctly added to support the use ofRegistrar::REG_TYPE_*constants throughout the class.
54-58: LGTM!The constructor signature is simplified to remove
$defaultNameserversand$cacheparameters, which are now managed via setters in the baseAdapterclass. This is a cleaner approach that separates construction from configuration.
128-128: LGTM!Including the
nameserversin the return array provides useful feedback to callers about what nameservers were set.
167-183: LGTM!The
purchase()method correctly updated to:
- Use
Registrar::REG_TYPE_NEWconstant- Return the order ID as a
stringinstead of aRegistrationobjectThis aligns with the new Adapter interface contract.
201-217: LGTM!The
transfer()method correctly updated to:
- Use
Registrar::REG_TYPE_TRANSFERconstant- Return the order ID as a
stringinstead of aRegistrationobjectThis aligns with the new Adapter interface contract.
452-452: LGTM!The
getPrice()signature correctly usesRegistrar::REG_TYPE_NEWas the default for$regType, aligning with the centralized constants in theRegistrarclass.
730-740: LGTM!The
checkTransferStatus()method is simplified to always check status (check_status => 1) and never get the request address (get_request_address => 0). This is a sensible default behavior for transfer status checks.src/Domains/Registrar/Adapter.php (4)
6-7: LGTM!The imports for
CacheandRegistrarare correctly added to support the new configuration properties and constants.
11-29: LGTM!The protected properties provide sensible defaults:
- Empty default nameservers array
- No cache by default
- 5-second connection timeout
- 10-second request timeout
These defaults are reasonable for domain registrar API interactions.
147-150: Default implementation intentionally throws — consider documenting the override pattern.This is a valid "optional override" pattern where the base class provides a default that throws, allowing concrete adapters to optionally implement the method. The PHPMD warnings about unused parameters are false positives in this context.
Consider adding a note in the docblock that concrete adapters should override this method.
99-99: LGTM!The abstract method signatures are well-defined with clear return types:
purchase()andtransfer()returnstring(order ID)getPrice()usesRegistrar::REG_TYPE_NEWas defaultcheckTransferStatus()returnsTransferStatuscancelPurchase()returnsboolThese align with the PR objectives for consistent API surface across adapters.
Also applies to: 161-161, 182-182, 198-198, 205-205
src/Domains/Registrar/Adapter/NameCom.php (10)
1-21: LGTM!The imports are comprehensive and well-organized, covering all necessary exception types, domain objects, and the Registrar constants.
44-61: LGTM!The constructor properly:
- Stores credentials securely
- Normalizes the endpoint to always use HTTPS
- Sets appropriate JSON content-type headers
95-112: LGTM!The
updateNameserversmethod properly implements the abstract method from the base class, returning a structured array with success status and either the nameservers or an error message.
123-159: LGTM!The
purchasemethod:
- Properly handles both single Contact and array of Contacts
- Falls back to default nameservers when none provided
- Maps exceptions to domain-specific exception types
- Re-throws
AuthExceptionwithout wrapping
171-218: LGTM!The
transfermethod has comprehensive error handling that maps HTTP status codes and error messages to appropriate domain-specific exceptions.
509-535: LGTM!The
checkTransferStatusmethod properly:
- Uses the single-domain lookup endpoint
- Maps Name.com statuses to the internal
TransferStatusEnum- Handles 404 as "Transferrable" (no transfer initiated)
- Wraps other exceptions appropriately
549-560: LGTM!The status mapping is comprehensive and handles all documented Name.com transfer statuses appropriately.
570-622: LGTM!The
sendmethod properly:
- Uses HTTP Basic Auth via
CURLOPT_USERPWD- Sets connection and request timeouts from class properties
- Enforces SSL verification
- Handles JSON encoding/decoding errors
- Maps 401 to
AuthException
630-668: LGTM!The
sanitizeContactsmethod handles three input formats correctly:
- Single contact → replicate to all types
- Numerically-indexed array → map by position
- Associative array → map by key with
owner→registranttranslation
676-693: LGTM!The
formatContactmethod correctly maps the internal Contact structure to Name.com's expected API format with proper field name translations (e.g.,postalcode→zip).
ChiragAgg5k
force-pushed
the
feat-add-namecom-adapter
branch
from
2a2dcf8 to
0dac67b
Compare
ChiragAgg5k
force-pushed
the
feat-add-namecom-adapter
branch
from
0dac67b to
e56c8b9
Compare
ChiragAgg5k
force-pushed
the
feat-add-namecom-adapter
branch
from
a7e0eaa to
6375cbf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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.php (1)
75-87: Docblock parameter order doesn't match method signature.The docblock shows
@param int $periodYearsbefore@param array|Contact $contacts, but the actual method signature has$contactsbefore$periodYears.📝 Proposed fix
/** * Purchase a domain * * @param string $domain - * @param int $periodYears * @param array|Contact $contacts + * @param int $periodYears * @param array $nameservers * @return string Order ID */
🤖 Fix all issues with AI agents
In @tests/Registrar/NameComTest.php:
- Around line 36-51: Both registrars are sharing $this->adapter so the second
Registrar construction (which calls setCache on the adapter) mutates the shared
adapter and causes $this->registrar to unexpectedly use the cache; fix this by
creating a separate adapter instance for the cached registrar (e.g., instantiate
a new adapter variable like $this->adapterWithCache) and pass that to the
Registrar constructor for $this->registrarWithCache instead of reusing
$this->adapter so the uncached and cached registrars have independent adapter
state.
🧹 Nitpick comments (18)
src/Domains/Registrar/Adapter/Mock/UpdateDetails.php (1)
28-30: Consider serializing Contact objects for consistency.When
contactsis aContactobject (or array ofContactobjects), it's stored directly in the result without callingtoArray(). Other adapters typically serialize contacts to arrays. For a Mock adapter this may be intentional for flexibility, but could cause type inconsistencies if consumers expect array format.♻️ Optional: Serialize contacts
if ($this->contacts !== null) { - $result['contacts'] = $this->contacts; + if ($this->contacts instanceof Contact) { + $result['contacts'] = $this->contacts->toArray(); + } elseif (is_array($this->contacts)) { + $result['contacts'] = array_map(fn($c) => $c instanceof Contact ? $c->toArray() : $c, $this->contacts); + } else { + $result['contacts'] = $this->contacts; + } }tests/Registrar/NameComTest.php (3)
27-28: Consider usingmarkTestSkippedfor missing environment variables.Using
assertNotEmptyinsetUp()will fail the test with an assertion error, which may be confusing. UsingmarkTestSkipped()is more appropriate for missing configuration.♻️ Proposed fix
- $this->assertNotEmpty($username, 'NAMECOM_USERNAME environment variable must be set'); - $this->assertNotEmpty($token, 'NAMECOM_TOKEN environment variable must be set'); + if (empty($username) || empty($token)) { + $this->markTestSkipped('NAMECOM_USERNAME and NAMECOM_TOKEN environment variables must be set'); + }
86-92: Unused$contactsparameter is intentional but undocumented.The
$contactsparameter is unused because Name.com's UpdateDetails doesn't support contact updates. This is correct since the method signature must match the abstract parent, but consider adding a brief comment for clarity.protected function getUpdateDetails(array $details = [], array|Contact|null $contacts = null): UpdateDetails { + // Note: Name.com UpdateDetails doesn't support contact updates, so $contacts is ignored $autorenewEnabled = $details['autorenew'] ?? null;
139-145: Unused loop variable$domain.The
$domainkey variable in the foreach loops is unused. Consider prefixing with underscore to indicate intentional non-use, or usearray_values()if keys aren't needed.- foreach ($result as $domain => $data) { + foreach ($result as $_domain => $data) {Also applies to: 159-161
src/Domains/Registrar/Adapter/OpenSRS/UpdateDetails.php (1)
6-6: Unused import:Contactis imported but not used in this class.The
Contacttype is only referenced in the docblock annotation but never used in the actual code. Consider removing it to keep imports clean.♻️ Proposed fix
namespace Utopia\Domains\Registrar\Adapter\OpenSRS; use Utopia\Domains\Registrar\UpdateDetails as BaseUpdateDetails; -use Utopia\Domains\Registrar\Contact; class UpdateDetails extends BaseUpdateDetailssrc/Domains/Registrar/Adapter.php (1)
59-73: Consider adding validation for timeout values.The timeout setters accept any integer, including negative values. While this is unlikely to be an issue in practice, adding a guard clause would prevent misconfiguration.
♻️ Proposed fix
public function setConnectTimeout(int $connectTimeout): void { + if ($connectTimeout < 0) { + throw new \InvalidArgumentException('Connect timeout must be non-negative'); + } $this->connectTimeout = $connectTimeout; } public function setTimeout(int $timeout): void { + if ($timeout < 0) { + throw new \InvalidArgumentException('Timeout must be non-negative'); + } $this->timeout = $timeout; }src/Domains/Registrar/Adapter/Mock.php (1)
292-297: Consider explicitly handlingREG_TYPE_NEWin the match expression.The
defaultcase handlesREG_TYPE_NEW, but explicitly listing it would improve code clarity and self-documentation.♻️ Proposed fix
$multiplier = match ($regType) { + Registrar::REG_TYPE_NEW => 1.0, Registrar::REG_TYPE_TRANSFER => 1.0, Registrar::REG_TYPE_RENEWAL => 1.1, Registrar::REG_TYPE_TRADE => 1.2, - default => 1.0, + default => 1.0, // Fallback for unknown types };src/Domains/Registrar/Adapter/NameCom.php (8)
186-199: Redundant nameserver check.The condition at lines 197-199 is redundant. Line 186 already assigns
$this->defaultNameserverswhen$nameserversis empty, so the check at line 197 will always be true unless$this->defaultNameserversis also empty.♻️ Suggested simplification
$nameservers = empty($nameservers) ? $this->defaultNameservers : $nameservers; $contactData = $this->sanitizeContacts($contacts); $data = [ 'domainName' => $domain, 'authCode' => $authCode, 'years' => $periodYears, 'contacts' => $contactData, + 'nameservers' => $nameservers, ]; - if (!empty($nameservers)) { - $data['nameservers'] = $nameservers; - } - $result = $this->send('POST', '/core/v1/transfers', $data);
234-239: Stub implementation returns unconditional success.This always returns
trueregardless of context. Consider whether callers might rely on this to actually cancel operations. The comment is helpful but consumers of the API may not be aware of this limitation.
350-359: Consider adding explicit default case to switch.The switch statement falls through to the exception at line 359 for unknown
$regTypevalues, but adding an explicitdefaultcase would make the intent clearer and prevent accidental fallthrough if new cases are added.♻️ Suggested improvement
switch ($regType) { case Registrar::REG_TYPE_NEW: return $purchasePrice; case Registrar::REG_TYPE_RENEWAL: return $renewalPrice; case Registrar::REG_TYPE_TRANSFER: return $transferPrice; + default: + throw new PriceNotFoundException('Price not found for domain: ' . $domain, 400); } - - throw new PriceNotFoundException('Price not found for domain: ' . $domain, 400);
384-388: Empty TLD list may surprise consumers.Returning an empty array is acceptable given the API limitation, but ensure this is documented for consumers who might expect a list of supported TLDs.
413-416: Consider throwing DomainNotFoundException for 404 errors.Unlike
checkTransferStatus(line 534-536), this method doesn't check for 404 errors to throw aDomainNotFoundException. This inconsistency may confuse callers who expect uniform exception handling across domain operations.♻️ Suggested improvement
} catch (Exception $e) { + if ($e->getCode() === 404) { + throw new DomainNotFoundException('Domain not found: ' . $domain, $e->getCode(), $e); + } throw new DomainsException('Failed to get domain information: ' . $e->getMessage(), $e->getCode(), $e); }
506-506: HTTP 404 code for missing auth code may be misleading.Using code 404 when the auth code is not found in the response implies an HTTP "not found" error, but this is actually a malformed response. Consider using a more appropriate error code or a custom code.
526-526: Use null coalescing for consistency.Line 526 uses
isset()with ternary while the rest of the code uses null coalescing (??). For consistency:♻️ Suggested change
- $reason = isset($result['statusDetails']) ? $result['statusDetails'] : null; + $reason = $result['statusDetails'] ?? null;
620-622: Minor formatting issue in error message.Missing space before the parenthesis when appending details:
♻️ Suggested fix
if ($details) { - $message .= '(' . $details . ')'; + $message .= ' (' . $details . ')'; }tests/Registrar/Base.php (3)
293-294: Unnecessary assertion on DateTime.
assertNotEmpty($result->expiresAt)afterassertInstanceOf(\DateTime::class, ...)is redundant. A valid DateTime instance is never considered "empty".♻️ Suggested fix
$this->assertInstanceOf(\DateTime::class, $result->expiresAt); - $this->assertNotEmpty($result->expiresAt);
301-313: Exception assertion could mask unexpected failures.The catch block asserts the exception type, but if a different exception is thrown (e.g.,
AuthException), the test fails with a confusing message. Consider using a union type check or catching the specific exception type.♻️ Suggested improvement
public function testTransfer(): void { $domain = $this->generateRandomString() . '.' . $this->getDefaultTld(); try { $result = $this->getRegistrar()->transfer($domain, 'test-auth-code', $this->getPurchaseContact()); $this->assertIsString($result); $this->assertNotEmpty($result); - } catch (\Exception $e) { - $this->assertInstanceOf(DomainNotTransferableException::class, $e); + } catch (DomainNotTransferableException $e) { + // Expected for random domains that aren't registered elsewhere + $this->assertNotEmpty($e->getMessage()); } }
342-351: Enum assertion is redundant.Since
$result->statusis typed asTransferStatusEnum, it will always be one of the enum values. This assertion is effectively testing PHP's type system rather than the code.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/Domains/Registrar.phpsrc/Domains/Registrar/Adapter.phpsrc/Domains/Registrar/Adapter/Mock.phpsrc/Domains/Registrar/Adapter/Mock/UpdateDetails.phpsrc/Domains/Registrar/Adapter/NameCom.phpsrc/Domains/Registrar/Adapter/NameCom/UpdateDetails.phpsrc/Domains/Registrar/Adapter/OpenSRS.phpsrc/Domains/Registrar/Adapter/OpenSRS/UpdateDetails.phpsrc/Domains/Registrar/Exception/DomainNotFoundException.phpsrc/Domains/Registrar/UpdateDetails.phptests/Registrar/Base.phptests/Registrar/MockTest.phptests/Registrar/NameComTest.phptests/Registrar/OpenSRSTest.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-05T13:46:00.588Z
Learnt from: ChiragAgg5k
Repo: utopia-php/domains PR: 44
File: src/Domains/Registrar/OpenSRS.php:292-438
Timestamp: 2025-10-05T13:46:00.588Z
Learning: In the OpenSRS `suggest()` method, non-premium (suggestion) domains should be prioritized and shown first. When a limit is applied, the limit should be filled with suggestion domains first, and premium domains should only be included if there's remaining capacity after processing suggestions.
Applied to files:
tests/Registrar/OpenSRSTest.php
🧬 Code graph analysis (9)
src/Domains/Registrar/UpdateDetails.php (1)
src/Domains/Registrar.php (1)
Registrar(12-221)
src/Domains/Registrar/Exception/DomainNotFoundException.php (1)
src/Domains/Exception.php (1)
Exception(5-7)
tests/Registrar/NameComTest.php (4)
src/Domains/Registrar/Exception/AuthException.php (1)
AuthException(7-9)src/Domains/Registrar/Adapter/NameCom.php (3)
NameCom(23-708)purchase(134-170)suggest(252-314)src/Domains/Registrar/Adapter/NameCom/UpdateDetails.php (1)
UpdateDetails(7-39)tests/Registrar/Base.php (10)
Base(15-365)getRegistrar(20-20)getRegistrarWithCache(25-25)getTestDomain(31-31)generateRandomString(78-89)getPurchaseContact(50-73)getExpectedAdapterName(36-36)getDefaultNameservers(358-364)getUpdateDetails(45-45)getPricingTestDomain(103-106)
src/Domains/Registrar/Adapter/NameCom/UpdateDetails.php (1)
src/Domains/Registrar/Adapter/NameCom.php (1)
NameCom(23-708)
src/Domains/Registrar/Adapter/OpenSRS/UpdateDetails.php (3)
src/Domains/Registrar/Adapter/Mock/UpdateDetails.php (3)
UpdateDetails(8-34)__construct(14-18)toArray(20-33)src/Domains/Registrar/Adapter/NameCom/UpdateDetails.php (3)
UpdateDetails(7-39)__construct(14-19)toArray(21-38)src/Domains/Registrar/Contact.php (1)
Contact(5-44)
src/Domains/Registrar/Adapter/Mock.php (5)
src/Domains/Registrar/Adapter/Mock/UpdateDetails.php (2)
UpdateDetails(8-34)toArray(20-33)src/Domains/Registrar/Adapter/OpenSRS/UpdateDetails.php (2)
UpdateDetails(8-41)toArray(24-40)src/Domains/Registrar/Adapter.php (7)
purchase(99-99)getPrice(160-160)updateDomain(136-136)transfer(181-181)checkTransferStatus(197-197)updateNameservers(146-149)cancelPurchase(204-204)src/Domains/Registrar/Contact.php (2)
Contact(5-44)toArray(24-43)src/Domains/Registrar/TransferStatus.php (1)
TransferStatus(19-27)
tests/Registrar/OpenSRSTest.php (5)
src/Domains/Registrar/Exception/AuthException.php (1)
AuthException(7-9)src/Domains/Registrar/Exception/DomainNotTransferableException.php (1)
DomainNotTransferableException(7-9)src/Domains/Registrar/Adapter/OpenSRS.php (2)
OpenSRS(22-1164)purchase(174-206)src/Domains/Registrar/Adapter/OpenSRS/UpdateDetails.php (1)
UpdateDetails(8-41)src/Domains/Registrar/Adapter/Mock.php (1)
purchase(135-146)
src/Domains/Registrar/Adapter/OpenSRS.php (4)
src/Domains/Registrar.php (5)
Registrar(12-221)purchase(84-87)transfer(185-188)updateDomain(133-136)checkTransferStatus(217-220)src/Domains/Registrar/UpdateDetails.php (2)
UpdateDetails(5-13)toArray(12-12)src/Domains/Registrar/Adapter.php (4)
purchase(99-99)transfer(181-181)updateDomain(136-136)checkTransferStatus(197-197)src/Domains/Registrar/Contact.php (2)
Contact(5-44)toArray(24-43)
src/Domains/Registrar.php (4)
src/Domains/Registrar/TransferStatus.php (2)
TransferStatus(19-27)__construct(21-26)src/Domains/Registrar/Adapter/Mock/UpdateDetails.php (2)
UpdateDetails(8-34)__construct(14-18)src/Domains/Registrar/Domain.php (1)
__construct(9-16)src/Domains/Registrar/Contact.php (2)
__construct(7-22)Contact(5-44)
🪛 PHPMD (2.15.0)
tests/Registrar/NameComTest.php
86-86: Avoid unused parameters such as '$contacts'. (undefined)
(UnusedFormalParameter)
139-139: Avoid unused local variables such as '$domain'. (undefined)
(UnusedLocalVariable)
159-159: Avoid unused local variables such as '$domain'. (undefined)
(UnusedLocalVariable)
src/Domains/Registrar/Adapter/Mock.php
135-135: Avoid unused parameters such as '$periodYears'. (undefined)
(UnusedFormalParameter)
135-135: Avoid unused parameters such as '$nameservers'. (undefined)
(UnusedFormalParameter)
371-371: Avoid unused parameters such as '$authCode'. (undefined)
(UnusedFormalParameter)
371-371: Avoid unused parameters such as '$periodYears'. (undefined)
(UnusedFormalParameter)
371-371: Avoid unused parameters such as '$nameservers'. (undefined)
(UnusedFormalParameter)
493-493: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
src/Domains/Registrar/Adapter.php
146-146: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
146-146: Avoid unused parameters such as '$nameservers'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (43)
src/Domains/Registrar/Exception/DomainNotFoundException.php (1)
1-9: LGTM!Clean exception class following the established project pattern. The marker exception approach is appropriate for distinguishing domain-not-found scenarios in error handling.
src/Domains/Registrar/UpdateDetails.php (1)
1-13: LGTM!Well-designed abstract class that provides a clean contract for adapter-specific update details. The minimal interface with
toArray()gives each adapter flexibility to serialize its own fields.src/Domains/Registrar/Adapter/NameCom/UpdateDetails.php (1)
1-39: LGTM!Clean implementation with constructor property promotion. The conditional inclusion of only non-null fields in
toArray()correctly supports partial updates, and the field names align with Name.com API v4 expectations.tests/Registrar/NameComTest.php (2)
64-71: Domain purchased on every call - consider cost in non-sandbox environments.This method purchases a new domain each time it's called. This is fine for sandbox testing but ensure this test class only runs against the sandbox endpoint to avoid costs.
164-167: LGTM!Good practice to use
markTestSkipped()with an explanatory message rather than commenting out or deleting the test.src/Domains/Registrar/Adapter/OpenSRS/UpdateDetails.php (1)
8-41: LGTM!The OpenSRS-specific UpdateDetails implementation is well-structured. The design correctly separates the
contactshandling (which is processed separately inupdateDomain) from the serialized payload, aligning with the OpenSRS API requirements.src/Domains/Registrar/Adapter/OpenSRS.php (7)
33-40: LGTM!The contact type constants improve maintainability and reduce magic strings throughout the codebase. Good addition.
61-82: LGTM!Constructor simplification aligns with the new architecture where default nameservers and cache are configured via the
Registrarfacade using setter methods on the baseAdapterclass.
174-206: LGTM!The
purchasemethod correctly returns the order ID as a string, aligning with the updatedAdapterinterface. The use ofRegistrar::REG_TYPE_NEWconstant is consistent with the centralized constant definition.
208-241: LGTM!The
transfermethod signature and implementation are consistent with thepurchasemethod changes and the updatedAdapterinterface.
606-647: LGTM!The
updateDomainmethod properly validates theUpdateDetailstype and correctly separates contact handling from the main payload. The type check ensures adapter-specific details are used.
744-812: LGTM!The
checkTransferStatusmethod is well-implemented with proper response mapping toTransferStatusEnum. Thematchexpression provides clear status translation.
1146-1163: LGTM!The
sanitizeContactsmethod now uses the defined contact type constants, improving code maintainability and reducing magic strings.src/Domains/Registrar/Adapter.php (2)
146-149: The unused parameters are expected for this default stub implementation.Static analysis flagged
$domainand$nameserversas unused, but this is intentional. This is a template method pattern where the base class provides a default "not implemented" behavior, and subclasses (OpenSRS, Mock, NameCom) override it with actual implementations that use these parameters.
99-99: LGTM!The updated abstract method signatures establish a clean contract for all registrar adapters. Return types changed from
Registrationtostring(order ID) simplify the API.src/Domains/Registrar.php (3)
17-20: LGTM!Centralizing the registration type constants in the
Registrarfacade class provides a single source of truth. Adapters correctly reference these viaRegistrar::REG_TYPE_*.
33-52: LGTM!The constructor properly initializes the adapter with configuration. The conditional checks for
defaultNameserversandcacheprevent unnecessary overwrites, while timeouts are consistently applied.
200-220: LGTM!The
cancelPurchaseandcheckTransferStatusfacade methods properly delegate to the adapter and maintain type safety withTransferStatusreturn type.src/Domains/Registrar/Adapter/Mock.php (6)
89-103: LGTM!Constructor simplification aligns with the base
Adapterclass changes. Cache is now managed via the inheritedsetCache()method rather than constructor injection.
135-146: Unused parameters are acceptable for this mock implementation.Static analysis flagged
$periodYearsand$nameserversas unused. This is intentional for a mock adapter that provides minimal implementation for testing while maintaining interface compliance.
342-357: Consider adding type validation forMock\UpdateDetailsfor consistency with OpenSRS adapter.The OpenSRS adapter validates that the passed
UpdateDetailsis specifically anOpenSRS\UpdateDetailsinstance. The Mock adapter accepts anyUpdateDetails, which may be intentional for flexibility in testing, but could lead to unexpected behavior if a different adapter'sUpdateDetailsis passed.If this flexibility is intentional for testing purposes, the current implementation is acceptable. Otherwise, consider adding:
if (!$details instanceof Mock\UpdateDetails) { throw new Exception("Invalid details type: expected Mock\\UpdateDetails"); }
371-383: LGTM!The
transfermethod correctly returns an order ID string. Unused parameters ($authCode,$periodYears,$nameservers) are acceptable for this mock implementation that maintains interface compliance.
493-509: LGTM!The
updateNameserversandcancelPurchasemethods provide minimal mock implementations suitable for testing. The unused$domainparameter inupdateNameserversis acceptable for maintaining interface compliance.
463-484: LGTM!The
checkTransferStatusmethod provides appropriate mock behavior for the different domain states, enabling comprehensive testing of transfer status scenarios.src/Domains/Registrar/Adapter/NameCom.php (7)
55-72: LGTM on constructor with endpoint normalization.The constructor properly normalizes endpoints to HTTPS and sets up HTTP Basic Auth headers. Good defensive handling for different endpoint formats.
90-97: LGTM on availability check.Clean implementation with safe array access using null coalescing.
106-123: LGTM on nameserver update.The error handling returns a structured result with success status and error message, which is appropriate for this operation.
252-314: LGTM on suggest implementation.Good approach to apply price and type filters client-side after the API call, with the limit re-enforced after filtering. This correctly handles the case where API-side limit is applied before client-side filters.
437-457: LGTM on updateDomain implementation.Good validation to ensure at least one update field is provided before making the API call.
468-487: LGTM on renew implementation.Consistent with the purchase pattern. The nested access for expire date is safely handled with null coalescing.
150-152: No action needed. The response keyorderis correct according to Name.com API v4 documentation. The Create Domain endpoint returnsorder(integer), notorderId. The code at line 152 is using the correct key.Likely an incorrect or invalid review comment.
tests/Registrar/OpenSRSTest.php (5)
15-54: LGTM on test class refactoring.Clean migration to the new Base class pattern. The abstract methods are properly implemented, and the setup correctly initializes the adapter and registrar instances.
89-93: LGTM on UpdateDetails factory.Correctly creates OpenSRS-specific UpdateDetails with the required
datafield.
97-117: LGTM on authentication failure test.Good coverage for the invalid password scenario.
119-172: LGTM on suggestion tests.Good coverage for multiple keywords and premium filtering with price bounds. Based on learnings, suggestion domains are correctly expected to be prioritized.
174-198: LGTM on transfer tests.The try-catch pattern is appropriate for live API tests where the outcome may vary. Both success and expected exception paths are validated.
tests/Registrar/Base.php (4)
15-46: LGTM on abstract test base class design.Good abstraction with clear abstract methods that child classes must implement. This pattern reduces code duplication across adapter tests.
50-73: LGTM on contact helper.Flexible helper with suffix support for creating distinct test contacts.
78-89: LGTM on random string generation.Good use of
random_int()for secure randomness. Lowercase-only is appropriate for domain names.
130-137: Live domain purchases in tests may incur costs.This test actually purchases domains when run against live registrar APIs. Ensure this is intended behavior and that tests use sandbox endpoints to avoid billing.
tests/Registrar/MockTest.php (3)
15-29: LGTM on MockTest setup.Clean setup with proper initialization of adapter and both registrar variants (with and without cache).
46-52: LGTM on mock test domain generation.Purchasing on-the-fly is appropriate for the mock adapter since no real purchases occur.
74-170: LGTM on mock-specific tests.Good coverage of edge cases including nameserver handling, transfer conflicts, and invalid contact validation.
Summary
This PR adds a complete Name.com domain registrar adapter to the library, expanding support beyond OpenSRS.
Changes
✅ New NameCom Adapter (
src/Domains/Registrar/Adapter/NameCom.php)✅ Comprehensive Test Suite (
tests/Registrar/NameComTest.php)✅ Updated GitHub Actions Workflow (
.github/workflows/test.yml)NAMECOM_USERNAMEandNAMECOM_TOKENenvironment variables✅ Updated Documentation (
README.md)Implemented Methods
All required adapter methods are fully implemented:
getName()- Returns 'namecom'available()- Check domain availabilitypurchase()- Register new domainstransfer()- Transfer domains with auth codesrenew()- Renew existing domainsgetDomain()- Get domain informationupdateDomain()- Update domain detailsupdateNameservers()- Update nameserverssuggest()- Domain name suggestionsgetPrice()- Get domain pricingtlds()- List available TLDsgetAuthCode()- Retrieve EPP auth codescheckTransferStatus()- Check transfer statusAPI Details
https://api.name.comhttps://api.dev.name.comTesting
To run the tests locally, set these environment variables:
Additional Notes
php -lRequirements
Don't forget to add these secrets to GitHub Actions:
NAMECOM_USERNAMENAMECOM_TOKENTest results:
Summary by CodeRabbit
Release Notes
New Features
Documentation
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.