Conversation
WalkthroughMoved top-level RDAP conformance out of Common into a new TopMostEntityTrait; Domain adopts the trait and adds redaction rules. Removed rdapConformance from Common and renamed getStatuses() → getStatus(). Introduced EventAction::REGISTRAR_EXPIRATION, a domain VCard class and updated Entity vcard API, Link fallbacks, Notice constructor/type handling, SecureDNS nullability, serializer interface/signature and Symfony implementation, plus updated fixtures and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Test/Caller
participant Domain
participant TopTrait as TopMostEntityTrait
participant Entity
participant VCard
participant Serializer
participant VcardNorm as VcardNormalizer
Note over Domain,TopTrait: rdapConformance now provided by trait
Test->>Domain: getRdapConformance()
Domain->>TopTrait: getRdapConformance()
TopTrait-->>Domain: rdapConformance array
Domain-->>Test: rdapConformance array
Note over Entity,VCard: VCard stored as domain object
Test->>Entity: addVcard(vcard)
Entity->>VCard: store vCard data
Entity-->>Test: updated entity
Note over Serializer,VcardNorm: serialization flow (new options param)
Test->>Serializer: serialize(domain, FORMAT_JSON, {options})
Serializer->>VcardNorm: normalize(VCard)
VcardNorm-->>Serializer: VCard.jsonSerialize()
Serializer-->>Test: JSON string
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (2)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Domain/Entity/Entity.php (1)
102-108: Fix inconsistent vCard storage behavior.The method has several issues:
- Lines 104-106 initialize
$this->vcardArrayas an empty array, but line 107 assigns a singleVCardobject (not an array).- The method name
addVcardimplies appending, but the implementation now overwrites any existing value.- The docblock at line 92 states
@return VCard[]|null, but the implementation stores a singleVCardobject.Apply this diff to fix the initialization logic:
public function addVcard(VCard $vcard): void { - if (empty($this->vcardArray)) { - $this->vcardArray = []; - } $this->vcardArray = $vcard; }Additionally, consider renaming the method to
setVcard()to reflect its overwriting behavior, and update the docblock to@return VCard|nullfor consistency.
🧹 Nitpick comments (1)
src/Domain/Entity/Domain.php (1)
214-214: Minor formatting change.An extra blank line was added before appending the entity. While this doesn't affect functionality, ensure it aligns with the project's code style guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Domain/Constant/EventAction.php(1 hunks)src/Domain/Entity/Common.php(2 hunks)src/Domain/Entity/Domain.php(3 hunks)src/Domain/Entity/Entity.php(2 hunks)src/Domain/ValueObject/Link.php(2 hunks)
🔇 Additional comments (3)
src/Domain/Entity/Domain.php (1)
230-230: Remove extra closing brace.There is an extra closing brace at line 230 after the class definition ends at line 231. This is a syntax error.
Apply this diff to remove the extra brace:
return $this; } - }Likely an incorrect or invalid review comment.
src/Domain/Entity/Common.php (1)
24-35: No compatibility issues found.The verification confirms that
DEFAULT_RDAP_CONFORMANCEhas only two references within the codebase: its definition and the property initialization at line 35, which correctly accepts the new array structure. The code change is compatible and safe.src/Domain/ValueObject/Link.php (1)
149-152: Update the docblock to match the non-nullable return type.The fallback behavior is intentional and safe—
hrefis required in the constructor andgetHref()always returns a non-null string. However, the docblock at line 147 incorrectly declares@return string|nullwhen the implementation now guarantees a non-nullstringreturn. Update the docblock to@return string.
src/Domain/Constant/EventAction.php
Outdated
| public const REREGISTRATION = 'reregistration'; | ||
| public const LAST_CHANGED = 'last changed'; | ||
| public const EXPIRATION = 'expiration'; | ||
| public const REFISTRAR_EXPIRATION = 'registrar expiration'; |
There was a problem hiding this comment.
Fix typo in constant name.
The constant name REFISTRAR_EXPIRATION has a typo—it should be REGISTRAR_EXPIRATION (missing 'G').
Apply this diff to fix the typo:
- public const REFISTRAR_EXPIRATION = 'registrar expiration';
+ public const REGISTRAR_EXPIRATION = 'registrar expiration';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public const REFISTRAR_EXPIRATION = 'registrar expiration'; | |
| public const REGISTRAR_EXPIRATION = 'registrar expiration'; |
🤖 Prompt for AI Agents
In src/Domain/Constant/EventAction.php around line 39, rename the constant
REFISTRAR_EXPIRATION to REGISTRAR_EXPIRATION to fix the typo; update all
references/usages throughout the codebase to use the new constant name and run
tests/grep to ensure no remaining references to the old name remain (the
constant value 'registrar expiration' can stay unchanged).
| if (empty($this->entities['entities'])) { | ||
| return $this->entities ?? []; | ||
| } | ||
|
|
||
| return $this->entities['entities']; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Clarify the nested entities structure logic.
The conditional logic checks for $this->entities['entities'], treating $this->entities as an associative array with an 'entities' key. However, the property is typed as Entity[]|null (line 54), and addEntity() appends Entity objects directly to the array (line 215).
This creates a type mismatch: addEntity() builds a flat array of Entity objects, but getEntities() expects a nested structure like ['entities' => [...]].
Run the following script to understand how entities are being stored:
🏁 Script executed:
#!/bin/bash
# Description: Search for code that assigns nested structure to entities property.
# Look for assignments to entities property with nested structure
rg -nP --type=php -C5 '\$this->entities\s*='
# Look for all calls to addEntity on Domain objects
rg -nP --type=php -C3 '->addEntity\('Length of output: 2287
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Get the complete getEntities() method in Domain.php
rg -nP --type=php -A10 'public function getEntities' src/Domain/Entity/Domain.php
# Check the property declaration and constructor initialization
rg -nP --type=php -B2 -A2 'private.*entities|protected.*entities|public.*entities' src/Domain/Entity/Domain.php
# Look for any array key assignment pattern to entities
rg -nP --type=php '\$this->entities\[.*\]\s*=' src/Domain/Entity/Domain.phpLength of output: 405
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Check getEntities() in Entity.php and IPNetwork.php
rg -nP --type=php -A10 'public function getEntities' src/Domain/Entity/Entity.php src/Domain/Entity/IPNetwork.phpLength of output: 836
Fix Domain.php getEntities() to access flat array structure.
The method tries to access $this->entities['entities'] (nested structure), but addEntity() creates a flat Entity[] array. This is inconsistent with how Entity.php and IPNetwork.php implement getEntities(), which both return the flat array directly.
Fix required at lines 126-131 in Domain.php:
public function getEntities(): ?array
{
return $this->entities;
}🤖 Prompt for AI Agents
In src/Domain/Entity/Domain.php around lines 126 to 131, getEntities()
incorrectly expects a nested ['entities'] key while addEntity() stores a flat
Entity[]; replace the current conditional and return with a direct return of
$this->entities (i.e., return $this->entities;), and ensure the method signature
remains public function getEntities(): ?array to match other implementations
returning a flat array.
| * @return VCard[]|null | ||
| */ | ||
| public function getVcardArray(): ?array | ||
| public function getVcardArray() | ||
| { | ||
| return $this->vcardArray; | ||
| } |
There was a problem hiding this comment.
Update docblock and consider removing misleading 'Array' suffix.
The docblock indicates @return VCard[]|null, but the implementation now stores a single VCard object instead of an array. The property and method names (vcardArray, getVcardArray) are misleading.
Consider these changes:
- Update the docblock to reflect the actual return type.
- Rename the property from
$vcardArrayto$vcard. - Rename the method from
getVcardArray()togetVcard().
If you need to maintain backward compatibility, at minimum update the docblock:
/**
- * @return VCard[]|null
+ * @return VCard|null
*/
public function getVcardArray()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * @return VCard[]|null | |
| */ | |
| public function getVcardArray(): ?array | |
| public function getVcardArray() | |
| { | |
| return $this->vcardArray; | |
| } | |
| /** | |
| * @return VCard|null | |
| */ | |
| public function getVcardArray() | |
| { | |
| return $this->vcardArray; | |
| } |
🤖 Prompt for AI Agents
In src/Domain/Entity/Entity.php around lines 92 to 97, the docblock and names
claim the value is an array (VCard[]|null) but the implementation stores a
single VCard; update the docblock to @return VCard|null, rename the private
property $vcardArray to $vcard and rename the getter getVcardArray() to
getVcard(), then update all internal references (constructors, setters,
serializers, tests) to use $vcard/getVcard(); if backward compatibility is
required, add a deprecated getVcardArray() that simply returns $this->vcard (or
null) and mark it @deprecated in the docblock so external callers continue to
work while moving to the new API.
| public function getType(): ?string | ||
| { | ||
| return $this->type; | ||
| return $this->type ?? 'text/html'; | ||
| } |
There was a problem hiding this comment.
Update return type annotation to match non-nullable behavior.
The method now always returns a string (defaulting to 'text/html' when $this->type is null), but the return type annotation is ?string. This is inconsistent.
Apply this diff to update the return type:
/**
- * @return string|null
+ * @return string
*/
- public function getType(): ?string
+ public function getType(): string
{
return $this->type ?? 'text/html';
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function getType(): ?string | |
| { | |
| return $this->type; | |
| return $this->type ?? 'text/html'; | |
| } | |
| /** | |
| * @return string | |
| */ | |
| public function getType(): string | |
| { | |
| return $this->type ?? 'text/html'; | |
| } |
🤖 Prompt for AI Agents
In src/Domain/ValueObject/Link.php around lines 87 to 90, the getType() method
always returns a string (defaulting to 'text/html') but its signature is
declared as ?string; update the method signature to return string (remove the
nullable ?), and update any PHPDoc/annotations for this method to reflect a
non-nullable string return type; keep the internal property nullable if needed
but ensure the public API and docs indicate a string is always returned.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/unit/Domain/Entity/CommonTest.php(1 hunks)tests/unit/Serialization/Symfony/stub/full_domain_info.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/Domain/Entity/CommonTest.php (1)
src/Domain/Entity/Common.php (1)
getStatus(150-153)
🔇 Additional comments (1)
tests/unit/Domain/Entity/CommonTest.php (1)
102-102: LGTM! Test correctly updated to match the API change.The test has been appropriately updated to call
getStatus()instead ofgetStatuses(), aligning with the method rename in theCommonclass.
…t` and applied it to relevant classes. Adjusted serialization logic and updated tests accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/Domain/Entity/Domain.php (1)
128-133: Fix the type mismatch in getEntities().The method attempts to access
$this->entities['entities'](nested structure), butaddEntity()at line 217 builds a flatEntity[]array. This creates a type inconsistency:
- Property declared as
Entity[]|null(line 54)addEntity()appends directly:$this->entities[] = $entitygetEntities()expects nested:$this->entities['entities']This will cause runtime errors when the nested key is accessed on a flat array.
Apply this diff to return the flat array structure consistently:
public function getEntities(): ?array { - if (empty($this->entities['entities'])) { - return $this->entities ?? []; - } - - return $this->entities['entities']; + return $this->entities; }tests/unit/Serialization/Symfony/stub/full_domain_info.json (1)
234-234: Typo in link title.Line 234 contains
"title": "new tittle"which appears to be a typo (should be "title" with one 't'). While the remarks section uses "tittle" consistently, the first link at line 228 uses the correct spelling"title": "title".Apply this diff to fix the typo:
{ "href": "google.com", "type": "plain\/text", - "title": "new tittle", + "title": "new title", "value": "google.com" }
🧹 Nitpick comments (2)
src/Domain/Entity/Domain.php (1)
216-216: Consider removing the unnecessary blank line.The blank line at line 216 doesn't add clarity and deviates from the consistent pattern used in other add methods within this class (e.g.,
addPublicId,addVariant,addNameserver).Apply this diff to maintain consistency:
public function addEntity(Entity $entity): Domain { if ($this->entities === null) { $this->entities = []; } - $this->entities[] = $entity; return $this; }src/Domain/Entity/Common.php (1)
130-133: Consider renaming to clarify array return type.The method name
getStatus()is singular but returnsStatus[]|null, which may mislead developers into expecting a singleStatusobject. Consider renaming togetStatuses()or document clearly that it returns an array.However, since the past review comment indicates tests were already updated to use
getStatus()in commit 7a2d07e, this appears to be an intentional API design decision.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Domain/Entity/Common.php(1 hunks)src/Domain/Entity/Domain.php(3 hunks)src/Domain/Entity/TopMostEntityTrait.php(1 hunks)src/Infrastructure/Serialization/Symfony/SymfonySerializer.php(1 hunks)tests/unit/Domain/Entity/CommonTest.php(1 hunks)tests/unit/Domain/Entity/DomainTest.php(1 hunks)tests/unit/Serialization/Symfony/stub/full_domain_info.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/Domain/Entity/CommonTest.php
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit/Domain/Entity/DomainTest.php (2)
src/Domain/Entity/Domain.php (1)
Domain(19-232)src/Domain/Entity/TopMostEntityTrait.php (1)
getRdapConformance(27-30)
src/Domain/Entity/TopMostEntityTrait.php (1)
src/Domain/Entity/Domain.php (1)
Domain(19-232)
🔇 Additional comments (5)
src/Infrastructure/Serialization/Symfony/SymfonySerializer.php (1)
55-55: LGTM! rdapConformance will now be serialized.The removal of
'rdapConformance'fromIGNORED_ATTRIBUTESaligns with the introduction ofTopMostEntityTrait, which manages the rdapConformance metadata. This change ensures rdapConformance is included in the serialized output as intended.tests/unit/Domain/Entity/DomainTest.php (1)
149-157: LGTM! Test validates TopMostEntityTrait integration.The test correctly verifies that
Domain::getRdapConformance()returns the expected default RDAP conformance values from the newly introducedTopMostEntityTrait.src/Domain/Entity/Domain.php (1)
21-21: LGTM! TopMostEntityTrait integration.The trait provides the necessary rdapConformance management for the Domain entity, centralizing RDAP metadata handling as intended by the refactoring.
tests/unit/Serialization/Symfony/stub/full_domain_info.json (1)
1-293: LGTM! Structural changes align with the refactoring.The changes reflect the updated RDAP structure:
- Pretty-printed format improves readability
statusfield (singular) replacesstatusesthroughoutrdapConformancearray added at top level (lines 288-292)These align with the introduction of
TopMostEntityTraitand the API rename inCommon.src/Domain/Entity/TopMostEntityTrait.php (1)
1-47: LGTM! Clean encapsulation of RDAP conformance metadata.The trait provides:
- Well-defined default conformance values per RDAP specifications
- Type-safe accessors with clear method signatures
- Proper encapsulation via private property
This centralizes RDAP conformance management and eliminates duplication across top-level entities.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Domain/Entity/Domain.php (1)
128-135: Return the stored entity list directly
$this->entitiesis populated via$this->entities[] = $entity;, so there is never an'entities'key. The new check always hits theempty()branch and, as a side effect, coerces the “no entities” case fromnullto[], changing the public API. This is the same mismatch we flagged earlier—please just return the flat array (ornull) thataddEntity()builds.A minimal fix:
- if (empty($this->entities['entities'])) { - return $this->entities ?? []; - } - - return $this->entities['entities']; + return $this->entities;tests/unit/Serialization/Symfony/stub/full_domain_info.json (1)
234-234: Typo remains: "tittle" should be "title".The typo "new tittle" should be "new title" for correct spelling.
Apply this diff to fix:
- "title": "new tittle", + "title": "new title",
🧹 Nitpick comments (3)
src/Domain/ValueObject/Notice.php (1)
23-50: Fix Notice docblocks to match the new APIAfter dropping
$typefrom the constructor and allowing it to be optional, the phpdoc still claims there’s a$typeparameter and even uses@var string||null. Please align the annotations so IDEs and static analysers stop warning on stale signatures.A minimal fix looks like:
- /** - * @var string||null - */ + /** + * @var string|null + */ private $type; ... - /** - * @param string $title - * @param string $type - * @param string[] $description - * @param Link[] $links - */ - public function __construct(string $title, array $description, array $links = []) + /** + * @param string $title + * @param string[] $description + * @param Link[] $links + */ + public function __construct(string $title, array $description, array $links = [])src/Domain/Entity/Entity.php (1)
92-109: Align vCard storage with the new single-object behavior
addVcard()now replaces the stored value with a singleVCard, yet the property name, docblock (@var VCard[]|null), and getter (getVcardArray) still advertise array semantics. That’s confusing for callers and tooling. Please either restore true array support or rename/update the property/getter toVCard|null(and drop the redundant empty-initialization).One way forward:
- /** - * @var VCard[]|null a jCard with the entity's VCard information - */ - private $vcardArray; + /** + * @var VCard|null a jCard with the entity's VCard information + */ + private $vcard; ... - public function getVcardArray() - { - return $this->vcardArray; - } + public function getVcard(): ?VCard + { + return $this->vcard; + } ... - if (empty($this->vcardArray)) { - $this->vcardArray = []; - } - - $this->vcardArray = $vcard; + $this->vcard = $vcard;If you need to keep
getVcardArray()for BC, mark it deprecated and have it return$this->vcard.src/Domain/Entity/TopMostEntityTrait.php (1)
37-39: Consider adding runtime validation for array elements.While the PHPDoc indicates
string[], PHP doesn't enforce typed array elements at runtime. If you want to be defensive, you could validate that all elements are strings to prevent downstream issues.Example validation approach:
public function setRdapConformance(array $rdapConformance): void { + foreach ($rdapConformance as $item) { + if (!is_string($item)) { + throw new \InvalidArgumentException('All rdapConformance items must be strings'); + } + } $this->rdapConformance = $rdapConformance; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/Domain/Entity/Domain.php(4 hunks)src/Domain/Entity/Entity.php(2 hunks)src/Domain/Entity/TopMostEntityTrait.php(1 hunks)src/Domain/ValueObject/Notice.php(3 hunks)src/Domain/ValueObject/SecureDNS.php(5 hunks)tests/unit/Domain/Entity/CommonTest.php(2 hunks)tests/unit/Domain/Entity/DomainTest.php(1 hunks)tests/unit/Domain/Entity/EntityTest.php(1 hunks)tests/unit/Serialization/Symfony/DomainSerializerTest.php(1 hunks)tests/unit/Serialization/Symfony/stub/full_domain_info.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/Domain/Entity/DomainTest.php
🧰 Additional context used
🧬 Code graph analysis (6)
src/Domain/Entity/TopMostEntityTrait.php (1)
src/Domain/Entity/Domain.php (1)
Domain(19-234)
tests/unit/Serialization/Symfony/DomainSerializerTest.php (2)
src/Domain/Entity/Common.php (1)
addRemark(167-175)src/Domain/ValueObject/Notice.php (2)
Notice(15-90)setType(68-73)
src/Domain/Entity/Domain.php (1)
src/Domain/ValueObject/SecureDNS.php (1)
SecureDNS(16-107)
tests/unit/Domain/Entity/CommonTest.php (2)
src/Domain/ValueObject/Notice.php (1)
Notice(15-90)src/Domain/Entity/Common.php (1)
getStatus(130-133)
tests/unit/Domain/Entity/EntityTest.php (1)
src/Domain/Entity/Entity.php (1)
getVcardArray(94-97)
src/Domain/ValueObject/Notice.php (2)
src/Domain/ValueObject/Link.php (3)
__construct(63-66)getType(87-90)setType(79-82)src/Infrastructure/Serialization/Symfony/SymfonySerializer.php (1)
__construct(39-73)
🔇 Additional comments (2)
src/Domain/Entity/TopMostEntityTrait.php (1)
1-49: LGTM! Clean trait implementation.The trait provides a clean abstraction for managing RDAP conformance metadata. The default values align with RDAP standards and test expectations, and the accessor methods follow standard patterns.
tests/unit/Serialization/Symfony/stub/full_domain_info.json (1)
288-294: LGTM! rdapConformance correctly added.The rdapConformance array has been properly added to the top-level domain structure with values that match the TopMostEntityTrait defaults and align with RDAP specifications.
| bool $delegationSigned = false, | ||
| ?bool $zoneSigned = null, | ||
| ?int $maxSigLife = null, | ||
| ?array $dsData = null, | ||
| ?array $keyData = null | ||
| ) { | ||
| $this->delegationSigned = $delegationSigned; | ||
| if ($delegationSigned === false) { | ||
| return ; | ||
| } | ||
| $this->zoneSigned = $zoneSigned; |
There was a problem hiding this comment.
Restore backward-compatible constructor semantics
Swapping the parameter order now treats the first argument as $delegationSigned, so existing calls like new SecureDNS(true, false, …) silently flip the meaning of the booleans: what used to be “zone signed = true, delegation signed = false” now becomes “delegation signed = true, zone signed = false”. The early return also prevents you from representing RFC 7483’s perfectly valid case of zoneSigned === true while delegationSigned === false, because the constructor bails out before $zoneSigned (and the rest) are stored. Please keep the original parameter order and drop the early return so all combinations remain expressible.
Apply this diff to keep compatibility and still allow nullable inputs:
- public function __construct(
- bool $delegationSigned = false,
- ?bool $zoneSigned = null,
- ?int $maxSigLife = null,
- ?array $dsData = null,
- ?array $keyData = null
- ) {
- $this->delegationSigned = $delegationSigned;
- if ($delegationSigned === false) {
- return ;
- }
- $this->zoneSigned = $zoneSigned;
- $this->maxSigLife = $maxSigLife;
- $this->dsData = $dsData;
- $this->keyData = $keyData;
- }
+ public function __construct(
+ ?bool $zoneSigned = null,
+ bool $delegationSigned = false,
+ ?int $maxSigLife = null,
+ ?array $dsData = null,
+ ?array $keyData = null
+ ) {
+ $this->zoneSigned = $zoneSigned;
+ $this->delegationSigned = $delegationSigned;
+ $this->maxSigLife = $maxSigLife;
+ $this->dsData = $dsData;
+ $this->keyData = $keyData;
+ }🤖 Prompt for AI Agents
In src/Domain/ValueObject/SecureDNS.php around lines 52 to 62, the constructor
parameter order was changed and an early return was added which flips boolean
meanings for existing callers and prevents valid combinations (e.g.
delegationSigned === false while zoneSigned === true) from being stored; restore
the original parameter order so the first boolean remains $zoneSigned then
$delegationSigned (preserving backward compatibility), remove the early return
that skips assigning the remaining properties, and ensure the constructor still
accepts nullable $zoneSigned, $maxSigLife, $dsData, and $keyData so all
combinations can be represented.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Domain/Entity/TopMostEntityTrait.php (1)
35-38: Consider adding array content validation.The
setRdapConformancemethod accepts any array without verifying that all elements are strings. While the PHPDoc indicatesstring[], there's no runtime enforcement.If you'd like to add validation, consider this approach:
public function setRdapConformance(array $rdapConformance): void { + foreach ($rdapConformance as $item) { + if (!is_string($item)) { + throw new \InvalidArgumentException('All rdapConformance items must be strings'); + } + } $this->rdapConformance = $rdapConformance; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Domain/Entity/TopMostEntityTrait.php(1 hunks)tests/unit/Domain/Entity/DomainTest.php(1 hunks)tests/unit/Serialization/Symfony/stub/full_domain_info.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Domain/Entity/TopMostEntityTrait.php (1)
src/Domain/Entity/Domain.php (1)
Domain(19-234)
tests/unit/Domain/Entity/DomainTest.php (2)
src/Domain/Entity/Domain.php (1)
Domain(19-234)src/Domain/Entity/TopMostEntityTrait.php (1)
getRdapConformance(27-30)
🔇 Additional comments (3)
tests/unit/Domain/Entity/DomainTest.php (1)
149-157: LGTM!The test correctly verifies that the
Domainentity exposes the RDAP conformance metadata via theTopMostEntityTrait. The expected conformance values match the trait's defaults.src/Domain/Entity/TopMostEntityTrait.php (1)
1-47: LGTM!The trait provides a clean, reusable way to manage RDAP conformance metadata. The default values are appropriate, and the accessor methods follow standard patterns.
tests/unit/Serialization/Symfony/stub/full_domain_info.json (1)
234-234: Verify typo resolution from previous review.A past review comment indicated that "new tittle" on this line was a typo and was addressed in commit 8c4312d, but the typo still appears in the current code. Please confirm whether this change was reverted or if the issue remains unresolved.
| "rdapConformance": [ | ||
| "rdap_level_0", | ||
| "icann_rdap_technical_implementation_guide_1", | ||
| "icann_rdap_response_profile_1" | ||
| ] |
There was a problem hiding this comment.
Fix indentation inconsistency.
The rdapConformance array uses inconsistent indentation. Lines 289-291 use 6 spaces, while the rest of the file uses 2-space indentation.
Apply this diff to fix the indentation:
"rdapConformance": [
- "rdap_level_0",
- "icann_rdap_technical_implementation_guide_1",
- "icann_rdap_response_profile_1"
+ "rdap_level_0",
+ "icann_rdap_technical_implementation_guide_1",
+ "icann_rdap_response_profile_1"
]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "rdapConformance": [ | |
| "rdap_level_0", | |
| "icann_rdap_technical_implementation_guide_1", | |
| "icann_rdap_response_profile_1" | |
| ] | |
| "rdapConformance": [ | |
| "rdap_level_0", | |
| "icann_rdap_technical_implementation_guide_1", | |
| "icann_rdap_response_profile_1" | |
| ] |
🤖 Prompt for AI Agents
In tests/unit/Serialization/Symfony/stub/full_domain_info.json around lines 288
to 292, the rdapConformance array has inconsistent indentation (lines 289-291
use 6 spaces) compared to the file's 2-space style; update those lines to use
2-space indentation so the opening key, each array element, and the closing
bracket align with the rest of the JSON file.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Domain/Entity/Entity.php (1)
94-97: The docblock and method naming issues remain unaddressed.The docblock still indicates
@return VCard[]|null, but based on the AI summary and past review feedback, the implementation now stores a singleVCardobject rather than an array. The method namegetVcardArray()and property name$vcardArraycontinue to be misleading.This echoes the previous review comment. Please address the naming and documentation concerns raised earlier.
🧹 Nitpick comments (3)
src/Domain/Entity/VCard.php (2)
15-18: Consider adding return type hint for jsonSerialize().The
jsonSerialize()method lacks an explicit return type. While not strictly required by theJsonSerializableinterface, adding: mixed(PHP 8.0+) or: arraywould improve clarity and type safety.Apply this diff to add the return type:
- public function jsonSerialize() + public function jsonSerialize(): mixed { return $this->vCard; }Alternatively, if the return value is always an array, use
: arrayfor stricter typing.
10-13: Consider adding validation for the vCard array structure.The constructor accepts any array without validation. If the vCard array must follow a specific structure (e.g., jCard format with required fields), consider adding validation to fail fast on invalid input.
This is optional and depends on whether you want strict validation at the domain level or allow flexibility for different vCard formats.
src/Infrastructure/Serialization/SerializerInterface.php (1)
24-33: Good API extension, but update the docblock.The addition of the
$targetOptionsparameter and explicit: stringreturn type improves the interface by allowing customizable serialization behavior and providing better type safety.However, the docblock at line 27 still states
@return mixed depending on $targetFormat, which contradicts the explicit: stringreturn type.Apply this diff to update the docblock:
/** * @param object $entity * @param string $targetFormat - * @return mixed depending on $targetFormat + * @param array $targetOptions + * @return string */ public function serialize(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Domain/Entity/Entity.php(1 hunks)src/Domain/Entity/VCard.php(1 hunks)src/Infrastructure/Serialization/SerializerInterface.php(1 hunks)src/Infrastructure/Serialization/Symfony/Normalizer/VcardNormalizer.php(2 hunks)src/Infrastructure/Serialization/Symfony/SymfonySerializer.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/Infrastructure/Serialization/SerializerInterface.php (1)
src/Infrastructure/Serialization/Symfony/SymfonySerializer.php (1)
serialize(75-81)
src/Infrastructure/Serialization/Symfony/SymfonySerializer.php (1)
src/Infrastructure/Serialization/SerializerInterface.php (2)
serialize(29-33)deserialize(41-41)
src/Domain/Entity/VCard.php (2)
src/Domain/Entity/Entity.php (2)
Entity(19-186)__construct(70-73)src/Infrastructure/Serialization/Symfony/SymfonySerializer.php (1)
__construct(39-73)
src/Infrastructure/Serialization/Symfony/Normalizer/VcardNormalizer.php (2)
src/Domain/Entity/Entity.php (1)
Entity(19-186)src/Domain/Entity/VCard.php (2)
VCard(5-19)jsonSerialize(15-18)
🔇 Additional comments (3)
src/Infrastructure/Serialization/Symfony/SymfonySerializer.php (2)
55-55: Good change to enable rdapConformance serialization.Changing
IGNORED_ATTRIBUTESfrom['rdapConformance']to[]ensures thatrdapConformancewill now be included in serialized output. This aligns with the PR objective to fix the RDAP structure.
75-80: Correctly implements the updated interface signature.The
serialize()method now properly accepts the$targetOptionsparameter and passes it to the underlying Symfony serializer, with an explicit: stringreturn type matching the interface.src/Infrastructure/Serialization/Symfony/Normalizer/VcardNormalizer.php (1)
15-40: No changes needed—serialization is correctly implemented and tested.The review comment's concern was valid: vcardArray is indeed initialized as
['vcard']with VCard objects appended. However, verification confirms the implementation handles this structure correctly:
- Symfony's serializer properly traverses the array: The string
'vcard'remains as-is, and each VCard object is normalized via VcardNormalizer- VcardNormalizer correctly delegates to jsonSerialize(): This returns the internal vCard array, producing the expected jCard format
- Test coverage validates the output: The serialization test asserts against a fixture file showing vcardArray correctly serializes to
["vcard", [vcard_data]], matching RFC 7095 formatThe refactoring simplifies the code while maintaining correctness.
| public function addVcard( $vcard): void | ||
| { | ||
| if (empty($this->vcardArray)) { | ||
| $this->vcardArray = []; | ||
| $this->vcardArray = ['vcard']; | ||
| } | ||
|
|
||
| $this->vcardArray[] = $vcard; | ||
| } |
There was a problem hiding this comment.
Critical: Removed type hints and confusing data structure initialization.
Several critical issues in this method:
-
Removed type hint: The
VCardtype hint was removed from the$vcardparameter without justification, eliminating compile-time type safety. -
Confusing initialization: Line 105 initializes
$vcardArraywith['vcard'](a string), then line 108 appends$vcardobjects. This creates a mixed-type array like['vcard', VCard, VCard, ...], which is error-prone and confusing. -
Inconsistent with usage: The
VcardNormalizerexpects domainVCardobjects, but this mixed structure (string + objects) may cause serialization issues.
Apply this diff to restore type safety and clarify the data structure:
/**
* @param VCard $vcard
*/
- public function addVcard( $vcard): void
+ public function addVcard(VCard $vcard): void
{
if (empty($this->vcardArray)) {
- $this->vcardArray = ['vcard'];
+ $this->vcardArray = [];
}
-
$this->vcardArray[] = $vcard;
}If the ['vcard'] prefix is intentional for jCard format compliance, please add a comment explaining this structure and verify the serialization path handles it correctly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function addVcard( $vcard): void | |
| { | |
| if (empty($this->vcardArray)) { | |
| $this->vcardArray = []; | |
| $this->vcardArray = ['vcard']; | |
| } | |
| $this->vcardArray[] = $vcard; | |
| } | |
| public function addVcard(VCard $vcard): void | |
| { | |
| if (empty($this->vcardArray)) { | |
| $this->vcardArray = []; | |
| } | |
| $this->vcardArray[] = $vcard; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Domain/ValueObject/Event.php (2)
49-54: Consider explicit array initialization inaddLink()for clarity.The method appends to
$linkswhich can benullinitially. While PHP's implicit array initialization handles this, making it explicit improves readability.Apply this diff to make the initialization explicit:
public function addLink(Link $link): self { + if ($this->links === null) { + $this->links = []; + } $this->links[] = $link; return $this; }
91-93: Remove redundant?? nulloperator and consider consistency with similar implementations.The
?? nulloperator on line 93 is redundant since uninitialized properties in PHP are already null. Simply return$this->links;likeCommon.phpdoes.Additionally, the codebase shows inconsistent nullable return types for
getLinks():
Event.phpandCommon.phpreturn?arrayNotice.phpandAbstractData.phpreturn non-nullablearrayConsider aligning these implementations across similar domain entities for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Domain/ValueObject/Event.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Domain/ValueObject/Event.php (3)
src/Domain/ValueObject/Notice.php (1)
getLinks(86-89)src/Domain/Entity/Common.php (1)
getLinks(74-77)src/Domain/ValueObject/SecureDNS/AbstractData.php (1)
getLinks(65-68)
| * @var Link[] | ||
| */ | ||
| private $links = []; | ||
| private $links; |
There was a problem hiding this comment.
Update the docblock to reflect the nullable type.
The docblock declares @var Link[], but the property can now be null since it's no longer initialized and getLinks() returns ?array.
Apply this diff to update the docblock:
/**
- * @var Link[]
+ * @var Link[]|null
*/
private $links;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * @var Link[] | |
| */ | |
| private $links = []; | |
| private $links; | |
| /** | |
| * @var Link[]|null | |
| */ | |
| private $links; |
🤖 Prompt for AI Agents
In src/Domain/ValueObject/Event.php around lines 34 to 36, the property docblock
currently says "@var Link[]" but the property can be null and getLinks() returns
?array; update the docblock to reflect the nullable type by changing it to "@var
Link[]|null" (or "Link[]|null") so the documentation and static analyzers match
the actual nullable property; no other code changes required.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
tests/unit/Serialization/Symfony/stub/full_domain_info.json (2)
208-208: Typo in link title (already flagged).The typo "tittle" instead of "title" was already identified in previous review comments.
259-263: Indentation inconsistency (already flagged).The inconsistent indentation in the
rdapConformancearray was already identified in previous review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Domain/Entity/VCard.php(1 hunks)tests/unit/Serialization/Symfony/DomainSerializerTest.php(3 hunks)tests/unit/Serialization/Symfony/stub/full_domain_info.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit/Serialization/Symfony/DomainSerializerTest.php (4)
src/Domain/Entity/Entity.php (1)
Entity(19-186)src/Domain/Entity/VCard.php (5)
VCard(5-132)setEmail(50-53)setTel(74-81)setFullName(40-43)setCompany(64-67)src/Domain/Entity/Common.php (1)
addRemark(167-175)src/Domain/ValueObject/Notice.php (2)
Notice(15-90)setType(68-73)
src/Domain/Entity/VCard.php (2)
src/Domain/Entity/Entity.php (2)
Entity(19-186)__construct(70-73)src/Infrastructure/Serialization/Symfony/SymfonySerializer.php (1)
__construct(39-73)
🔇 Additional comments (3)
tests/unit/Serialization/Symfony/DomainSerializerTest.php (3)
33-33: Import switched to internal VCard implementation.The test now uses the internal
hiqdev\rdap\core\Domain\Entity\VCardinstead of the external library. Ensure the internal VCard implementation is fully functional before relying on it in tests (see critical issues flagged insrc/Domain/Entity/VCard.php).
180-191: Notice construction pattern updated correctly.The test correctly adapts to the new Notice API where
typeis set viasetType()after construction, rather than being passed to the constructor.
206-211: VCard fluent API usage looks correct.The fluent builder pattern correctly chains
setEmail,setTel,setFullName, andsetCompanymethods. However, this code depends on the VCard implementation being bug-free (critical issues were flagged insrc/Domain/Entity/VCard.php).
| public function __construct(array $vCard = []) | ||
| { | ||
| $this->setVersion(self::VERSION); | ||
| array_merge($this->vCard, $vCard); | ||
| } |
There was a problem hiding this comment.
Critical: Constructor fails to merge provided array.
The array_merge result on line 27 is not assigned back to $this->vCard, so the constructor parameter is silently ignored. This will cause any VCard data passed to the constructor to be lost.
Apply this diff:
public function __construct(array $vCard = [])
{
$this->setVersion(self::VERSION);
- array_merge($this->vCard, $vCard);
+ $this->vCard = array_merge($this->vCard, $vCard);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function __construct(array $vCard = []) | |
| { | |
| $this->setVersion(self::VERSION); | |
| array_merge($this->vCard, $vCard); | |
| } | |
| public function __construct(array $vCard = []) | |
| { | |
| $this->setVersion(self::VERSION); | |
| $this->vCard = array_merge($this->vCard, $vCard); | |
| } |
🤖 Prompt for AI Agents
In src/Domain/Entity/VCard.php around lines 24 to 28, the constructor calls
array_merge but doesn't assign its result back to $this->vCard so the passed-in
$vCard is ignored; fix it by assigning the merged array to $this->vCard (e.g.
set $this->vCard = array_merge($this->vCard, $vCard)) so provided values are
preserved/override defaults, keeping the existing setVersion call intact.
src/Domain/Entity/VCard.php
Outdated
| $this->vCard['addr'] = [ | ||
| 'adr', | ||
| $properties, | ||
| $this->availableProperies['adr'], | ||
| [ | ||
| $name, | ||
| $ext, | ||
| $street, | ||
| $city, | ||
| $state, | ||
| $zip, | ||
| '', | ||
| ], | ||
| ]; | ||
| return $this; |
There was a problem hiding this comment.
Inconsistent property key assignment.
The setAddress method directly assigns to $this->vCard['addr'] instead of using the property name 'adr' defined in $availableProperties (line 17), and it doesn't go through the setPropery helper like other setters. This creates inconsistency.
Consider refactoring to maintain consistency, or document why addr vs adr and the direct assignment pattern are necessary here.
🤖 Prompt for AI Agents
In src/Domain/Entity/VCard.php around lines 97 to 111, the setter for address
assigns directly to $this->vCard['addr'] and bypasses the setProperty helper,
causing an inconsistent key ('addr' vs the configured 'adr') and different
initialization flow; change the code to use the canonical property name from
$this->availableProperies (i.e. 'adr') and call the existing setProperty helper
(or replicate its initialization pattern) so the entry is created via
$this->setProperty('adr', [...]) using $this->availableProperies['adr'] for
metadata rather than directly writing to $this->vCard['addr'].
| protected function setPropery(string $property, string $value, array $properties = [], string $type = 'text'): VCard | ||
| { | ||
| $this->vCard[$property] = [ | ||
| $property, | ||
| empty($properties) ? new \stdClass() : $properties, | ||
| $this->availableProperties[$property] ?? $type, | ||
| $value | ||
| ]; | ||
| return $this; | ||
| } |
There was a problem hiding this comment.
Critical: Undefined property reference and typo in method name.
Line 127 references $this->availableProperties which doesn't exist (the actual property is $availableProperies with a typo at line 10). This will cause a runtime error. Additionally, the method name setPropery is missing a 't'.
After fixing the property name at line 10, apply this diff:
- protected function setPropery(string $property, string $value, array $properties = [], string $type = 'text'): VCard
+ protected function setProperty(string $property, string $value, array $properties = [], string $type = 'text'): VCard
{
$this->vCard[$property] = [
$property,
empty($properties) ? new \stdClass() : $properties,
$this->availableProperties[$property] ?? $type,
$value
];
return $this;
}And update all calls to setPropery throughout the class to setProperty.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Domain/Entity/VCard.php around lines 122 to 131 (and line 10), the method
name setPropery is misspelled and the code references $this->availableProperties
which doesn't exist because the class property was declared as
$availableProperies (typo). Rename the class property at its declaration (line
~10) from availableProperies to availableProperties, rename the method
setPropery to setProperty, and update every call to setPropery within this class
to setProperty so the property reference and method name are consistent and the
runtime error is resolved.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/Domain/Entity/VCard.php (3)
24-28: Critical: Constructor fails to merge provided array.This issue was already flagged in a previous review. The
array_mergeresult on line 27 is not assigned back to$this->vCard, so the constructor parameter is silently ignored.
97-111: Critical: Typo in property reference and inconsistent key.This method has multiple issues already flagged in previous reviews:
- Line 97 uses
'addr'as the key instead of'adr'(defined in$availableProperties)- Line 100 references
$this->availableProperies(typo) which will cause a fatal error - should be$this->availableProperties- The method bypasses the
setPropery/setPropertyhelper
122-131: Critical: Method name typo.This issue was already flagged in a previous review. The method is named
setPropery(missing 't') instead ofsetProperty, and all callers use the typo name.
🧹 Nitpick comments (1)
src/Domain/Entity/VCard.php (1)
22-22: Initialize the$vCardproperty.The
$vCardproperty is declared but not initialized. While PHP auto-vivifies arrays, explicitly initializing to an empty array is better practice.Apply this diff:
- private $vCard; + private $vCard = [];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Domain/Entity/VCard.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Domain/Entity/VCard.php (2)
src/Domain/Entity/Entity.php (2)
Entity(19-186)__construct(70-73)src/Infrastructure/Serialization/Symfony/SymfonySerializer.php (1)
__construct(39-73)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
tests/unit/Serialization/Symfony/stub/full_domain_info.json (2)
306-311: Fix indentation inconsistency.Lines 307-310 use 6-space indentation while the rest of the file uses consistent 2-space increments. The array elements should use 4-space indentation (2 base + 2 for nesting).
Apply this diff:
"rdapConformance": [ - "rdap_level_0", - "icann_rdap_technical_implementation_guide_1", - "icann_rdap_response_profile_1", - "redacted" + "rdap_level_0", + "icann_rdap_technical_implementation_guide_1", + "icann_rdap_response_profile_1", + "redacted" ]
255-255: Fix typo in link title.Line 255 contains
"title": "new tittle"which should be"title": "new title"(spelling error).Apply this diff:
- "title": "new tittle", + "title": "new title",src/Domain/Entity/VCard.php (2)
24-28: Critical: Constructor ignores provided data.The
array_mergeresult on line 27 is not assigned back to$this->vCard, so any VCard data passed to the constructor is silently discarded.Apply this diff:
public function __construct(array $vCard = []) { $this->setVersion(self::VERSION); - array_merge($this->vCard, $vCard); + $this->vCard = array_merge($this->vCard, $vCard); }
120-129: Fix method name typo.The method name
setProperyis missing a 't' and should besetProperty. This typo affects all callers (lines 37, 42, 47, 52, 61, 71, 80).Apply this diff to rename the method:
-protected function setPropery(string $property, string $value, array $properties = [], string $type = 'text'): VCard +protected function setProperty(string $property, string $value, array $properties = [], string $type = 'text'): VCard { $this->vCard[$property] = [ $property, empty($properties) ? new \stdClass() : $properties, $this->availableProperties[$property] ?? $type, $value ]; return $this; }And update all callers throughout the class (lines 37, 42, 47, 52, 61, 71, 80) from
setProperytosetProperty.src/Domain/Entity/Domain.php (1)
133-140: Critical: Type mismatch in getEntities() logic.The method checks for
$this->entities['entities'](nested structure), butaddEntity()at line 224 builds a flatEntity[]array. This creates a type mismatch and will cause incorrect behavior.Apply this diff to return the flat array directly:
public function getEntities(): ?array { - if (empty($this->entities['entities'])) { - return $this->entities ?? []; - } - - return $this->entities['entities']; + return $this->entities; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Domain/Entity/Domain.php(5 hunks)src/Domain/Entity/TopMostEntityTrait.php(1 hunks)src/Domain/Entity/VCard.php(1 hunks)tests/unit/Domain/Entity/DomainTest.php(1 hunks)tests/unit/Serialization/Symfony/stub/full_domain_info.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tests/unit/Domain/Entity/DomainTest.php (2)
src/Domain/Entity/Domain.php (1)
Domain(19-262)src/Domain/Entity/TopMostEntityTrait.php (1)
getRdapConformance(28-31)
src/Domain/Entity/Domain.php (4)
src/Domain/Entity/Entity.php (1)
__construct(70-73)src/Domain/ValueObject/SecureDNS.php (2)
__construct(51-66)SecureDNS(16-107)src/Domain/Entity/Common.php (1)
__construct(66-69)src/Domain/ValueObject/DomainName.php (3)
__construct(33-46)DomainName(20-132)toLDH(97-104)
src/Domain/Entity/TopMostEntityTrait.php (1)
src/Domain/Entity/Domain.php (1)
Domain(19-262)
src/Domain/Entity/VCard.php (2)
src/Domain/Entity/Entity.php (2)
Entity(19-186)__construct(70-73)src/Infrastructure/Serialization/Symfony/SymfonySerializer.php (1)
__construct(39-73)
🔇 Additional comments (5)
tests/unit/Domain/Entity/DomainTest.php (1)
149-158: LGTM!The test correctly validates that
Domain::getRdapConformance()returns the expected conformance tokens fromTopMostEntityTrait.src/Domain/Entity/Domain.php (3)
21-21: LGTM!The integration of
TopMostEntityTraitcorrectly brings RDAP conformance management to the Domain entity.
66-75: LGTM!The constructor correctly initializes
secureDNSand sets up default redaction rules viasetDefaultRedactedRules().
240-243: LGTM!The getter correctly returns the redaction rules array.
src/Domain/Entity/TopMostEntityTrait.php (1)
1-48: LGTM!The trait provides clean management of RDAP conformance tokens with sensible defaults and a complete API (getter, setter, and adder).
| private function setDefaultRedactedRules(): void | ||
| { | ||
| $this->redacted = []; | ||
| foreach (['registrant', 'admin', 'tech', 'billing'] as $k => $v) { | ||
| $this->redacted[] = [ | ||
| 'name' => [ | ||
| 'type' => 'Registry ' . ucfirst($v) . ' ID', | ||
| ], | ||
| 'prePath' => "$.entities[{$k}].handle", | ||
| 'pathLang' => "jsonpath", | ||
| 'method' => 'removal', | ||
| 'reason' => [ | ||
| 'description' => 'Server policy', | ||
| ] | ||
| ]; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify entity ordering assumption.
The redaction rules assume entities are ordered as registrant (index 0), admin (1), tech (2), billing (3). The prePath references like "$.entities[{$k}].handle" rely on this exact ordering. Ensure that entity ordering is guaranteed when building the domain object, or the redaction rules will target incorrect entities.
Run the following script to verify how entities are added and if ordering is enforced:
🏁 Script executed:
#!/bin/bash
# Description: Check if entity ordering is enforced or documented
# Search for code that adds entities to Domain objects
rg -nP --type=php -C5 '->addEntity\(' | grep -A5 -B5 'Domain'
# Look for role-based entity ordering logic or documentation
rg -nP --type=php -C3 'registrant|admin|tech|billing' | grep -i 'entit'
# Check for any role constants or enums
ast-grep --pattern 'class Role {
$$$
}'Length of output: 2016
Now let me search for more details on the specific codebase structure:
Use role-based JSONPath filters instead of hardcoded positional indices for entity redaction rules.
The RFC 9537 standard explicitly warns against removing array elements by position where that position determines semantic meaning, and recommends using role-based JSONPath filters like $.entities[?(@.roles[0]=='administrative')] instead of positional indices.
The current code assumes entities are ordered as registrant (index 0), admin (1), tech (2), billing (3), but entity ordering in RDAP responses varies across servers and is not guaranteed. This will cause redaction rules to target incorrect entities when the server returns entities in a different order.
Replace the hardcoded indices with role-based filters to ensure the rules target the correct entities regardless of their position in the array.
🤖 Prompt for AI Agents
In src/Domain/Entity/Domain.php around lines 245-261, the redaction rules
currently use hardcoded array indices for entities which is fragile; change
prePath to use role-based JSONPath filters instead. For each role (registrant,
admin/administrative, tech/technical, billing) build the prePath using a
JSONPath filter that matches the entity's roles (e.g.
$.entities[?(@.roles[*]=='registrant')].handle or the appropriate variant for
administrative/technical) so the rule targets entities by role rather than
position; keep the rest of the rule fields (pathLang, method, reason) the same.
Ensure the role name variants used match the RDAP responses your system expects
(administrative vs admin, technical vs tech) and handle any pluralization/array
match with the JSONPath expression.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/Domain/Entity/Domain.php (2)
131-138: Nested structure logic remains inconsistent with flat array type.The conditional
$this->entities['entities']expects a nested structure, butaddEntity()builds a flatEntity[]array and the property is typed asEntity[]|null. This mismatch was flagged in previous reviews and remains unresolved.The method should return
$this->entitiesdirectly, consistent withEntity::getEntities()andIPNetwork::getEntities()implementations shown in the relevant code snippets.
247-263: Positional entity indices remain fragile.The redaction rules continue to use hardcoded positional indices
$.entities[{$k}], assuming entities are ordered consistently. This issue was flagged in previous reviews—entity ordering in RDAP responses is not guaranteed, and RFC 9537 recommends role-based JSONPath filters instead.Replace positional indices with role-based filters (e.g.,
$.entities[?(@.roles[*]=='registrant')]) to ensure rules target the correct entities regardless of array position.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Domain/Entity/Domain.php(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Domain/Entity/Domain.php (4)
src/Domain/Entity/Entity.php (2)
__construct(70-73)getEntities(125-128)src/Domain/ValueObject/SecureDNS.php (2)
__construct(51-66)SecureDNS(16-107)src/Domain/Entity/Common.php (1)
__construct(66-69)src/Domain/ValueObject/DomainName.php (3)
__construct(33-46)DomainName(20-132)toLDH(97-104)
🪛 PHPMD (2.15.0)
src/Domain/Entity/Domain.php
250-250: Avoid unused local variables such as '$v'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (3)
src/Domain/Entity/Domain.php (3)
21-21: LGTM: TopMostEntityTrait integration.The trait adoption correctly adds rdapConformance support to the Domain entity, aligning with the redaction functionality introduced in this PR.
63-64: LGTM: Redaction rules storage property.The property declaration is appropriate for storing redaction configurations.
238-245: LGTM: Lazy initialization of redaction rules.The method appropriately uses lazy initialization to generate default redaction rules on first access.
|
|
||
| $this->ldhName = $ldhName->toLDH(); | ||
|
|
||
| $this->secureDNS = new SecureDNS(); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify eager SecureDNS initialization.
The constructor now always creates a SecureDNS() instance with default values (delegationSigned=false). Since the property is typed as SecureDNS|null, this changes the behavior to never return null from getSecureDNS().
Confirm this is intentional—previously the property could be null until explicitly set via setSecureDNS().
🏁 Script executed:
#!/bin/bash
# Description: Check how SecureDNS is used in domain construction and if null checks exist
# Look for null checks on getSecureDNS()
rg -nP --type=php -C3 '->getSecureDNS\(\)\s*(?:===|!==|==|!=)\s*null'
# Look for Domain construction patterns to see if SecureDNS is always set
rg -nP --type=php -C5 'new Domain\(' | head -50Length of output: 4596
---
🏁 Script executed:
```shell
#!/bin/bash
# Inspect the Domain constructor signature
cat src/Domain/Entity/Domain.php | head -100
echo "---"
# Search for getSecureDNS() usage patterns (escaped properly)
rg -nP --type=php 'getSecureDNS' -C3
Length of output: 2882
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for null checks on SecureDNS (various patterns)
echo "=== Checking for null checks on getSecureDNS() ==="
rg -nP --type=php 'getSecureDNS\(\)' -C3
echo "=== Checking for secureDNS property access ==="
rg -nP --type=php 'secureDNS' -C2
echo "=== Checking SecureDNS constructor to understand default behavior ==="
fd -t f 'SecureDNS\.php' --exec head -80 {}
Length of output: 4060
Update return type annotation to reflect non-nullable contract.
The constructor now eagerly initializes SecureDNS with default values, ensuring getSecureDNS() never returns null. However, the return type annotation remains ?SecureDNS (line 123 in src/Domain/Entity/Domain.php), which is now inaccurate.
Change line 123 from:
public function getSecureDNS(): ?SecureDNSto:
public function getSecureDNS(): SecureDNSThis reflects the actual contract that the property is always initialized after construction.
🤖 Prompt for AI Agents
In src/Domain/Entity/Domain.php (constructor change at line 72 and getter at
line 123), the constructor now eagerly sets $this->secureDNS so getSecureDNS()
is no longer nullable; update the method signature at line 123 from a nullable
return type to a non-nullable one by changing the declaration to return
SecureDNS (and update any PHPDoc/@return annotations accordingly), and run
static checks to ensure callers are compatible with the non-nullable return.
| private function setDefaultRedactedRules(): void | ||
| { | ||
| $this->redacted = []; | ||
| foreach (($this->getEntities() ?? []) as $k => $v) { |
There was a problem hiding this comment.
Remove unused variable $v in foreach loop.
The loop variable $v is declared but never used. Consider using array_keys($this->getEntities() ?? []) to iterate over indices only.
Apply this diff:
- foreach (($this->getEntities() ?? []) as $k => $v) {
+ foreach (array_keys($this->getEntities() ?? []) as $k) {Based on static analysis.
🧰 Tools
🪛 PHPMD (2.15.0)
250-250: Avoid unused local variables such as '$v'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In src/Domain/Entity/Domain.php around line 250 the foreach declares ($k => $v)
but $v is never used; replace the iteration to only iterate keys, e.g. call
array_keys on the possibly-null array: foreach (array_keys($this->getEntities()
?? []) as $k) { ... } so only $k is declared and the unused $v is removed.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.