SRV records support with endpoint notifications #67
Conversation
…n SubdomainResource
…record target or port
…delete functionality
…ifications on success or failure
…ecords created outside plugin)
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds SRV record support and Cloudflare API integration: DB migrations for Changes
Sequence Diagram(s)sequenceDiagram
participant User as Filament User
participant Filament as Filament UI
participant Subdomain as Subdomain Model
participant Service as CloudflareService
participant Cloudflare as Cloudflare API
User->>Filament: Submit create/edit subdomain (srv_record toggle)
Filament->>Subdomain: Persist attributes (including srv_record)
Subdomain->>Subdomain: Trigger saving hook -> upsertOnCloudflare()
alt srv_record = true (SRV)
Subdomain->>Service: upsertDnsRecord(zoneId, name, "SRV", srv_target, recordId, port)
else (A/AAAA)
Subdomain->>Service: upsertDnsRecord(zoneId, name, "A/AAAA", ip, recordId)
end
Service->>Cloudflare: HTTP request (create/update/delete)
Cloudflare-->>Service: API response
Service-->>Subdomain: Normalized result { success,id,status,body }
alt success
Subdomain->>Filament: Emit success notification / update model fields
else failure
Subdomain->>Filament: Emit error/danger notification / log warning
end
sequenceDiagram
participant Admin as Admin User
participant EditForm as EditNode Form
participant Node as Node Model
participant Job as UpdateServerSubdomains Job
participant Subdomain as Subdomain Model
Admin->>EditForm: Update srv_target and save node
EditForm->>Node: Persist srv_target (dehydrate/afterState hooks)
EditForm->>Admin: Show localized "settings saved" notification
EditForm->>Job: Dispatch UpdateServerSubdomains for node
Job->>Subdomain: Iterate servers -> trigger upsertOnCloudflare()
Subdomain->>CloudflareService: upsertDnsRecord(...) per subdomain
CloudflareService->>Cloudflare: API calls
Cloudflare-->>CloudflareService: responses
CloudflareService-->>Subdomain: results -> notifications/logging
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 9
🧹 Nitpick comments (3)
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php (1)
120-124: Consider using a relationship or callback optimization.The
CloudflareDomain::find($get('domain_id'))query runs on every form render/interaction. While functional, this could be optimized by leveraging the relationship already defined on the model, or by using->live()on thedomain_idfield to control when the lookup occurs.🔎 Suggested optimization
Toggle::make('srv_record') ->label(trans('subdomains::strings.srv_record')) ->helperText(trans('subdomains::strings.srv_record_help')) - ->disabled(fn (callable $get) => !$get('domain_id') || empty(CloudflareDomain::find($get('domain_id'))->srv_target ?? null)) + ->disabled(fn (callable $get, ?Subdomain $record) => + !$get('domain_id') || + empty($record?->domain?->srv_target ?? CloudflareDomain::find($get('domain_id'))?->srv_target) + ) ->default(false),subdomains/src/Services/CloudflareService.php (1)
69-82: Consider making the SRV service prefix configurable or explicitly document the Minecraft-specific scope.The
_minecraft._tcpprefix is hardcoded at line 70 with no mechanism to customize it. Given that the plugin documentation describes it as supporting "game servers" generically, this architectural decision limits the plugin's reusability for other services that use SRV records (e.g., SIP, XMPP, other games). Either explicitly scope the plugin to Minecraft-only in documentation and naming, or refactor to accept the service prefix as a parameter or configuration option.subdomains/src/Models/Subdomain.php (1)
10-10: Remove unused import.The
Httpfacade is imported but never used in this file. The code now usesCloudflareServicefor all HTTP interactions.🔎 Proposed fix
-use Illuminate\Support\Facades\Http;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
subdomains/database/migrations/004_add_srv_target_to_cloudflare_domains.phpsubdomains/lang/en/strings.phpsubdomains/src/Filament/Admin/Resources/CloudflareDomains/CloudflareDomainResource.phpsubdomains/src/Filament/Admin/Resources/CloudflareDomains/Pages/ManageCloudflareDomains.phpsubdomains/src/Filament/Admin/Resources/Servers/RelationManagers/SubdomainRelationManager.phpsubdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.phpsubdomains/src/Models/CloudflareDomain.phpsubdomains/src/Models/Subdomain.phpsubdomains/src/Services/CloudflareService.php
🧰 Additional context used
🧬 Code graph analysis (2)
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php (2)
subdomains/src/Models/Subdomain.php (2)
Subdomain(25-218)domain(58-61)subdomains/src/Models/CloudflareDomain.php (1)
CloudflareDomain(17-55)
subdomains/src/Models/CloudflareDomain.php (1)
subdomains/src/Services/CloudflareService.php (2)
CloudflareService(9-161)getZoneId(11-39)
🪛 GitHub Actions: Pint
subdomains/src/Filament/Admin/Resources/Servers/RelationManagers/SubdomainRelationManager.php
[error] 1-1: pint --test: no_unused_imports violation detected.
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php
[error] 1-1: pint --test: no_unused_imports violation detected.
subdomains/src/Services/CloudflareService.php
[error] 1-1: pint --test: class_attributes_separation violation detected.
subdomains/src/Models/CloudflareDomain.php
[error] 1-1: pint --test: no_unused_imports violation detected.
[error] 1-1: pint --test: ordered_imports violation detected.
subdomains/src/Models/Subdomain.php
[error] 1-1: pint --test: class_attributes_separation violation detected.
[error] 1-1: pint --test: statement_separation violation detected.
🔇 Additional comments (12)
subdomains/src/Filament/Admin/Resources/CloudflareDomains/Pages/ManageCloudflareDomains.php (1)
16-18: LGTM — notification delegation to model layer.Disabling
successNotification(null)here avoids duplicate notifications since theCloudflareDomainmodel now emits its own success/failure notifications during thecreatedevent. This is a reasonable approach for consolidated feedback.subdomains/src/Filament/Admin/Resources/CloudflareDomains/CloudflareDomainResource.php (1)
56-57: LGTM — SRV target field implementation.The new
srv_targetfield is well-integrated with appropriate labels, helper text, and a solid hostname validation regex that follows RFC conventions for domain names.Also applies to: 82-86
subdomains/src/Filament/Admin/Resources/Servers/RelationManagers/SubdomainRelationManager.php (1)
87-90: LGTM — SRV record toggle implementation.The toggle replaces the previous hidden
record_typefield with a user-controllable option, which aligns with the PR's goal of enabling SRV record support.subdomains/lang/en/strings.php (1)
17-37: LGTM — comprehensive language strings.The new translation keys provide clear, consistent messaging for SRV record functionality and Cloudflare notifications.
subdomains/database/migrations/004_add_srv_target_to_cloudflare_domains.php (1)
7-21: LGTM — clean migration.The migration correctly adds a nullable
srv_targetcolumn and provides proper rollback support.subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php (1)
83-86: LGTM — ToggleColumn with appropriate guards.The toggle is properly disabled when the domain is missing or lacks an
srv_target, with contextual tooltips to inform users why it's unavailable.subdomains/src/Services/CloudflareService.php (2)
41-120: LGTM — well-structured upsert logic.The method properly validates inputs, handles both SRV and A/AAAA record types, and provides comprehensive error handling with structured response arrays. The separation between create (POST) and update (PUT) based on
recordIdpresence follows Cloudflare API conventions.
11-39: LGTM — solid service layer implementation.The
getZoneIdanddeleteDnsRecordmethods have appropriate input validation, exception handling, and logging. The structured return format enables consistent error handling in calling code.Also applies to: 139-160
subdomains/src/Models/Subdomain.php (4)
27-38: LGTM!The
srv_recordfield is properly added to fillable and cast as boolean.
44-51: Consider whether Cloudflare failures should halt the save.The
upsertOnCloudflare()method returns early on failure but the model save proceeds regardless. This could leave the database record and Cloudflare DNS out of sync. If this is intentional (e.g., to allow offline editing), consider documenting this behavior; otherwise, consider throwing an exception to abort the save on Cloudflare failures.
73-76: LGTM!Clean accessor implementation for the virtual
srv_recordattribute.
78-89: LGTM!Proper null-safe checking for server allocation and correct IP version detection logic.
subdomains/src/Filament/Admin/Resources/Servers/RelationManagers/SubdomainRelationManager.php
Outdated
Show resolved
Hide resolved
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php
Outdated
Show resolved
Hide resolved
- Fixed potential potential NPE - Corrected some notifications
- Removed inbuilt notifications in favor of service notifications
- Fixed Style/unused class issues
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
subdomains/src/Models/CloudflareDomain.php (1)
28-47: Use translation keys for notification messages.Lines 34 and 39 contain hardcoded English strings. The past review comment indicated that translation keys
zone_request_failedandzone_request_succeededare already defined instrings.php. Please use these keys instead of hardcoded strings for consistency and i18n support.subdomains/src/Services/CloudflareService.php (1)
136-138: Fix formatting to resolve pipeline failure.The pipeline is failing due to
braces_positionandblank_lineviolations. Based on past review comments, there may be spacing issues between methods. Ensure consistent spacing (single blank line) between method declarations per the project's PHP-CS-Fixer rules.subdomains/src/Models/Subdomain.php (1)
190-211: Critical: Fix delete notification logic.Multiple issues with the delete notifications:
- Success notification uses wrong severity: Lines 198-202 send a
danger()notification on success when it should besuccess().- Typo: Line 200 has "successed" which should be "succeeded".
- Unconditional failure notification: Lines 205-209 always execute, sending a failure notification even after a successful delete. This results in users seeing both success and failure notifications.
Add an
elsebranch or early return after the success case.🔎 Proposed fix
if (!empty($result['success'])) { + Log::info('Deleted Cloudflare record for Subdomain ID ' . $this->id, ['result' => $result]); + Notification::make() - ->danger() - ->title('Cloudflare: Delete successed') + ->success() + ->title('Cloudflare: Delete succeeded') ->body('Successfully deleted Cloudflare record for ' . $this->name . '.' . ($this->domain->name ?? 'unknown') . '.') ->send(); + } else { + Log::warning('Failed to delete Cloudflare record for Subdomain ID ' . $this->id, ['result' => $result]); + + Notification::make() + ->danger() + ->title('Cloudflare: Delete failed') + ->body('Failed to delete Cloudflare record for ' . $this->name . '.' . ($this->domain->name ?? 'unknown') . '. See logs for details. Errors: ' . json_encode($result['errors'] ?? $result['body'] ?? [])) + ->send(); } - - Notification::make() - ->danger() - ->title('Cloudflare: Delete failed') - ->body('Failed to delete Cloudflare record for ' . $this->name . '.' . ($this->domain->name ?? 'unknown') . '. See logs for details. Errors: ' . json_encode($result['errors'] ?? $result['body'] ?? [])) - ->send();
🧹 Nitpick comments (1)
subdomains/src/Services/CloudflareService.php (1)
69-82: Hardcoded Minecraft SRV naming limits flexibility.Line 70 hardcodes the SRV record name as
_minecraft._tcp.%s, which restricts this implementation to Minecraft servers only. If the plugin is intended solely for Minecraft, this should be documented. Otherwise, consider making the service and protocol configurable.🔎 Potential refactor to support configurable service/protocol
public function upsertDnsRecord( string $zoneId, string $name, string $recordType, string $target, ?string $recordId = null, ?int $port = null, + ?string $service = '_minecraft', + ?string $protocol = '_tcp', ): array { // ... if ($recordType === 'SRV') { // ... $payload = [ - 'name' => sprintf('_minecraft._tcp.%s', $name), + 'name' => sprintf('%s.%s.%s', $service, $protocol, $name), // ... ]; } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
subdomains/src/Filament/Admin/Resources/Servers/RelationManagers/SubdomainRelationManager.phpsubdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.phpsubdomains/src/Models/CloudflareDomain.phpsubdomains/src/Models/Subdomain.phpsubdomains/src/Services/CloudflareService.php
🚧 Files skipped from review as they are similar to previous changes (2)
- subdomains/src/Filament/Admin/Resources/Servers/RelationManagers/SubdomainRelationManager.php
- subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php
🧰 Additional context used
🧬 Code graph analysis (2)
subdomains/src/Models/CloudflareDomain.php (1)
subdomains/src/Services/CloudflareService.php (2)
CloudflareService(9-160)getZoneId(11-39)
subdomains/src/Models/Subdomain.php (2)
subdomains/src/Services/CloudflareService.php (3)
CloudflareService(9-160)upsertDnsRecord(41-120)deleteDnsRecord(138-159)subdomains/src/Models/CloudflareDomain.php (1)
boot(24-48)
🪛 GitHub Actions: Pint
subdomains/src/Services/CloudflareService.php
[error] 1-1: pint --test failed: braces_position, blank_line issues detected in CloudflareService.php. Command failed with exit code 1.
subdomains/src/Models/Subdomain.php
[error] 1-1: pint --test failed: no_unused_imports, ordered_imports issues detected in Subdomain.php. Command failed with exit code 1.
🔇 Additional comments (7)
subdomains/src/Services/CloudflareService.php (3)
11-39: LGTM!The
getZoneIdmethod properly validates inputs, handles exceptions, and returns the first matching zone ID. Error logging is appropriate.
94-120: LGTM!The upsert logic correctly differentiates between create (POST) and update (PUT) based on the presence of
$recordId. Error handling and logging are appropriate.
138-159: LGTM!The
deleteDnsRecordmethod properly validates inputs, handles the DELETE request, and includes appropriate error handling and logging.subdomains/src/Models/Subdomain.php (4)
44-58: LGTM!The lifecycle hooks are well-structured. Using
savingto deriverecord_typefromsrv_recordbefore persistence, thensavedto trigger the Cloudflare upsert, ensures the model state is consistent before making external API calls.
75-91: LGTM!The accessor/mutator pattern for
srv_recordis clean. ThesetRecordTypemethod properly determines the record type (SRV, A, or AAAA) based on the boolean flag and IP version, with appropriate null checks.
110-149: LGTM!The SRV record upsert logic properly validates the port, calls the service with the domain's
srv_target, and usesupdateQuietlyon line 130 to avoid triggering lifecycle hooks recursively. Notifications provide clear feedback on success and failure.
151-187: LGTM!The A/AAAA record upsert logic includes proper null checks for
$this->serverand$this->server->allocation(addressing the past review comment), validates IP addresses, and provides appropriate notifications. The use ofupdateQuietlyprevents recursive hook invocation.
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)
subdomains/src/Models/Subdomain.php (1)
1-211: Fix Pint violation: Remove whitespace from blank lines.The pipeline failure indicates the
no_whitespace_in_blank_linerule is triggered. Ensure all blank lines throughout the file contain no spaces or tabs.Run Pint locally to identify and auto-fix the whitespace:
./vendor/bin/pint subdomains/src/Models/Subdomain.php
🧹 Nitpick comments (1)
subdomains/src/Models/Subdomain.php (1)
92-187: Optional: Consider extracting duplicate success-handling logic.The cloudflare_id update (lines 128-130, 166-168) and success notification (lines 132-136, 170-174) are duplicated across the SRV and A/AAAA paths. Extracting a small helper method would reduce repetition.
🔎 Example refactor to reduce duplication
private function handleUpsertSuccess(string $recordId): void { if ($this->cloudflare_id !== $recordId) { $this->updateQuietly(['cloudflare_id' => $recordId]); } Notification::make() ->success() ->title('Cloudflare: Record updated') ->body('Successfully updated ' . $this->name . '.' . ($this->domain->name ?? 'unknown') . ' to ' . $this->record_type) ->send(); }Then replace lines 128-136 and 166-174 with:
$this->handleUpsertSuccess($result['id']);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
subdomains/src/Models/Subdomain.phpsubdomains/src/Services/CloudflareService.php
🚧 Files skipped from review as they are similar to previous changes (1)
- subdomains/src/Services/CloudflareService.php
🧰 Additional context used
🧬 Code graph analysis (1)
subdomains/src/Models/Subdomain.php (2)
subdomains/src/Services/CloudflareService.php (3)
CloudflareService(9-160)upsertDnsRecord(41-120)deleteDnsRecord(138-159)subdomains/src/Providers/SubdomainsPluginProvider.php (1)
boot(23-36)
🪛 GitHub Actions: Pint
subdomains/src/Models/Subdomain.php
[error] 1-1: pint --test failed with exit code 1. Rule 'no_whitespace_in_blank_line' triggered.
🔇 Additional comments (4)
subdomains/src/Models/Subdomain.php (4)
6-7: LGTM: Imports support new CloudflareService integration.The new imports (CloudflareService, Notification, Log) are properly used throughout the upsert and delete flows.
Also applies to: 12-12
26-37: LGTM: srv_record field properly configured.The fillable array and boolean cast correctly support the new SRV record toggle feature.
39-57: LGTM: Lifecycle hooks properly refactored.The consolidation from
created/updatedtosaving/savedis cleaner. The srv_record virtual attribute is correctly processed and unset before persistence, and Cloudflare operations are properly triggered.
74-90: LGTM: srv_record accessor/mutator logic is sound.The accessor provides a clean boolean interface, and the mutator correctly derives record_type with proper null-safe checks for the server allocation.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
subdomains/src/Services/CloudflareService.php (1)
122-136: Refine success detection to match Cloudflare API semantics.Line 127's logic
$body['success'] === true || count($body['result']) > 0treatssuccessandresultas interchangeable indicators, which doesn't align with the Cloudflare API's response structure wheresuccessis an explicit boolean flag andresultis the data payload.Issues:
- The OR condition returns
success=trueif onlyresulthas data, even when$body['success']isfalsecount()on a non-arrayresultwill error- Line 131 extracts
idfromresultwithout verifyingresultexists or is an arrayThe stricter pattern at line 30 (
!empty($body['result']) && count($body['result']) > 0) is more defensive.🔎 Proposed fix
protected function parseCloudflareHttpResponse(Response $response): array { $status = $response->status(); $body = $response->json() ?? []; - $success = $response->successful() && ($body['success'] === true || count($body['result']) > 0); + $success = $response->successful() && $body['success'] === true; return [ 'success' => $success, - 'id' => $body['result']['id'] ?? null, + 'id' => is_array($body['result'] ?? null) ? ($body['result']['id'] ?? null) : null, 'errors' => $body['errors'] ?? [], 'status' => $status, 'body' => $body, ]; }
🧹 Nitpick comments (1)
subdomains/src/Services/CloudflareService.php (1)
69-82: Consider making the SRV naming convention configurable.Line 70 hardcodes the Minecraft-specific SRV naming convention (
_minecraft._tcp.%s). While this works for the current use case, it limits the service's reusability for other SRV record types (e.g.,_http._tcp,_xmpp._tcp).Consider adding optional parameters for the service and protocol, or documenting that this service is Minecraft-specific.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
subdomains/lang/en/strings.phpsubdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.phpsubdomains/src/Models/CloudflareDomain.phpsubdomains/src/Models/Subdomain.phpsubdomains/src/Services/CloudflareService.phpsubdomains/src/SubdomainsPlugin.php
🚧 Files skipped from review as they are similar to previous changes (1)
- subdomains/src/Models/CloudflareDomain.php
🧰 Additional context used
🧬 Code graph analysis (2)
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php (2)
subdomains/src/Models/Subdomain.php (2)
Subdomain(24-212)domain(59-62)subdomains/src/Models/CloudflareDomain.php (1)
CloudflareDomain(16-54)
subdomains/src/Models/Subdomain.php (2)
subdomains/src/Services/CloudflareService.php (3)
CloudflareService(9-160)upsertDnsRecord(41-120)deleteDnsRecord(138-159)subdomains/src/Providers/SubdomainsPluginProvider.php (1)
boot(23-36)
🪛 GitHub Actions: Pint
subdomains/src/Services/CloudflareService.php
[warning] 1-1: Pint lint issue: braces_position, blank_line...
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php
[error] 89-89: Parse error: syntax error, unexpected token '->', expecting ']' on line 89.
subdomains/src/Models/Subdomain.php
[warning] 1-1: Pint lint issue: blank_line_before_statement, no_whitespace...
🔇 Additional comments (9)
subdomains/src/SubdomainsPlugin.php (1)
50-50: LGTM! Proper internationalization.The notification title is now properly localized using the translation key from the strings file.
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php (1)
82-85: LGTM! Well-implemented ToggleColumn with proper validation.The SRV record toggle includes helpful tooltips and correctly disables when the domain is missing an SRV target, preventing invalid configurations.
subdomains/src/Services/CloudflareService.php (2)
11-39: LGTM! Robust zone ID fetching with proper error handling.The method includes appropriate validation, exception handling, and logging for both request failures and API errors.
138-159: LGTM! Consistent error handling pattern.The delete method follows the same robust error handling pattern as the other methods, with proper validation, logging, and exception handling.
subdomains/lang/en/strings.php (1)
17-65: LGTM! Comprehensive and clear translation keys.The new translation keys provide clear, user-friendly messages for SRV record functionality and Cloudflare operations. The notification messages include helpful context with placeholder variables.
subdomains/src/Models/Subdomain.php (4)
39-57: LGTM! Cleaner lifecycle hook implementation.The refactored hooks consolidate create/update logic into
saving/savedevents, which is more maintainable than separatecreated/updatedhandlers. Thesrv_recordattribute conversion before persistence is well-placed.
74-90: LGTM! Well-implemented record type logic.The accessor and setter properly handle the SRV/A/AAAA record type determination with appropriate null safety checks for the server relationship.
92-187: LGTM! Comprehensive upsert implementation with excellent error handling.The method properly:
- Validates zone ID availability
- Handles SRV and A/AAAA record types with appropriate validation
- Provides clear user notifications for both success and failure cases
- Updates the
cloudflare_idwhen it changes- Uses the translation keys for user-facing messages
The delegation of SRV target validation to
CloudflareServiceis appropriate.
189-211: LGTM! Correct notification flow with early return.The delete method properly handles success and failure cases with the
returnstatement on line 202 ensuring only one notification is sent. The notification types and translation keys are appropriate for delete operations.
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php
Outdated
Show resolved
Hide resolved
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
subdomains/lang/en/strings.php (1)
2-2: Fix the whitespace on the blank line to resolve the Pint violation.The pipeline failure indicates that line 2 contains whitespace characters (spaces or tabs) on what should be a completely blank line. Laravel Pint requires blank lines to be entirely empty.
🔎 Proposed fix
Remove any spaces or tabs from line 2 so it is completely empty:
<?php - + return [(Ensure line 2 has zero characters, not even spaces or tabs.)
🧹 Nitpick comments (2)
subdomains/src/Models/Subdomain.php (2)
79-90: Add type hint for the$valueparameter.The method logic correctly determines
record_typebased on the boolean value and IP version. However, the$valueparameter lacks a type hint.🔎 Proposed improvement
- public function setRecordType($value): void + public function setRecordType(bool $value): void {
92-199: Consider decomposing this method for improved maintainability.The method is comprehensive and handles all validation and notification flows correctly. However, at 107 lines, it could benefit from extraction into smaller methods (e.g.,
validateAndUpsertSrvRecord(),validateAndUpsertIpRecord()) to improve readability and testability.💡 Example decomposition approach
protected function upsertOnCloudflare(): void { $service = app(CloudflareService::class); $zoneId = $this->validateZoneId(); if (!$zoneId) { return; } if ($this->record_type === 'SRV') { $this->upsertSrvRecord($service, $zoneId); } else { $this->upsertIpRecord($service, $zoneId); } } protected function validateZoneId(): ?string { // lines 96-107 } protected function upsertSrvRecord(CloudflareService $service, string $zoneId): void { // lines 110-160 } protected function upsertIpRecord(CloudflareService $service, string $zoneId): void { // lines 162-198 }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
subdomains/lang/en/strings.phpsubdomains/src/Models/Subdomain.php
🧰 Additional context used
🧬 Code graph analysis (1)
subdomains/src/Models/Subdomain.php (3)
subdomains/src/Services/CloudflareService.php (3)
CloudflareService(9-162)upsertDnsRecord(43-120)deleteDnsRecord(138-161)subdomains/src/Models/CloudflareDomain.php (1)
boot(24-48)subdomains/src/Providers/SubdomainsPluginProvider.php (1)
boot(23-36)
🪛 GitHub Actions: Pint
subdomains/lang/en/strings.php
[error] 1-1: Laravel Pint: no_whitespace_in_blank_line
🔇 Additional comments (4)
subdomains/src/Models/Subdomain.php (4)
32-37: LGTM: Virtual attribute handling is well-designed.The
srv_recordattribute is correctly configured as fillable and cast to boolean, enabling clean form handling. Thesavinghook (lines 43-48) properly removes it before database persistence after deriving the actualrecord_type.
43-52: LGTM: Lifecycle hooks follow Laravel best practices.The
savinghook correctly transforms the virtualsrv_recordattribute into the persistentrecord_type, and thesavedhook ensures Cloudflare synchronization happens after database persistence. Usingarray_key_existsproperly detects when the attribute was explicitly set.
74-77: LGTM: Accessor provides clean boolean interface.The
getSrvRecordAttribute()accessor correctly derives the booleansrv_recordvalue from the storedrecord_type, completing the virtual attribute pattern.
201-224: LGTM: Delete handling is clean and correct.The method properly validates prerequisites, delegates to
CloudflareService, and sends appropriate success or failure notifications. The past issue with duplicate notifications has been resolved.
|
So, if I understand correctly you can assign a SRV target to a cloudflare domain? And that will be used to create a minecraft SRV record? The two things that bother me here:
|
|
Yes 100% on both points at this time:
|
|
Then we should change that.
|
Boy132
left a comment
Boy132
left a comment
There was a problem hiding this comment.
- The SRV target should be configurable per node.
- The service name should be entered by the user. And/or use egg tags.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
subdomains/src/Models/Subdomain.php (1)
69-72: Add null check for$this->domainto prevent potential NPE.
getLabel()accesses$this->domain->namewithout checking if the domain relationship is loaded. If the domain is null or not loaded, this will throw a null pointer exception.🔎 Proposed fix
public function getLabel(): string|Htmlable|null { - return $this->name . '.' . $this->domain->name; + return $this->name . '.' . ($this->domain->name ?? 'unknown'); }
🧹 Nitpick comments (2)
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php (1)
123-128: Consider caching the domain lookup for better performance.The
disabled()callback queries the database viaCloudflareDomain::find()on each form render. While acceptable for admin UIs, you could optimize by using the form's relationship data if available.🔎 Optional optimization using relationship
Toggle::make('srv_record') ->label(trans('subdomains::strings.srv_record')) ->helperText(trans('subdomains::strings.srv_record_help')) ->reactive() - ->disabled(fn (callable $get) => (empty($get('domain_id')) || empty(CloudflareDomain::find($get('domain_id'))?->srv_target))) + ->disabled(function (callable $get) { + $domainId = $get('domain_id'); + if (empty($domainId)) { + return true; + } + return empty(CloudflareDomain::find($domainId)?->srv_target); + }) ->default(false),Alternatively, if the relationship is already eager-loaded, access it through the record context.
subdomains/src/Models/Subdomain.php (1)
220-223: Remove unnecessaryreturnstatement.The
returnon line 222 is at the end of the method and serves no purpose since execution would naturally end there anyway.🔎 Proposed fix
} else { Notification::make() ->danger() ->title(trans('subdomains::strings.notifications.cloudflare_delete_failed_title')) ->body(trans('subdomains::strings.notifications.cloudflare_delete_failed', ['subdomain' => $this->name . '.' . ($this->domain->name ?? 'unknown'), 'errors' => json_encode($result['errors'] ?? $result['body'] ?? [])])) ->send(); } - - return; } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
subdomains/database/migrations/005_add_srv_target_to_nodes.phpsubdomains/lang/en/strings.phpsubdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.phpsubdomains/src/Models/Subdomain.php
🚧 Files skipped from review as they are similar to previous changes (1)
- subdomains/lang/en/strings.php
🧰 Additional context used
🧬 Code graph analysis (2)
subdomains/src/Models/Subdomain.php (1)
subdomains/src/Services/CloudflareService.php (3)
CloudflareService(9-162)upsertDnsRecord(43-120)deleteDnsRecord(138-161)
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php (2)
subdomains/src/Models/Subdomain.php (2)
Subdomain(24-225)domain(59-62)subdomains/src/Models/CloudflareDomain.php (1)
CloudflareDomain(16-54)
🪛 GitHub Actions: Pint
subdomains/database/migrations/005_add_srv_target_to_nodes.php
[error] 1-1: Laravel Pint style check failed in 005_add_srv_target_to_nodes.php. See the Pint output for details.
🔇 Additional comments (3)
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php (1)
82-85: Well-implemented SRV toggle with proper guards.The
ToggleColumncorrectly disables itself when the related domain is missingsrv_target, preventing invalid configurations. The tooltip provides helpful context to users.subdomains/src/Models/Subdomain.php (1)
39-57: Clean lifecycle hook implementation with CloudflareService integration.The refactored
boot()method properly handles thesrv_recordvirtual attribute conversion in thesavinghook and delegates Cloudflare operations to the dedicated service. The use ofupdateQuietly()inupsertOnCloudflare()correctly avoids recursive event triggers.subdomains/database/migrations/005_add_srv_target_to_nodes.php (1)
1-6: Verify specific Pint violations before making changes.The migration file appears properly formatted according to Laravel Pint standards (consistent indentation, proper spacing, valid method signatures). The file does not require
declare(strict_types=1)as Laravel migrations typically don't mandate it. If the pipeline indicates a Pint failure, run./vendor/bin/pint --testto identify the specific rule violation(s) before applying fixes.
… egg tags (Only minecraft at this time)
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
subdomains/src/Models/Subdomain.php (1)
91-106: Add null check for$this->domainbefore accessing properties.Line 95 accesses
$this->domain->cloudflare_idwithout first verifying that$this->domainis not null. If the relationship isn't loaded or doesn't exist, this will throw a null property access error.🔎 Proposed fix
protected function upsertOnCloudflare(): void { $registrar = app(CloudflareService::class); + if (!$this->domain) { + Log::warning('Domain relationship not loaded for subdomain', ['subdomain_id' => $this->id]); + return; + } + $zoneId = $this->domain->cloudflare_id;
🧹 Nitpick comments (5)
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php (1)
124-128: Consider eager-loading the node relationship.The SRV toggle implementation correctly prevents invalid configurations. However,
Filament::getTenant()->node->srv_targetmay trigger N+1 queries if the node relationship isn't eager-loaded on the tenant (Server). Consider adding->with('node')to the tenant query if not already present.subdomains/src/Enums/ServiceRecordType.php (1)
49-61: Add bounds checking for robust parsing.The
service()andprotocol()methods assume the enum value always contains exactly two dot-separated parts. While this works for the currentminecraftcase, future enum cases with different formats could cause undefined array key errors.🔎 Proposed fix with bounds checking
public function service(): string { $parts = explode('.', $this->value); + + if (count($parts) < 2) { + throw new \InvalidArgumentException("Invalid service record format: {$this->value}"); + } return $parts[0]; } public function protocol(): string { $parts = explode('.', $this->value); + + if (count($parts) < 2) { + throw new \InvalidArgumentException("Invalid service record format: {$this->value}"); + } return $parts[1]; }subdomains/src/Filament/Admin/Nodes/Pages/EditNode.php (2)
44-45: MissingThrowableimport for docblock reference.The docblock at line 44 references
@throws Throwable, butThrowableis not imported. While this won't cause a runtime error (it's just documentation), it's inconsistent. Either importThrowableor use the fully qualified name\Throwable.🔎 Proposed fix
+use Throwable; class EditNode extends BaseEditNode { /** - * @throws Throwable + * @throws \Throwable */
368-397: Side effects indehydrateStateUsingcallback may cause unexpected behavior.The
dehydrateStateUsingcallback performs multiple side effects:
- Directly saves the model with
forceFill()->save()- Sends a notification
- Dispatches jobs for all linked servers
This callback runs during form dehydration, which may execute multiple times or at unexpected moments. Consider moving this logic to a dedicated
afterSavehook or action handler for more predictable behavior.Additionally, line 390 has a minor log message issue - "Updating subdomains for Node ID " should include the ID in the message string rather than just in the context array for better log readability.
🔎 Suggested improvement for log message
- Log::info('Updating subdomains for Node ID ', ['node_id' => $this->record->id, 'serverIds' => $serverIds]); + Log::info('Updating subdomains for Node ID ' . $this->record->id, ['node_id' => $this->record->id, 'serverIds' => $serverIds]);subdomains/src/Models/Subdomain.php (1)
233-234: Unnecessaryreturnstatement.The
returnstatement at line 233 is redundant since it's at the end of the method and there's no code after the if block.🔎 Proposed fix
} else { Notification::make() ->danger() ->title(trans('subdomains::strings.notifications.cloudflare_delete_failed_title')) ->body(trans('subdomains::strings.notifications.cloudflare_delete_failed', ['subdomain' => $this->name . '.' . ($this->domain->name ?? 'unknown'), 'errors' => json_encode($result['errors'] ?? $result['body'] ?? [])])) ->send(); } - - return; } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
subdomains/database/migrations/005_add_srv_target_to_nodes.phpsubdomains/lang/en/strings.phpsubdomains/src/Enums/ServiceRecordType.phpsubdomains/src/Filament/Admin/Nodes/Pages/EditNode.phpsubdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.phpsubdomains/src/Models/CloudflareDomain.phpsubdomains/src/Models/Subdomain.phpsubdomains/src/Providers/SubdomainsPluginProvider.phpsubdomains/src/Services/CloudflareService.php
🚧 Files skipped from review as they are similar to previous changes (1)
- subdomains/src/Models/CloudflareDomain.php
🧰 Additional context used
🧬 Code graph analysis (3)
subdomains/src/Enums/ServiceRecordType.php (1)
subdomains/src/Models/Subdomain.php (1)
server(62-65)
subdomains/src/Providers/SubdomainsPluginProvider.php (1)
subdomains/src/Filament/Admin/Nodes/Pages/EditNode.php (1)
EditNode(41-790)
subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php (2)
subdomains/src/Models/Subdomain.php (2)
Subdomain(25-236)server(62-65)subdomains/src/Models/CloudflareDomain.php (1)
CloudflareDomain(16-53)
🔇 Additional comments (19)
subdomains/src/Providers/SubdomainsPluginProvider.php (1)
5-5: LGTM!The custom page registration for NodeResource follows the established pattern and correctly wires the EditNode page for node editing functionality.
Also applies to: 9-9, 18-18
subdomains/database/migrations/005_add_srv_target_to_nodes.php (1)
9-18: LGTM!The migration correctly moves
srv_targetfromcloudflare_domainstonodes. Since both migrations 004 and 005 are in the same PR and haven't been deployed yet, there's no data loss concern.subdomains/src/Filament/Server/Resources/Subdomains/SubdomainResource.php (3)
81-85: LGTM!The
record_typecolumn provides excellent visual feedback when SRV records are misconfigured (missingsrv_target), helping users identify and resolve configuration issues.
88-91: Success notifications suppressed.The
.successNotification(null)calls suppress Filament's default success messages. Ensure that custom notifications (likely from CloudflareService operations) are shown to users so they receive appropriate feedback on their actions.Also applies to: 102-102
114-114: LGTM!The domain suffix display improves UX by showing the full subdomain format. The direct query is acceptable in this form context.
subdomains/src/Enums/ServiceRecordType.php (2)
18-46: LGTM!The tag-based detection logic correctly maps server egg tags to service record types. The implementation returns the first matching case, which is appropriate for the current single-service scenario.
13-16: LGTM!The
getLabel()implementation appropriately converts the enum name to a human-readable format.subdomains/lang/en/strings.php (1)
8-27: LGTM!The translation keys are comprehensive, well-organized, and provide clear user messaging for SRV record functionality and Cloudflare operations. Previous phrasing inconsistencies have been resolved.
Also applies to: 32-67
subdomains/src/Services/CloudflareService.php (3)
11-41: LGTM!The
getZoneIdmethod has proper input validation, exception handling, and response parsing. The error handling and logging are appropriate.
43-120: LGTM with minor observation.The
upsertDnsRecordmethod is well-structured with clear separation between SRV and standard record handling. Exception handling and logging are comprehensive.Note: The method signature accepts 6 parameters which is at the higher end. Consider grouping related parameters into a DTO/value object in a future refactor if the signature grows further.
138-161: LGTM!The
deleteDnsRecordmethod properly validates inputs, handles exceptions, and returns consistent response structures.subdomains/src/Filament/Admin/Nodes/Pages/EditNode.php (2)
61-105: LGTM!The overview tab properly displays node system information with appropriate state derivation and responsive column layouts.
645-786: LGTM!The diagnostics tab has comprehensive pull/upload/clear functionality with proper error handling, notifications, and state management.
subdomains/src/Models/Subdomain.php (6)
27-42: LGTM!The fillables, casts, and appends are properly configured for the new
srv_recordattribute.
44-55: LGTM!Using
static::saved()instead of separatecreatedandupdatedhooks correctly consolidates the upsert logic.
72-75: LGTM!The
getSrvRecordAttributeaccessor correctly derives the boolean from therecord_typefield.
108-171: LGTM!The SRV record handling is comprehensive with proper validation for port, service record type, and srv_target. Error notifications provide clear feedback to users.
173-210: LGTM!The A/AAAA record handling properly validates the server allocation and IP address, with appropriate error handling and notifications.
212-235: LGTM!The
deleteOnCloudflaremethod correctly handles success/failure cases with proper conditional branching. The previous issues with duplicate notifications and missing else clause have been addressed.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
subdomains/src/Models/Subdomain.php (1)
136-149: Optional: Consider null-safe operators for defensive coding.While line 136 validates that
srv_targetis not empty (ensuring node is not null), line 149 accesses$this->server->node->srv_targetwithout null-safe operators. For consistency and defensive programming, consider:$result = $registrar->upsertDnsRecord($zoneId, $recordName, 'SRV', $this->server->node->srv_target, $this->cloudflare_id, $port);Could be:
$target = $this->server->node->srv_target; // already validated $result = $registrar->upsertDnsRecord($zoneId, $recordName, 'SRV', $target, $this->cloudflare_id, $port);This makes the validated value explicit and slightly more readable.
subdomains/src/Filament/Admin/Nodes/Pages/EditNode.php (3)
618-629: Log caught exceptions for debugging.The exception is caught but not logged, which makes troubleshooting node connection issues difficult. While the user receives a notification, administrators and developers have no trace of what went wrong.
🔎 Proposed fix
- } catch (Exception) { + } catch (Exception $e) { Notification::make() ->title(trans('admin/node.error_connecting', ['node' => $node->name])) ->body(trans('admin/node.error_connecting_description')) ->color('warning') ->icon('tabler-database') ->warning() ->send(); - + + report($e); }
690-693: Add timeout to external HTTP request.The HTTP POST to
logs.pelican.devlacks a timeout configuration. If the external service is slow or unresponsive, this could cause the admin panel request to hang, degrading user experience.🔎 Proposed fix
try { $response = Http::asMultipart() + ->timeout(30) ->attach('c', $get('log')) ->attach('e', '14d') ->post('https://logs.pelican.dev');
724-729: Log caught exceptions for debugging.Similar to the earlier exception handling issue, this exception is caught but not logged, making it difficult to diagnose upload failures beyond the user-facing notification.
🔎 Proposed fix
- } catch (\Exception $e) { + } catch (\Exception $e) { Notification::make() ->title(trans('admin/node.diagnostics.upload_failed')) ->body($e->getMessage()) ->danger() ->send(); + + report($e); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
subdomains/src/Filament/Admin/Nodes/Pages/EditNode.phpsubdomains/src/Models/Subdomain.phpsubdomains/src/Services/CloudflareService.php
🚧 Files skipped from review as they are similar to previous changes (1)
- subdomains/src/Services/CloudflareService.php
🧰 Additional context used
🧬 Code graph analysis (1)
subdomains/src/Models/Subdomain.php (2)
subdomains/src/Services/CloudflareService.php (3)
CloudflareService(9-162)upsertDnsRecord(43-120)deleteDnsRecord(138-161)subdomains/src/Enums/ServiceRecordType.php (3)
fromServer(30-35)service(49-54)protocol(56-61)
🔇 Additional comments (7)
subdomains/src/Models/Subdomain.php (6)
6-13: LGTM: Clean import additions.All new imports are properly used throughout the refactored methods.
27-42: LGTM: Proper attribute configuration.The
srv_recordvirtual attribute is correctly configured with fillable, cast, and appends declarations.
44-55: LGTM: Consolidated lifecycle hooks.Using the
savedevent instead of separatecreatedandupdatedhooks is cleaner and ensures consistent Cloudflare synchronization.
72-89: LGTM: Correct accessor/mutator implementation.The IPv6 detection now correctly uses
filter_varwithFILTER_VALIDATE_IPandFILTER_FLAG_IPV6, and null-safe operators prevent potential NPEs.
174-211: LGTM: Clean A/AAAA record handling.The A/AAAA path correctly validates IP addresses (including wildcard addresses), properly calls
upsertDnsRecordwith 6 parameters, and provides clear success/failure notifications.
214-237: LGTM: Correct delete implementation.The delete flow properly validates prerequisites, uses the CloudflareService, and sends appropriate conditional notifications. The previous duplicate notification issue has been resolved.
subdomains/src/Filament/Admin/Nodes/Pages/EditNode.php (1)
367-377: Service name configuration is properly implemented via egg tags.The ServiceRecordType enum correctly determines the service name from the server's egg tags using
fromServer()andfromTags()methods. The system checks$server->egg->tagsand matches them against enum cases, making the service name configurable per server rather than hardcoded. The srv_target field at the node level is appropriately placed and works with this design.
| ->dehydrateStateUsing(function ($state) { | ||
| $this->record->forceFill(['srv_target' => $state])->save(); | ||
|
|
||
| Notification::make() | ||
| ->title(trans('subdomains::strings.notifications.srv_target_updated_title')) | ||
| ->body(trans('subdomains::strings.notifications.srv_target_updated')) | ||
| ->warning() | ||
| ->send(); | ||
|
|
||
| return $state; | ||
| }), |
There was a problem hiding this comment.
Incorrect use of dehydrateStateUsing for side effects.
The dehydrateStateUsing callback is performing direct database saves and sending notifications, which violates Filament's form lifecycle pattern:
- Premature persistence: Calling
forceFill()andsave()here bypasses form validation and saves the field before the rest of the form is processed. If the main form save fails, this field will still be persisted, causing data inconsistency. - Bypassed protection:
forceFill()bypasses mass assignment protection. - Misleading notification: The notification is sent regardless of whether the overall form save succeeds, and uses
->warning()for what appears to be a success message. - Wrong hook:
dehydrateStateUsingis intended for state transformation only, not side effects.
The proper pattern is to let Filament's normal save cycle handle persistence and move the notification to mutateFormDataBeforeSave() or an after-save hook.
🔎 Proposed fix
Remove the side effects from dehydrateStateUsing:
TextInput::make('srv_target')
->label(trans('subdomains::strings.srv_target'))
->placeholder('play.example.com')
->helperText(trans('subdomains::strings.srv_target_help'))
->rules(['nullable', 'string', 'regex:/^(?=.{1,253}$)(?!-)[A-Za-z0-9-]{1,63}(?<!-)(?:\.(?!-)[A-Za-z0-9-]{1,63}(?<!-))*$/'])
- ->columnSpan([
- 'default' => 1,
- 'sm' => 1,
- 'md' => 1,
- 'lg' => 3,
- ])
- ->dehydrateStateUsing(function ($state) {
- $this->record->forceFill(['srv_target' => $state])->save();
-
- Notification::make()
- ->title(trans('subdomains::strings.notifications.srv_target_updated_title'))
- ->body(trans('subdomains::strings.notifications.srv_target_updated'))
- ->warning()
- ->send();
-
- return $state;
- }),
+ ->columnSpan([
+ 'default' => 1,
+ 'sm' => 1,
+ 'md' => 1,
+ 'lg' => 3,
+ ]),Then handle the notification in the parent class's save logic or add a mutateFormDataBeforeSave method to this class if custom notification is needed after successful save.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In subdomains/src/Filament/Admin/Nodes/Pages/EditNode.php around lines 378-388,
remove the direct database and notification side-effects from the
dehydrateStateUsing callback (do not call forceFill()/save() or send
Notification there); keep dehydrateStateUsing only for transforming the field
state (return the transformed $state). Persist the updated srv_target as part of
the normal form save flow instead (either rely on Filament's default save,
implement mutateFormDataBeforeSave to adjust incoming form data, or use an
afterSave/afterCreate hook to perform the save/update), and move the
notification to that after-save hook (use a success notification rather than
->warning()). Also stop using forceFill() so mass-assignment protections are
respected—use the model update/save path provided by the form lifecycle.
| $recordName = sprintf('%s.%s.%s', $serviceRecordType->service(), $serviceRecordType->protocol(), $this->name); | ||
|
|
||
| $result = $registrar->upsertDnsRecord($zoneId, $recordName, 'SRV', $this->server->node->srv_target, $this->cloudflare_id, $port, $serviceRecordType); |
There was a problem hiding this comment.
Critical: Remove extra parameter from upsertDnsRecord call.
Line 149 passes 7 arguments to upsertDnsRecord, but the method signature (from CloudflareService.php) only accepts 6 parameters:
public function upsertDnsRecord(string $zoneId, string $name, string $recordType, string $target, ?string $recordId = null, ?int $port = null): arrayThe 7th parameter $serviceRecordType will cause a runtime error. The service type information is already encoded in $recordName on line 147, so this parameter is unnecessary.
🔎 Proposed fix
$recordName = sprintf('%s.%s.%s', $serviceRecordType->service(), $serviceRecordType->protocol(), $this->name);
- $result = $registrar->upsertDnsRecord($zoneId, $recordName, 'SRV', $this->server->node->srv_target, $this->cloudflare_id, $port, $serviceRecordType);
+ $result = $registrar->upsertDnsRecord($zoneId, $recordName, 'SRV', $this->server->node->srv_target, $this->cloudflare_id, $port);🤖 Prompt for AI Agents
In subdomains/src/Models/Subdomain.php around lines 147 to 149, the call to
$registrar->upsertDnsRecord passes seven arguments but the method signature in
CloudflareService accepts only six; remove the extra $serviceRecordType argument
(the service info is already included in $recordName) so the call uses: zoneId,
recordName, 'SRV', target, cloudflare_id, port.
2 participants
There are some pretty sweeping changes here, but they can be summarized in to the following:
Let me know if there are any issues (honestly never made a PR before and this is my first time with PHP)
Summary by CodeRabbit
New Features
Bug Fixes / UX
Chores
✏️ Tip: You can customize this high-level summary in your review settings.