feat: import detect zone name

[loks0n]

Summary by CodeRabbit

• Breaking Changes

  • Zone file import method signature updated—zone name now derived from file content or default origin instead of passed as a parameter.

• Improvements

  • Enhanced validation for zone configuration with improved error reporting for missing or invalid origins and SOA records.

[coderabbitai]

Walkthrough

The pull request refactors the File::import() method signature, removing the $name parameter and reordering parameters so that zone file content becomes the first argument, followed by an optional default origin. The implementation was updated to delay zone name resolution, compute zone name from the provided default origin or directives, and validate origins more strictly. Directive handling was modified to return specific directive types rather than a boolean, and error handling was enhanced to throw exceptions for empty origins or missing zone names. All corresponding test cases were updated to use the new parameter order.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45–60 minutes

• Public API signature change: Removal of the $name parameter fundamentally alters how callers invoke the method; backward compatibility breaks require careful consideration
• Zone name resolution logic: The delayed resolution and interaction between default origin, directives, and zone name determination requires careful tracing through the new flow
• Directive handling refactor: The return type change from bool to ?string and the conditional logic branching on returned values need verification
• Error handling additions: New validation checks for empty origins and missing zone names require examination for correctness and coverage
• Test updates scope: Numerous test cases updated with new parameter ordering; verify all scenarios (with/without origin, with/without directives) are properly covered

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The pull request title 'feat: import detect zone name' is vague and does not clearly convey the main change. While it hints at zone name detection, it fails to communicate that the primary change is a significant API modification—specifically, the removal of the zone name parameter from the import method signature and the refactoring of how zone names are resolved (now derived from directives or a default origin). A developer scanning the history would not immediately understand the breaking API change or the shift from explicit zone name specification to automatic detection.	Consider revising the title to be more specific about the main change, such as 'refactor: remove zone name parameter from import method' or 'feat: auto-detect zone name from origin or directives'. This would better communicate the breaking API change and the new behavior to reviewers and maintainers reviewing the commit history.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat-import-detect-zone-name

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 39e14d2 and f361766.

📒 Files selected for processing (2)

• src/DNS/Zone/File.php (4 hunks)
• tests/unit/DNS/Zone/FileTest.php (38 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

tests/unit/DNS/Zone/FileTest.php (1)

src/DNS/Zone/File.php (2)

• File (13-815)
• import (36-125)

src/DNS/Zone/File.php (1)

src/DNS/Zone.php (1)

• Zone (10-72)

⏰ 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: Unit & E2E

🔇 Additional comments (9)

tests/unit/DNS/Zone/FileTest.php (4)

19-19: LGTM! Test calls updated correctly.

These test calls now rely on the zone file content containing $ORIGIN directives to infer the zone name, which aligns with the new API design.

Also applies to: 29-29, 49-49, 59-59

---

39-39: LGTM! Explicit origin parameter.

This correctly tests the scenario where a defaultOrigin is explicitly provided as the second parameter.

---

192-203: Excellent test coverage for the new feature!

This test validates the core functionality of the refactored API: when no $ORIGIN directive is present in the zone file, the defaultOrigin parameter is used to determine the zone name and resolve relative record names.

---

78-78: LGTM! All test calls consistently updated.

All remaining File::import() calls have been correctly updated to use the new signature with content as the first parameter. The tests cover a comprehensive range of scenarios including error cases, round-trip import/export, and various record types.

Also applies to: 109-112, 127-127, 142-142, 157-157, 172-172, 187-187, 210-210, 224-224, 246-246, 262-262, 280-280, 295-295, 311-311, 326-326, 347-347, 364-364, 368-368, 393-393, 407-407, 430-430, 449-449, 467-467, 485-485, 502-502, 519-519, 523-523, 539-539, 545-545, 566-566, 577-577, 594-594, 611-611, 625-625, 642-642, 659-659, 670-670, 678-678, 686-689

src/DNS/Zone/File.php (5)

31-36: LGTM! Improved API design.

The refactored signature makes the API more intuitive by inferring the zone name from the content rather than requiring it as a separate parameter. The parameter order (content first, optional origin second) is logical and user-friendly.

---

42-53: LGTM! Clean initialization logic.

The initialization properly handles the optional defaultOrigin parameter:

• Canonicalizes the origin to ensure consistent format
• Validates that it's not empty after canonicalization
• Uses the $zoneNameFromDefault flag to track whether the zone name came from the default, allowing $ORIGIN directives to override it later

---

65-76: LGTM! Correct directive handling logic.

The zone name resolution logic is well-designed:

• Validates that $ORIGIN directives have non-empty values
• The first $ORIGIN directive (or defaultOrigin if no directive) determines the zone name
• Subsequent $ORIGIN directives update only the origin for relative name resolution
• The $zoneNameFromDefault flag correctly allows directives to override the default origin

This behavior is validated by the existing test testImportHandlesMultipleOrigins() which verifies that zone name remains consistent while origins can change for relative name resolution.

---

120-122: LGTM! Appropriate validation with clear error message.

This final check ensures that a zone name was determined through either an $ORIGIN directive or defaultOrigin parameter. The error message clearly guides users on how to fix the issue.

---

263-286: LGTM! Improved directive handling design.

Changing the return type from bool to ?string with specific directive type values ('origin', 'ttl', null) is a good refactor. This provides:

• More descriptive return values indicating which directive was processed
• Better extensibility for handling different directives differently
• Clear semantic meaning in the calling code

The reference parameters continue to work correctly, and the 'origin' return value enables the caller to perform additional validation and zone name resolution logic.

---

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

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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