Make clone action prefix already prefixed routes correctly (#28)

* Add regression test

* Complete prefixing test

* Delete redundant line from test

* Refactore clone action, fix prefixing logic

* Improve test name

* Improve URI string manipulation

* Refactor createNewRoute()

* Fix code style (php-cs-fixer)

* Fix PHPStan error

---------

Co-authored-by: PHP CS Fixer <phpcsfixer@example.com>

src/Actions/CloneRoutesAsTenant.php

@@ -8,6 +8,7 @@ use Closure;
 use Illuminate\Routing\Route;
 use Illuminate\Routing\Router;
 use Illuminate\Support\Collection;
+use Illuminate\Support\Str;
 use Stancl\Tenancy\Enums\RouteMode;
 use Stancl\Tenancy\PathIdentificationManager;
 
@@ -48,9 +49,7 @@ class CloneRoutesAsTenant
 
     public function handle(): void
     {
-        $this->router
-            ->prefix('/{' . PathIdentificationManager::getTenantParameterName() . '}/')
-            ->group(fn () => $this->getRoutesToClone()->each(fn (Route $route) => $this->cloneRoute($route)));
+        $this->getRoutesToClone()->each(fn (Route $route) => $this->cloneRoute($route));
 
         $this->router->getRoutes()->refreshNameLookups();
     }
@@ -145,40 +144,41 @@ class CloneRoutesAsTenant
             return;
         }
 
-        $newRoute = $this->createNewRoute($route);
-
-        if (! tenancy()->routeHasMiddleware($route, 'tenant')) {
-            $newRoute->middleware('tenant');
-        }
-
-        $this->copyMiscRouteProperties($route, $newRoute);
+        $this->copyMiscRouteProperties($route, $this->createNewRoute($route));
     }
 
     protected function createNewRoute(Route $route): Route
     {
         $method = strtolower($route->methods()[0]);
-        $routeName = $route->getName();
+        $uri = $route->getPrefix() ? Str::after($route->uri(), $route->getPrefix()) : $route->uri();
+
+        $newRouteAction = collect($route->action)->tap(function (Collection $action) use ($route) {
+            /** @var array $routeMiddleware */
+            $routeMiddleware = $action->get('middleware') ?? [];
+
+            // Make the new route have the same middleware as the original route
+            // Add the 'tenant' middleware to the new route
+            // Exclude `universal` and `clone` middleware from the new route (it should only be flagged as tenant)
+            $newRouteMiddleware = collect($routeMiddleware)
+                ->merge(['tenant']) // Add 'tenant' flag
+                ->filter(fn (string $middleware) => ! in_array($middleware, ['universal', 'clone']))
+                ->toArray();
+
             $tenantRouteNamePrefix = PathIdentificationManager::getTenantRouteNamePrefix();
 
+            // Make sure the route name has the tenant route name prefix
+            $newRouteNamePrefix = $route->getName()
+                ? $tenantRouteNamePrefix . Str::after($route->getName(), $tenantRouteNamePrefix)
+                : null;
+
+            return $action
+                ->put('as', $newRouteNamePrefix)
+                ->put('middleware', $newRouteMiddleware)
+                ->put('prefix', '/{' . PathIdentificationManager::getTenantParameterName() . '}/' . $route->getPrefix());
+        })->toArray();
+
         /** @var Route $newRoute */
-        $newRoute = $this->router->$method($route->uri(), $route->action);
-
-        // Delete middleware from the new route and
-        // Add original route middleware to ensure there's no duplicate middleware
-        unset($newRoute->action['middleware']);
-
-        // Exclude `universal` and `clone` middleware from the new route -- it should specifically be a tenant route
-        $newRoute->middleware(array_filter(
-            tenancy()->getRouteMiddleware($route),
-            fn (string $middleware) => ! in_array($middleware, ['universal', 'clone'])
-        ));
-
-        if ($routeName && ! $route->named($tenantRouteNamePrefix . '*')) {
-            // Clear the route name so that `name()` sets the route name instead of suffixing it
-            unset($newRoute->action['as']);
-
-            $newRoute->name($tenantRouteNamePrefix . $routeName);
-        }
+        $newRoute = $this->router->$method($uri, $newRouteAction);
 
         return $newRoute;
     }

tests/CloneActionTest.php

@@ -9,6 +9,7 @@ use Illuminate\Support\Facades\Route as RouteFacade;
 use Stancl\Tenancy\Tests\Etc\HasMiddlewareController;
 use Stancl\Tenancy\Middleware\InitializeTenancyByPath;
 use Stancl\Tenancy\Middleware\InitializeTenancyByDomain;
+use Stancl\Tenancy\PathIdentificationManager;
 
 test('a route can be universal using path identification', function (array $routeMiddleware, array $globalMiddleware) {
     foreach ($globalMiddleware as $middleware) {
@@ -244,6 +245,29 @@ test('routes with the clone flag get cloned without making the routes universal'
     pest()->get(route('tenant.' . $routeName, ['tenant' => $tenant]))->assertSee('Tenancy initialized.');
 })->with([InitializeTenancyByPath::class, CustomInitializeTenancyByPath::class]);
 
+test('the clone action prefixes already prefixed routes correctly', function () {
+    RouteFacade::get('/home', fn () => tenant() ? 'Tenancy initialized.' : 'Tenancy not initialized.')
+        ->middleware(['universal', InitializeTenancyByPath::class])
+        ->name($routeName = 'home')
+        ->prefix($prefix = 'prefix');
+
+    app(CloneRoutesAsTenant::class)->handle();
+
+    $clonedRoute = RouteFacade::getRoutes()->getByName($clonedRouteName = 'tenant.' . $routeName);
+
+    $clonedRouteUrl = route($clonedRouteName, ['tenant' => $tenant = Tenant::create()]);
+
+    // The cloned route is prefixed correctly
+    expect($clonedRoute->getPrefix())->toBe('{tenant}/' . $prefix);
+
+    expect($clonedRouteUrl)
+        ->toContain('/' . $tenant->getTenantKey() . '/' . $prefix . '/home')
+        ->not()->toContain($prefix . '/' . $tenant->getTenantKey() . '/' . $prefix . '/home');
+
+    // The cloned route is accessible
+    pest()->get($clonedRouteUrl)->assertSee('Tenancy initialized.');
+});
+
 class CustomInitializeTenancyByPath extends InitializeTenancyByPath
 {