From 53e21abe97078a3d405a334a516c675ed754418e Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Fri, 6 Nov 2026 15:17:35 +0100 Subject: [PATCH] fix(services): do not warn about the port when a domain has no port An empty port routes to the required port, so it needs no confirmation. The edit dialog shows the effective port, the warning buttons name the ports, and domain conflicts ignore the container port. Co-Authored-By: Claude Opus 5.5 --- app/Livewire/Project/Service/Domains.php | 6 ++-- app/Livewire/Project/Service/Index.php | 6 ++-- app/Models/ServiceApplication.php | 22 +++---------- bootstrap/helpers/domains.php | 5 ++- .../project/service/domains.blade.php | 12 +++---- .../livewire/project/service/index.blade.php | 12 +++---- .../service/partials/domain-table.blade.php | 2 +- tests/Feature/ServiceDomainsTest.php | 33 ++++++++++++++++--- 8 files changed, 57 insertions(+), 41 deletions(-) diff --git a/app/Livewire/Project/Service/Domains.php b/app/Livewire/Project/Service/Domains.php index dae2642825..6029e17bce 100644 --- a/app/Livewire/Project/Service/Domains.php +++ b/app/Livewire/Project/Service/Domains.php @@ -112,6 +112,8 @@ class Domains extends Component public ?int $requiredPort = null; + public ?int $requestedPort = null; + public bool $isCheckingDns = false; #[Locked] @@ -1603,10 +1605,10 @@ class Domains extends Component if ($checkPorts && ! $this->forceRemovePort) { $requiredPort = $app->getRequiredPort(); if ($requiredPort !== null && $domainString) { - $previousFqdn = $app->getOriginal('fqdn'); foreach ($this->splitDomains($domainString) as $fqdn) { - if ($app->portRequiresConfirmation($fqdn, $requiredPort, is_string($previousFqdn) ? $previousFqdn : null)) { + if ($app->portRequiresConfirmation($fqdn, $requiredPort)) { $this->requiredPort = $requiredPort; + $this->requestedPort = ServiceApplication::extractPortFromUrl($fqdn); $this->showPortWarningModal = true; $app->refresh(); diff --git a/app/Livewire/Project/Service/Index.php b/app/Livewire/Project/Service/Index.php index edfc3c0e97..43bf885457 100644 --- a/app/Livewire/Project/Service/Index.php +++ b/app/Livewire/Project/Service/Index.php @@ -84,6 +84,8 @@ class Index extends Component public $requiredPort = null; + public ?int $requestedPort = null; + public ?string $fqdn = null; public bool $isGzipEnabled = false; @@ -630,7 +632,6 @@ class Index extends Component $persistedApplication = $this->serviceApplication->fresh(); $previousDnsHostnames = app(ManagedDnsRecordCleanup::class)->hostnamesOf($persistedApplication); $previousServiceDnsHostnames = array_keys(app(ResourceDnsRecords::class)->hostnameUsers($this->service)); - $previousEditableUrls = $persistedApplication->url; $previousFqdn = $persistedApplication->fqdn; $previousPortOverrides = $persistedApplication->domain_port_overrides; $this->authorize('update', $this->serviceApplication); @@ -669,8 +670,9 @@ class Index extends Component continue; } - if ($this->serviceApplication->portRequiresConfirmation($fqdn, $requiredPort, $previousEditableUrls)) { + if ($this->serviceApplication->portRequiresConfirmation($fqdn, $requiredPort)) { $this->requiredPort = $requiredPort; + $this->requestedPort = ServiceApplication::extractPortFromUrl($fqdn); $this->showPortWarningModal = true; $this->serviceApplication->fqdn = $previousFqdn; $this->serviceApplication->domain_port_overrides = $previousPortOverrides; diff --git a/app/Models/ServiceApplication.php b/app/Models/ServiceApplication.php index 5f737459ab..d180d9790a 100644 --- a/app/Models/ServiceApplication.php +++ b/app/Models/ServiceApplication.php @@ -275,7 +275,7 @@ class ServiceApplication extends BaseModel /** * True when saving this URL should confirm that it does not use the required template port. */ - public function portRequiresConfirmation(string $fqdn, ?int $requiredPort, ?string $previousFqdn = null): bool + public function portRequiresConfirmation(string $fqdn, ?int $requiredPort): bool { if ($requiredPort === null) { return false; @@ -286,28 +286,14 @@ class ServiceApplication extends BaseModel return false; } - $canonical = DomainPortOverrides::withoutPort($fqdn); $explicit = self::extractPortFromUrl($fqdn); - if ($explicit === $requiredPort) { + // A domain without a port routes to the required port. + if ($explicit === null || $explicit === $requiredPort) { return false; } - if ($explicit === null) { - $previous = collect(explode(',', (string) $previousFqdn)) - ->filter(); - $previousUrl = $previous->first( - fn (string $url): bool => DomainPortOverrides::withoutPort(trim($url)) === $canonical - ); - - if (is_string($previousUrl) && self::extractPortFromUrl($previousUrl) !== null) { - return true; - } - - return $previousUrl === null; - } - - $existingOverride = $this->domain_port_overrides[$canonical] ?? null; + $existingOverride = $this->domain_port_overrides[DomainPortOverrides::withoutPort($fqdn)] ?? null; return (int) $existingOverride !== $explicit; } diff --git a/bootstrap/helpers/domains.php b/bootstrap/helpers/domains.php index 176a4e1153..488c04b261 100644 --- a/bootstrap/helpers/domains.php +++ b/bootstrap/helpers/domains.php @@ -79,7 +79,10 @@ function domainConflictKey(string $domain): string ? str($domain)->beforeLast('/')->toString() : $domain; - return preg_replace('#^https?://#i', '', $domain) ?? $domain; + $domain = preg_replace('#^https?://#i', '', $domain) ?? $domain; + + // The URL port is the container port. The public address does not include it. + return preg_replace('#^([^/:\[]+|\[[^\]]+\]):\d+#', '$1', $domain) ?? $domain; } /** diff --git a/resources/views/livewire/project/service/domains.blade.php b/resources/views/livewire/project/service/domains.blade.php index c2170ae160..98e34e5f37 100644 --- a/resources/views/livewire/project/service/domains.blade.php +++ b/resources/views/livewire/project/service/domains.blade.php @@ -336,18 +336,18 @@
This service requires port {{ $requiredPort }} to function correctly. - One or more of your domains use a different port, or none. + One or more of your domains use port {{ $requestedPort }}.
+ + Use port {{ $requestedPort }} anyway + - Keep required port - - - Use this port anyway + Keep port {{ $requiredPort }}
diff --git a/resources/views/livewire/project/service/index.blade.php b/resources/views/livewire/project/service/index.blade.php index ff31e43c50..35709eb641 100644 --- a/resources/views/livewire/project/service/index.blade.php +++ b/resources/views/livewire/project/service/index.blade.php @@ -141,7 +141,7 @@
This service requires port {{ $requiredPort }} to function correctly. - One or more of your domains use a different port, or none. + One or more of your domains use port {{ $requestedPort }}. @@ -154,13 +154,13 @@
- - Keep required port - - Use this port anyway + Use port {{ $requestedPort }} anyway + + + Keep port {{ $requiredPort }}
diff --git a/resources/views/livewire/project/service/partials/domain-table.blade.php b/resources/views/livewire/project/service/partials/domain-table.blade.php index 1e8496e985..922ea2b41e 100644 --- a/resources/views/livewire/project/service/partials/domain-table.blade.php +++ b/resources/views/livewire/project/service/partials/domain-table.blade.php @@ -56,7 +56,7 @@ $faviconUrl = $isSuggested || $serviceApplication?->hasProxyBasicAuth() ? null : \App\Support\DomainFavicon::url($publicUrl, $row['dns_status'] ?? null, $row['expected_ip'] ?? null); $domainKey = hash('sha256', $row['url'].'|'.($row['service_application_id'] ?? '')); $editingParts = \App\Support\DomainUrlParts::split($row['url']); - if ($row['has_port_override'] ?? false) { + if (filled($row['internal_port'] ?? null)) { $editingParts['port'] = (string) $row['internal_port']; } @endphp diff --git a/tests/Feature/ServiceDomainsTest.php b/tests/Feature/ServiceDomainsTest.php index 4aca098a61..a3addc19e0 100644 --- a/tests/Feature/ServiceDomainsTest.php +++ b/tests/Feature/ServiceDomainsTest.php @@ -755,7 +755,7 @@ YAML, ->assertDispatched('success'); }); -it('saves after confirming both a domain conflict and a missing required port', function () { +it('saves after confirming both a domain conflict and a different port', function () { $this->service->update([ 'docker_compose_raw' => <<<'YAML' services: @@ -770,7 +770,9 @@ YAML, $component = Livewire::test(Domains::class, ['service' => $this->service->fresh(['applications', 'server'])]) ->set('newServiceApplicationId', $this->webApp->id) - ->set('newDomain', 'https://api.example.com') + ->set('newDomainParts.host', 'api.example.com') + ->set('newDomainParts.port', '3000') + ->set('newDomainPartsChanged', true) ->call('addDomain') ->assertSet('showDomainConflictModal', true) ->call('confirmDomainUsage') @@ -778,8 +780,8 @@ YAML, ->assertSet('showPortWarningModal', true) ->assertSet('forceSaveDomains', true) ->assertSee('Use a different port?') - ->assertSee('Keep required port') - ->assertSee('Use this port anyway'); + ->assertSee('Keep port 8000') + ->assertSee('Use port 3000 anyway'); $component ->call('confirmRemovePort') @@ -790,7 +792,28 @@ YAML, ->assertDispatched('success'); expect(explode(',', (string) $this->webApp->fresh()->fqdn)) - ->toBe(['https://api.example.com']); + ->toBe(['https://api.example.com']) + ->and($this->webApp->fresh()->domain_port_overrides)->toBe(['https://api.example.com' => 3000]); +}); + +it('does not warn about the port when a domain with an empty port is renamed', function () { + $this->service->update([ + 'docker_compose_raw' => "services:\n api:\n image: node:alpine\n environment:\n - SERVICE_URL_API_8080\n web:\n image: nginx:alpine\n", + ]); + $this->apiApp->update(['fqdn' => 'https://api.example.com']); + + Livewire::test(Domains::class, ['service' => $this->service->fresh(['applications', 'server'])]) + ->call('startEdit', 0) + ->set('editingDomainParts.host', 'renamed.example.com') + ->set('editingDomainParts.port', '') + ->set('editingDomainPartsChanged', true) + ->call('updateDomain') + ->assertSet('showPortWarningModal', false) + ->assertDispatched('success'); + + $this->apiApp->refresh(); + expect($this->apiApp->fqdn)->toBe('https://renamed.example.com') + ->and($this->apiApp->domain_port_overrides)->toBeNull(); }); it('loads persisted dns status for service applications', function () {