From 81917b86fb4c2720cf16f96308d8c57cf43d37f1 Mon Sep 17 00:00:00 2001
From: lukinovec <lukinovec@gmail.com>
Date: Mon, 23 Dec 2024 15:08:41 +0100
Subject: [PATCH] Improve TenancyUrlGenerator and RootUrlBootstrapperTest
 clarity

---
 src/Overrides/TenancyUrlGenerator.php         | 31 +++++++++----
 .../Bootstrappers/RootUrlBootstrapperTest.php | 43 +++++++++++--------
 2 files changed, 48 insertions(+), 26 deletions(-)

diff --git a/src/Overrides/TenancyUrlGenerator.php b/src/Overrides/TenancyUrlGenerator.php
index 7c21ce3f..8f7e86fe 100644
--- a/src/Overrides/TenancyUrlGenerator.php
+++ b/src/Overrides/TenancyUrlGenerator.php
@@ -62,18 +62,31 @@ class TenancyUrlGenerator extends UrlGenerator
         $url = parent::route($name, $parameters, $absolute);
 
         if (isset($parameters[PathTenantResolver::tenantParameterName()])) {
-            // Ensure the tenant key is present in the URL just once
-            // This is necessary when using UrlGeneratorBootstrapper with RootUrlBootstrapper
-            $tenantId = $parameters[PathTenantResolver::tenantParameterName()];
-            $afterTenant = str($url)->afterLast($tenantId)->toString();
-            $beforeTenant = str($url)->before($tenantId)->toString();
+            /**
+             * Ensure the tenant key appears in the final URL only once.
+             * This adjustment is necessary when RootUrlBootstrapper is enabled (and used as intended).
+             *
+             * When RootUrlBootstrapper adds the tenant key to the root URL:
+             * - The root URL includes the tenant key (http://localhost/tenantfoo).
+             * - Passing the tenant key as a parameter to `parent::route()` adds it again, causing duplication.
+             *
+             * To fix this:
+             * - For relative URLs: Include only the part AFTER the tenant key.
+             * - For absolute URLs: Rebuild the URL so that the tenant key is included exactly once.
+             */
+            $tenantKey = $parameters[PathTenantResolver::tenantParameterName()];
 
-            if (! $absolute && str(url('/'))->contains($tenantId)) {
-                // If the URL should be relative and the tenant key is already present in the full URL, don't add it again
-                return $afterTenant;
+            // Separate the URL into parts before the first and after the last tenant key
+            $urlBeforeTenantKey = str($url)->before($tenantKey)->toString(); // e.g. "http://localhost/"
+            $urlAfterTenantKey = str($url)->afterLast($tenantKey)->toString(); // e.g. "/home"
+
+            if (! $absolute && str(url('/'))->contains($tenantKey)) {
+                // For relative URLs, return only the part after the tenant key
+                return $urlAfterTenantKey;
             }
 
-            return $beforeTenant . $tenantId . $afterTenant;
+            // Reconstruct the URL with the tenant key appearing exactly once
+            return $urlBeforeTenantKey . $tenantKey . $urlAfterTenantKey;
         }
 
         return $url;
diff --git a/tests/Bootstrappers/RootUrlBootstrapperTest.php b/tests/Bootstrappers/RootUrlBootstrapperTest.php
index 9128307b..2269d520 100644
--- a/tests/Bootstrappers/RootUrlBootstrapperTest.php
+++ b/tests/Bootstrappers/RootUrlBootstrapperTest.php
@@ -72,31 +72,40 @@ test('root url bootstrapper overrides the root url when tenancy gets initialized
 });
 
 test('root url bootstrapper can be used with url generator bootstrapper', function() {
+    /**
+     * Order matters when combining these two bootstrappers.
+     * Before overriding the URL generator's root URL, we need to bind TenancyUrlGenerator.
+     * Otherwise (when using RootUrlBootstrapper BEFORE UrlGeneratorBootstrapper),
+     * the original URL generator's root URL will be changed, and only after that will the TenancyUrlGenerator bound,
+     * ultimately making the root URL override pointless.
+     */
     config(['tenancy.bootstrappers' => [UrlGeneratorBootstrapper::class, RootUrlBootstrapper::class]]);
 
     TenancyUrlGenerator::$prefixRouteNames = true;
     TenancyUrlGenerator::$passTenantParameterToRoutes = true;
+    RootUrlBootstrapper::$rootUrlOverride = fn (Tenant $tenant, string $originalRootUrl) => $originalRootUrl . '/' . $tenant->getTenantKey();
 
-    Route::get('/', function () {
-        return true;
-    })->name('home');
+    Route::get('/home', fn () => 'home')->name('home');
+    Route::get('/{tenant}/home', fn () => 'tenant.home')->name('tenant.home')->middleware(InitializeTenancyByPath::class);
 
-    Route::get('/{tenant}', function () {
-        return true;
-    })->name('tenant.home')->middleware(InitializeTenancyByPath::class);
+    expect(url('/home'))->toBe('http://localhost/home');
 
-    $rootUrlOverride = function (Tenant $tenant) {
-        return 'http://localhost/' . $tenant->getTenantKey();
-    };
+    expect(route('home'))->toBe('http://localhost/home');
+    expect(route('home', absolute: false))->toBe('/home');
 
-    $tenant = Tenant::create(['id' => 'acme']);
+    tenancy()->initialize(Tenant::create(['id' => 'acme']));
 
-    RootUrlBootstrapper::$rootUrlOverride = $rootUrlOverride;
+    // The url() helper should generate the full URL containing the tenant key
+    expect(url('/home'))->toBe('http://localhost/acme/home');
 
-    expect(route('home'))->toBe('http://localhost');
-
-    tenancy()->initialize($tenant);
-
-    expect(route('home'))->toBe('http://localhost/acme');
-    expect(url('/'))->toBe('http://localhost/acme');
+    /**
+     * The absolute path should return the correct absolute path, containing just one tenant key,
+     * and the relative path should still be /home.
+     *
+     * We use string manipulation in the route() method override for this to behave correctly.
+     *
+     * @see TenancyUrlGenerator
+     */
+    expect(route('home'))->toBe('http://localhost/acme/home');
+    expect(route('home', absolute: false))->toBe('/home');
 });