From 948754cadc19b495c2cc612a307b228aa2eb641d Mon Sep 17 00:00:00 2001 From: lukinovec Date: Mon, 7 Aug 2023 18:32:13 +0200 Subject: [PATCH] Clone routes action improvements (#4) * Use "clone" instead of "re-register" in UniversalRouteTest * Fix `$shouldCloneRoute` condition, improve tenant flagging logic * Use clone instead of reregister in stub --- assets/TenancyServiceProvider.stub.php | 14 +++++++------- src/Actions/CloneRoutesAsTenant.php | 17 ++++++++++------- tests/UniversalRouteTest.php | 14 +++++++------- 3 files changed, 24 insertions(+), 21 deletions(-) diff --git a/assets/TenancyServiceProvider.stub.php b/assets/TenancyServiceProvider.stub.php index 10f069be..eba11d52 100644 --- a/assets/TenancyServiceProvider.stub.php +++ b/assets/TenancyServiceProvider.stub.php @@ -171,12 +171,12 @@ class TenancyServiceProvider extends ServiceProvider if (InitializeTenancyByPath::inGlobalStack()) { TenancyUrlGenerator::$prefixRouteNames = true; - /** @var CloneRoutesAsTenant $reregisterRoutes */ - $reregisterRoutes = app(CloneRoutesAsTenant::class); + /** @var CloneRoutesAsTenant $cloneRoutes */ + $cloneRoutes = app(CloneRoutesAsTenant::class); /** - * You can provide a closure for re-registering a specific route, e.g.: - * $reregisterRoutes->reregisterUsing('welcome', function () { + * You can provide a closure for cloning a specific route, e.g.: + * $cloneRoutes->cloneUsing('welcome', function () { * Route::get('/tenant-welcome', fn () => 'Current tenant: ' . tenant()->getTenantKey()) * ->middleware(['universal', InitializeTenancyByPath::class]) * ->name('tenant.welcome'); @@ -185,17 +185,17 @@ class TenancyServiceProvider extends ServiceProvider * To make Livewire v2 (2.12.2+) work with kernel path identification, * use this closure to override the livewire.message-localized route: * - * $reregisterRoutes->reregisterUsing('livewire.message-localized', function (Route $route) { + * $cloneRoutes->cloneUsing('livewire.message-localized', function (Route $route) { * $route->setUri(str($route->uri())->replaceFirst('locale', $tenantParameter = PathTenantResolver::tenantParameterName())); * $route->parameterNames[0] = $tenantParameter; * $route->middleware('tenant'); * }); * - * To see the default behavior of re-registering the universal routes, check out the reregisterRoute() method in ReregisterRoutesAsTenant. + * To see the default behavior of re-registering the universal routes, check out the cloneRoute() method in CloneRoutesAsTenant. * @see CloneRoutesAsTenant */ - $reregisterRoutes->handle(); + $cloneRoutes->handle(); } } diff --git a/src/Actions/CloneRoutesAsTenant.php b/src/Actions/CloneRoutesAsTenant.php index a391d199..9428db18 100644 --- a/src/Actions/CloneRoutesAsTenant.php +++ b/src/Actions/CloneRoutesAsTenant.php @@ -105,24 +105,27 @@ class CloneRoutesAsTenant } $routesAreUniversalByDefault = $this->config->get('tenancy.default_route_mode') === RouteMode::UNIVERSAL; - $routeHasIdentificationMiddleware = tenancy()->routeHasIdentificationMiddleware($route); $routeHasPathIdentification = PathIdentificationManager::pathIdentificationOnRoute($route); $pathIdentificationMiddlewareInGlobalStack = PathIdentificationManager::pathIdentificationInGlobalStack(); + $routeHasNonPathIdentificationMiddleware = tenancy()->routeHasIdentificationMiddleware($route) && ! $routeHasPathIdentification; // Determine if the passed route should get cloned // The route should be cloned if it has path identification middleware // Or if the route doesn't have identification middleware and path identification middleware // Is not used globally or the routes are universal by default - $shouldCloneRoute = $routeHasPathIdentification || - (! $routeHasIdentificationMiddleware && ($routesAreUniversalByDefault || $pathIdentificationMiddlewareInGlobalStack)); + $shouldCloneRoute = ! $routeHasNonPathIdentificationMiddleware && + ($routesAreUniversalByDefault || $routeHasPathIdentification || $pathIdentificationMiddlewareInGlobalStack); if ($shouldCloneRoute) { $newRoute = $this->createNewRoute($route); - $routeIsUniversal = tenancy()->routeHasMiddleware($newRoute, 'universal'); + $routeConsideredUniversal = tenancy()->routeHasMiddleware($newRoute, 'universal') || $routesAreUniversalByDefault; - // Add the 'tenant' flag to the new route if the route is universal - // Or if it isn't universal and it doesn't have the identification middlware (= it isn't "flagged" as tenant by having the MW) - if ((! $routeHasPathIdentification && ! $routeIsUniversal) || $routeIsUniversal || $routesAreUniversalByDefault) { + if ($routeHasPathIdentification && ! $routeConsideredUniversal && ! tenancy()->routeHasMiddleware($newRoute, 'tenant')) { + // Skip adding tenant flag + // Non-universal routes with identification middleware are already considered tenant + // Also skip adding the flag if the route already has the flag + // So that the route only has the 'tenant' middleware group once + } else { $newRoute->middleware('tenant'); } diff --git a/tests/UniversalRouteTest.php b/tests/UniversalRouteTest.php index 892247f0..c44f9c3e 100644 --- a/tests/UniversalRouteTest.php +++ b/tests/UniversalRouteTest.php @@ -321,7 +321,7 @@ test('ReregisterRoutesAsTenant registers prefixed duplicates of universal routes config(['tenancy.identification.resolvers.' . PathTenantResolver::class . '.tenant_parameter_name' => $tenantParameterName = 'team']); config(['tenancy.identification.resolvers.' . PathTenantResolver::class . '.tenant_route_name_prefix' => $tenantRouteNamePrefix = 'team-route.']); - // Test that routes with controllers as well as routes with closure actions get re-registered correctly + // Test that routes with controllers as well as routes with closure actions get cloned correctly $universalRoute = RouteFacade::get('/home', $useController ? Controller::class : fn () => tenant() ? 'Tenancy initialized.' : 'Tenancy not initialized.')->middleware($routeMiddleware)->name('home'); $centralRoute = RouteFacade::get('/central', fn () => true)->name('central'); @@ -372,16 +372,16 @@ test('tenant resolver methods return the correct names for configured values', f ['tenant_route_name_prefix', 'prefix'] ]); -test('ReregisterRoutesAsTenant only re-registers routes with path identification by default', function () { +test('CloneRoutesAsTenant only clones routes with path identification by default', function () { app(Kernel::class)->pushMiddleware(InitializeTenancyByPath::class); $currentRouteCount = fn () => count(RouteFacade::getRoutes()->get()); $initialRouteCount = $currentRouteCount(); - // Path identification is used globally, and this route doesn't use a specific identification middleware, meaning path identification is used and the route should get re-registered + // Path identification is used globally, and this route doesn't use a specific identification middleware, meaning path identification is used and the route should get cloned RouteFacade::get('/home', fn () => tenant() ? 'Tenancy initialized.' : 'Tenancy not initialized.')->middleware('universal')->name('home'); - // The route uses a specific identification middleware other than InitializeTenancyByPath – the route shouldn't get re-registered + // The route uses a specific identification middleware other than InitializeTenancyByPath – the route shouldn't get cloned RouteFacade::get('/home-domain-id', fn () => tenant() ? 'Tenancy initialized.' : 'Tenancy not initialized.')->middleware(['universal', InitializeTenancyByDomain::class])->name('home-domain-id'); expect($currentRouteCount())->toBe($newRouteCount = $initialRouteCount + 2); @@ -391,7 +391,7 @@ test('ReregisterRoutesAsTenant only re-registers routes with path identification $reregisterRoutesAction->handle(); - // Only one of the two routes gets re-registered + // Only one of the two routes gets cloned expect($currentRouteCount())->toBe($newRouteCount + 1); }); @@ -403,7 +403,7 @@ test('custom callbacks can be used for reregistering universal routes', function $currentRouteCount = fn () => count(RouteFacade::getRoutes()->get()); $initialRouteCount = $currentRouteCount(); - // Skip re-registering the 'home' route + // Skip cloning the 'home' route $reregisterRoutesAction->cloneUsing($routeName, function (Route $route) { return; })->handle(); @@ -427,7 +427,7 @@ test('reregistration of specific routes can get skipped', function () { $currentRouteCount = fn () => count(RouteFacade::getRoutes()->get()); $initialRouteCount = $currentRouteCount(); - // Skip re-registering the 'home' route + // Skip cloning the 'home' route $reregisterRoutesAction->skipRoute($routeName)->handle(); // Expect route count to stay the same because the 'home' route re-registration gets skipped