[4.x] Cloning: addTenantParameter(bool), domain(string|null) (#1374)

* Add test for the new clone action addTenantParameter property

* Add $addTenantParameter properyt to the clone action

* Fix code style (php-cs-fixer)

* Update clone action annotation

* Make addTenantParameter(false) sound by adding domain() logic to route cloning

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Samuel Štancl <samuel@archte.ch>

.php-cs-fixer.php

@@ -21,7 +21,6 @@ $rules = [
     'blank_line_before_statement' => [
         'statements' => ['return']
     ],
-    'braces' => true,
     'cast_spaces' => true,
     'class_definition' => true,
     'concat_space' => [

src/Actions/CloneRoutesAsTenant.php

@@ -30,6 +30,11 @@ use Stancl\Tenancy\Resolvers\PathTenantResolver;
  * By providing a callback to shouldClone(), you can change how it's determined if a route should be cloned if you don't want to use middleware flags.
  *
  * Cloned routes are prefixed with '/{tenant}', flagged with 'tenant' middleware, and have their names prefixed with 'tenant.'.
+ *
+ * The addition of the tenant parameter can be controlled using addTenantParameter(true|false). Note that if you decide to disable
+ * tenant parameter addition, the routes MUST differ in domains. This can be controlled using the domain(string|null) method. The
+ * default behavior is to NOT set any domains on cloned routes -- unless specified otherwise using that method.
+ *
  * The parameter name and prefix can be changed e.g. to `/{team}` and `team.` by configuring the path resolver (tenantParameterName and tenantRouteNamePrefix).
  * Routes with names that are already prefixed won't be cloned - but that's just the default behavior.
  * The cloneUsing() method allows you to completely override the default behavior and customize how the cloned routes will be defined.
@@ -43,8 +48,8 @@ use Stancl\Tenancy\Resolvers\PathTenantResolver;
  *
  * Example usage:
  * ```
- * Route::get('/foo', fn () => true)->name('foo')->middleware('clone');
- * Route::get('/bar', fn () => true)->name('bar')->middleware('universal');
+ * Route::get('/foo', ...)->name('foo')->middleware('clone');
+ * Route::get('/bar', ...)->name('bar')->middleware('universal');
  *
  * $cloneAction = app(CloneRoutesAsTenant::class);
  *
@@ -54,10 +59,20 @@ use Stancl\Tenancy\Resolvers\PathTenantResolver;
  * // Clone bar route as /{tenant}/bar and name it tenant.bar ('universal' middleware won't be present in the cloned route)
  * $cloneAction->cloneRoutesWithMiddleware(['universal'])->handle();
  *
- * Route::get('/baz', fn () => true)->name('baz');
+ * Route::get('/baz', ...)->name('baz');
  *
  * // Clone baz route as /{tenant}/bar and name it tenant.baz ('universal' middleware won't be present in the cloned route)
  * $cloneAction->cloneRoute('baz')->handle();
+ *
+ * Route::domain('central.localhost')->get('/no-tenant-parameter', ...)->name('no-tenant-parameter')->middleware('clone');
+ *
+ * // Clone baz route as /no-tenant-parameter and name it tenant.no-tenant-parameter (the route won't have the tenant parameter)
+ * // This can be useful with domain identification. Importantly, the original route MUST have a domain set. The domain of the
+ * // cloned route can be customized using domain(string|null). By default, the cloned route will not be scoped to a domain,
+ * // unless a domain() call is used. It's important to keep in mind that:
+ * //   1. When addTenantParameter(false) is used, the paths will be the same, thus domains must differ.
+ * //   2. If the original route (with the same path) has no domain, the cloned route will never be used due to registration order.
+ * $cloneAction->addTenantParameter(false)->cloneRoutesWithMiddleware(['clone'])->cloneRoute('no-tenant-parameter')->handle();
  * ```
  *
  * Calling handle() will also clear the $routesToClone array.
@@ -70,6 +85,8 @@ use Stancl\Tenancy\Resolvers\PathTenantResolver;
 class CloneRoutesAsTenant
 {
     protected array $routesToClone = [];
+    protected bool $addTenantParameter = true;
+    protected string|null $domain = null;
     protected Closure|null $cloneUsing = null; // The callback should accept Route instance or the route name (string)
     protected Closure|null $shouldClone = null;
     protected array $cloneRoutesWithMiddleware = ['clone'];
@@ -78,6 +95,7 @@ class CloneRoutesAsTenant
         protected Router $router,
     ) {}
 
+    /** Clone routes. This resets routesToClone() but not other config. */
     public function handle(): void
     {
         // If no routes were specified using cloneRoute(), get all routes
@@ -102,15 +120,37 @@ class CloneRoutesAsTenant
                 $route = $this->router->getRoutes()->getByName($route);
             }
 
-            $this->copyMiscRouteProperties($route, $this->createNewRoute($route));
+            $this->createNewRoute($route);
         }
 
         // Clean up the routesToClone array after cloning so that subsequent calls aren't affected
         $this->routesToClone = [];
 
         $this->router->getRoutes()->refreshNameLookups();
+        $this->router->getRoutes()->refreshActionLookups();
     }
 
+    /**
+     * Should a tenant parameter be added to the cloned route.
+     *
+     * The name of the parameter is controlled using PathTenantResolver::tenantParameterName().
+     */
+    public function addTenantParameter(bool $addTenantParameter): static
+    {
+        $this->addTenantParameter = $addTenantParameter;
+
+        return $this;
+    }
+
+    /** The domain the cloned route should use. Set to null if it shouldn't be scoped to a domain. */
+    public function domain(string|null $domain): static
+    {
+        $this->domain = $domain;
+
+        return $this;
+    }
+
+    /** Provide a custom callback for cloning routes, instead of the default behavior. */
     public function cloneUsing(Closure|null $cloneUsing): static
     {
         $this->cloneUsing = $cloneUsing;
@@ -118,6 +158,7 @@ class CloneRoutesAsTenant
         return $this;
     }
 
+    /** Specify which middleware should serve as "flags" telling this action to clone those routes. */
     public function cloneRoutesWithMiddleware(array $middleware): static
     {
         $this->cloneRoutesWithMiddleware = $middleware;
@@ -125,6 +166,10 @@ class CloneRoutesAsTenant
         return $this;
     }
 
+    /**
+     * Provide a custom callback for determining whether a route should be cloned.
+     * Overrides the default middleware-based detection.
+     * */
     public function shouldClone(Closure|null $shouldClone): static
     {
         $this->shouldClone = $shouldClone;
@@ -132,6 +177,7 @@ class CloneRoutesAsTenant
         return $this;
     }
 
+    /** Clone an individual route. */
     public function cloneRoute(Route|string $route): static
     {
         $this->routesToClone[] = $route;
@@ -171,28 +217,31 @@ class CloneRoutesAsTenant
             $action->put('as', PathTenantResolver::tenantRouteNamePrefix() . $name);
         }
 
-        $action
-            ->put('middleware', $middleware)
-            ->put('prefix', $prefix . '/{' . PathTenantResolver::tenantParameterName() . '}');
+        if ($this->domain) {
+            $action->put('domain', $this->domain);
+        } elseif ($action->offsetExists('domain')) {
+            $action->offsetUnset('domain');
+        }
+
+        $action->put('middleware', $middleware);
+
+        if ($this->addTenantParameter) {
+            $action->put('prefix', $prefix . '/{' . PathTenantResolver::tenantParameterName() . '}');
+        }
 
         /** @var Route $newRoute */
         $newRoute = $this->router->$method($uri, $action->toArray());
 
-        return $newRoute;
-    }
-
-    /**
-     * Copy misc properties of the original route to the new route.
-     */
-    protected function copyMiscRouteProperties(Route $originalRoute, Route $newRoute): void
-    {
+        // Copy misc properties of the original route to the new route.
         $newRoute
-            ->setBindingFields($originalRoute->bindingFields())
-            ->setFallback($originalRoute->isFallback)
-            ->setWheres($originalRoute->wheres)
-            ->block($originalRoute->locksFor(), $originalRoute->waitsFor())
-            ->withTrashed($originalRoute->allowsTrashedBindings())
-            ->setDefaults($originalRoute->defaults);
+            ->setBindingFields($route->bindingFields())
+            ->setFallback($route->isFallback)
+            ->setWheres($route->wheres)
+            ->block($route->locksFor(), $route->waitsFor())
+            ->withTrashed($route->allowsTrashedBindings())
+            ->setDefaults($route->defaults);
+
+        return $newRoute;
     }
 
     /** Removes top-level cloneRoutesWithMiddleware and adds 'tenant' middleware. */

tests/CloneActionTest.php

@@ -6,6 +6,7 @@ use Stancl\Tenancy\Actions\CloneRoutesAsTenant;
 use Stancl\Tenancy\Resolvers\PathTenantResolver;
 use Illuminate\Support\Facades\Route as RouteFacade;
 use function Stancl\Tenancy\Tests\pest;
+use Illuminate\Routing\Exceptions\UrlGenerationException;
 
 test('CloneRoutesAsTenant action clones routes with clone middleware by default', function () {
     config(['tenancy.identification.resolvers.' . PathTenantResolver::class . '.tenant_parameter_name' => 'team']);
@@ -337,3 +338,54 @@ test('clone action can be used fluently', function() {
     expect(collect(RouteFacade::getRoutes()->get())->map->getName())
         ->toContain('tenant.foo', 'tenant.bar', 'tenant.baz');
 });
+
+test('the cloned route can be scoped to a specified domain', function () {
+    RouteFacade::domain('foo.localhost')->get('/foo', fn () => in_array('tenant', request()->route()->middleware()) ? 'tenant' : 'central')->name('foo')->middleware('clone');
+
+    // Importantly, we CANNOT add a domain to the cloned route *if the original route didn't have a domain*.
+    // This is due to the route registration order - the more strongly scoped route (= route with a domain)
+    // must be registered first, so that Laravel tries that route first and only moves on if the domain check fails.
+    $cloneAction = app(CloneRoutesAsTenant::class);
+    // To keep the test simple we don't even need a tenant parameter
+    $cloneAction->domain('bar.localhost')->addTenantParameter(false)->handle();
+
+    expect(route('foo'))->toBe('http://foo.localhost/foo');
+    expect(route('tenant.foo'))->toBe('http://bar.localhost/foo');
+});
+
+test('tenant parameter addition can be controlled by setting addTenantParameter', function (bool $addTenantParameter) {
+    RouteFacade::domain('central.localhost')
+        ->get('/foo', fn () => in_array('tenant', request()->route()->middleware()) ? 'tenant' : 'central')
+        ->name('foo')
+        ->middleware('clone');
+
+    // By default this action also removes the domain
+    $cloneAction = app(CloneRoutesAsTenant::class);
+    $cloneAction->addTenantParameter($addTenantParameter)->handle();
+
+    $clonedRoute = RouteFacade::getRoutes()->getByName('tenant.foo');
+
+    // We only use the route() helper here, since once a request is made
+    // the URL generator caches the request's domain and it affects route
+    // generation for routes that do not have domain() specified (tenant.foo)
+    expect(route('foo'))->toBe('http://central.localhost/foo');
+    if ($addTenantParameter)
+        expect(route('tenant.foo', ['tenant' => 'abc']))->toBe('http://localhost/abc/foo');
+    else
+        expect(route('tenant.foo'))->toBe('http://localhost/foo');
+
+    // Original route still works
+    $this->withoutExceptionHandling()->get(route('foo'))->assertSee('central');
+
+    if ($addTenantParameter) {
+        expect($clonedRoute->uri())->toContain('{tenant}');
+
+        $this->withoutExceptionHandling()->get('http://localhost/abc/foo')->assertSee('tenant');
+        $this->withoutExceptionHandling()->get('http://central.localhost/foo')->assertSee('central');
+    } else {
+        expect($clonedRoute->uri())->not()->toContain('{tenant}');
+
+        $this->withoutExceptionHandling()->get('http://localhost/foo')->assertSee('tenant');
+        $this->withoutExceptionHandling()->get('http://central.localhost/foo')->assertSee('central');
+    }
+})->with([true, false]);