From ea50cf797443155dbffe6239468be2be8fb20cee Mon Sep 17 00:00:00 2001 From: Voltra Date: Sat, 2 Dec 2023 14:23:44 +0000 Subject: [PATCH 01/25] Initial draft of the new route caching system --- src/FilamentFabricatorServiceProvider.php | 25 +-- src/Http/Controllers/PageController.php | 11 +- src/Models/Concerns/HandlesPageUrls.php | 62 ++++++++ src/Models/Contracts/HasPageUrls.php | 32 ++++ src/Models/Contracts/Page.php | 2 +- src/Models/Page.php | 11 +- src/Observers/PageRoutesObserver.php | 55 +++++++ src/Services/PageRoutesService.php | 177 ++++++++++++++++++++++ 8 files changed, 350 insertions(+), 25 deletions(-) create mode 100644 src/Models/Concerns/HandlesPageUrls.php create mode 100644 src/Models/Contracts/HasPageUrls.php create mode 100644 src/Observers/PageRoutesObserver.php create mode 100644 src/Services/PageRoutesService.php diff --git a/src/FilamentFabricatorServiceProvider.php b/src/FilamentFabricatorServiceProvider.php index 8a3ea61..b09eb45 100644 --- a/src/FilamentFabricatorServiceProvider.php +++ b/src/FilamentFabricatorServiceProvider.php @@ -12,7 +12,9 @@ use Symfony\Component\Finder\SplFileInfo; use Z3d0X\FilamentFabricator\Facades\FilamentFabricator; use Z3d0X\FilamentFabricator\Layouts\Layout; +use Z3d0X\FilamentFabricator\Observers\PageRoutesObserver; use Z3d0X\FilamentFabricator\PageBlocks\PageBlock; +use Z3d0X\FilamentFabricator\Services\PageRoutesService; class FilamentFabricatorServiceProvider extends PackageServiceProvider { @@ -69,17 +71,11 @@ public function packageRegistered(): void public function bootingPackage(): void { Route::bind('filamentFabricatorPage', function ($value) { - $pageModel = FilamentFabricator::getPageModel(); - - $pageUrls = FilamentFabricator::getPageUrls(); - - $value = Str::start($value, '/'); - - $pageId = array_search($value, $pageUrls); - - return $pageModel::query() - ->where('id', $pageId) - ->firstOrFail(); + /** + * @var PageRoutesService $routesService + */ + $routesService = resolve(PageRoutesService::class); + return $routesService->findPageOrFail($value); }); $this->registerComponentsFromDirectory( @@ -97,6 +93,13 @@ public function bootingPackage(): void ); } + public function packageBooted() + { + parent::packageBooted(); + + FilamentFabricator::getPageModel()::observe(PageRoutesObserver::class); + } + protected function registerComponentsFromDirectory(string $baseClass, array $register, ?string $directory, ?string $namespace): void { if (blank($directory) || blank($namespace)) { diff --git a/src/Http/Controllers/PageController.php b/src/Http/Controllers/PageController.php index 58ccf3e..d38ee95 100644 --- a/src/Http/Controllers/PageController.php +++ b/src/Http/Controllers/PageController.php @@ -13,14 +13,13 @@ public function __invoke(Page $filamentFabricatorPage = null): string { // Handle root (home) page if (blank($filamentFabricatorPage)) { - $pageUrls = FilamentFabricator::getPageUrls(); - - $pageId = array_search('/', $pageUrls); + /** + * @var PageRoutesService $routesService + */ + $routesService = resolve(PageRoutesService::class); /** @var Page $filamentFabricatorPage */ - $filamentFabricatorPage = FilamentFabricator::getPageModel()::query() - ->where('id', $pageId) - ->firstOrFail(); + $filamentFabricatorPage = $routesService->findPageOrFail('/'); } /** @var ?class-string $layout */ diff --git a/src/Models/Concerns/HandlesPageUrls.php b/src/Models/Concerns/HandlesPageUrls.php new file mode 100644 index 0000000..cfecc53 --- /dev/null +++ b/src/Models/Concerns/HandlesPageUrls.php @@ -0,0 +1,62 @@ +getDefaultUrlArgs())->merge($args)->all(); + $id = $this->id; + return "filament-fabricator::page-url--$id"; + } + + /** + * Get the URL determined by this entity and the provided arguments + */ + public function getUrl(array $args = []): string { + $cacheKey = $this->getUrlCacheKey($args); + + return Cache::rememberForever($cacheKey, function () use($args) { + /** + * @var ?Page $parent + */ + $parent = $this->parent; + $parentUri = is_null($parent) ? '/' : $parent->getUrl($args); + $parentUri = Str::start($parentUri, '/'); + + $selfUri = $this->slug; + $selfUri = Str::start($selfUri, '/'); + return $parentUri === '/' ? $selfUri : "{$parentUri}{$selfUri}"; + }); + } + + /** + * Get all the available argument sets for the available cache keys + * @return array[] + */ + public function getAllCacheKeyArgs(): array { + return [ + $this->getDefaultUrlCacheArgs(), + ]; + } + + /** + * Get all the available URLs for this entity + * @return string[] + */ + public function getAllUrls(): array { + return array_map([$this, 'getUrl'], $this->getAllUrlCacheKeysArgs()); + } +} diff --git a/src/Models/Contracts/HasPageUrls.php b/src/Models/Contracts/HasPageUrls.php new file mode 100644 index 0000000..bbd2dda --- /dev/null +++ b/src/Models/Contracts/HasPageUrls.php @@ -0,0 +1,32 @@ +table)) { @@ -32,12 +35,6 @@ public function __construct(array $attributes = []) 'parent_id' => 'integer', ]; - protected static function booted() - { - static::saved(fn () => Cache::forget('filament-fabricator::page-urls')); - static::deleted(fn () => Cache::forget('filament-fabricator::page-urls')); - } - public function parent(): BelongsTo { return $this->belongsTo(static::class, 'parent_id'); @@ -50,7 +47,7 @@ public function children(): HasMany public function allChildren(): HasMany { - return $this->hasMany(static::class, 'parent_id') + return $this->children() ->select('id', 'slug', 'title', 'parent_id') ->with('allChildren:id,slug,title,parent_id'); } diff --git a/src/Observers/PageRoutesObserver.php b/src/Observers/PageRoutesObserver.php new file mode 100644 index 0000000..c73685f --- /dev/null +++ b/src/Observers/PageRoutesObserver.php @@ -0,0 +1,55 @@ +pageRoutesService->updateUrlsOf($page); + } + + /** + * Handle the Page "updated" event. + */ + public function updated(Page $page): void + { + $this->pageRoutesService->updateUrlsOf($page); + } + + /** + * Handle the Page "deleted" event. + */ + public function deleted(Page $page): void + { + //TODO: implement this + } + + /** + * Handle the Page "restored" event. + */ + public function restored(Page $page): void + { + $this->pageRoutesService->updateUrlsOf($page); + } + + /** + * Handle the Page "force deleted" event. + */ + public function forceDeleted(Page $page): void + { + //TODO: implement this + } +} \ No newline at end of file diff --git a/src/Services/PageRoutesService.php b/src/Services/PageRoutesService.php new file mode 100644 index 0000000..3ac33b4 --- /dev/null +++ b/src/Services/PageRoutesService.php @@ -0,0 +1,177 @@ +getUriToIdMapping(); + $uri = Str::start($uri, '/'); + return $mapping[$uri] ?? -1; + } + + /** + * Get an instance of your Page model from a URI, or NULL if none matches + * @param string $uri + * @return ?Page + */ + public function getPageFromUri(string $uri): ?Page + { + $id = $this->getPageIdFromUri($uri); + return FilamentFabricator::getPageModel()::find($id); + } + + /** + * Update the cached URLs for the given page (as well as all its descendants') + * @param Page $page + */ + public function updateUrlsOf(Page $page): void + { + $mapping = $this->getUriToIdMapping(); + $this->updateUrlsAndDescendantsOf($page, $mapping); + $this->replaceUriToIdMapping($mapping); + } + + /** + * Get an instance of your Page model from a URI, or throw if there is none + * @param string $uri + * @return Page + */ + public function findPageOrFail(string $uri): Page { + $id = $this->getPageIdFromUri($uri); + return FilamentFabricator::getPageModel()::findOrFail($id); + } + + /** + * Get the URI -> ID mapping + * @return array + */ + protected function getUriToIdMapping(): array + { + return Cache::rememberForever(static::URI_TO_ID_MAPPING, function () { + $idsToUris = $this->getIdToUrisMapping(); + $mapping = []; + + foreach ($idsToUris as $id => $uris) { + foreach ($uris as $uri) { + $mapping[$uri] = $id; + } + } + + return $mapping; + }); + } + + /** + * Get the ID -> URI[] mapping + * @return array + */ + protected function getIdToUrisMapping(): array + { + return Cache::rememberForever(static::ID_TO_URI_MAPPING, function () { + $pages = FilamentFabricator::getPageModel()::all(); + $mapping = []; + $pages->each(function (Page $page) use (&$mapping) { + $mapping[$page->id] = $page->getAllUrls(); + }); + + return $mapping; + }); + } + + /** + * Get the cached URIs for the given page + * @param Page $page + * @return string[] + */ + protected function getUrisForPage(Page $page): array + { + $mapping = $this->getIdToUrisMapping(); + return $mapping[$page->id] ?? []; + } + + /** + * Update routine for the given page + * @param Page $page + * @param array $mapping - The URI -> ID mapping (as a reference, to be modified in-place) + * @return void + */ + protected function updateUrlsAndDescendantsOf(Page $page, array &$mapping) + { + $this->unsetOldUrlsOf($page, $mapping); + $urls = $page->getAllUrls(); + + foreach ($urls as $uri) { + $id = $mapping[$uri] ?? -1; + + if ($id === $page->id) { + continue; + } + + unset($mapping[$uri]); + $mapping[$uri] = $page->id; + } + + // $page->load(['children:id,slug']); + foreach ($page->children as $childPage) { + $this->updateUrlsAndDescendantsOf($childPage, $mapping); + } + } + + /** + * Remove old URLs of the given page from the cached mappings + * @param Page $page + * @param array $mapping - The URI -> ID mapping (as a reference, to be modified in-place) + * @return void + */ + protected function unsetOldUrlsOf(Page $page, array &$mapping) + { + $oldUrlSet = collect($this->getUrisForPage($page))->lazy()->sort()->all(); + $allUrlSet = collect($page->getAllUrls())->lazy()->sort()->all(); + + $oldUrls = array_diff($oldUrlSet, $allUrlSet); + + foreach ($oldUrls as $oldUrl) { + unset($mapping[$oldUrl]); + } + + $idToUrlsMapping = $this->getIdToUrisMapping(); + $idToUrlsMapping[$page->id] = $allUrlSet; + $this->replaceIdToUriMapping($idToUrlsMapping); + } + + /** + * Completely replaced the cached ID -> URI[] mapping + * @param array $mapping + * @return void + */ + protected function replaceIdToUriMapping(array $mapping): void + { + Cache::forget(static::ID_TO_URI_MAPPING); + Cache::rememberForever(static::ID_TO_URI_MAPPING, fn() => $mapping); + } + + /** + * Completely replace the cached URI -> ID mapping + * @param array $mapping + * @return void + */ + protected function replaceUriToIdMapping(array $mapping): void + { + Cache::forget(static::URI_TO_ID_MAPPING); + Cache::rememberForever(static::URI_TO_ID_MAPPING, fn() => $mapping); + } +} From 7a7fcceb33bb86e8acf53ab0d1d4d565807a3d34 Mon Sep 17 00:00:00 2001 From: Voltra Date: Sat, 2 Dec 2023 14:32:37 +0000 Subject: [PATCH 02/25] Avoid nested update event triggers --- src/Services/PageRoutesService.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/Services/PageRoutesService.php b/src/Services/PageRoutesService.php index 3ac33b4..606d53d 100644 --- a/src/Services/PageRoutesService.php +++ b/src/Services/PageRoutesService.php @@ -40,9 +40,11 @@ public function getPageFromUri(string $uri): ?Page */ public function updateUrlsOf(Page $page): void { - $mapping = $this->getUriToIdMapping(); - $this->updateUrlsAndDescendantsOf($page, $mapping); - $this->replaceUriToIdMapping($mapping); + FilamentFabricator::getPageModel()::withoutEvents(function() use($page) { + $mapping = $this->getUriToIdMapping(); + $this->updateUrlsAndDescendantsOf($page, $mapping); + $this->replaceUriToIdMapping($mapping); + }); } /** From 91cf440c8a6f195c8e6db052aefbfe1f73d5cff8 Mon Sep 17 00:00:00 2001 From: Voltra Date: Sat, 2 Dec 2023 14:37:13 +0000 Subject: [PATCH 03/25] Fix imports --- src/Models/Concerns/HandlesPageUrls.php | 2 +- src/Models/Page.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Models/Concerns/HandlesPageUrls.php b/src/Models/Concerns/HandlesPageUrls.php index cfecc53..1bc4c70 100644 --- a/src/Models/Concerns/HandlesPageUrls.php +++ b/src/Models/Concerns/HandlesPageUrls.php @@ -1,6 +1,6 @@ Date: Sun, 3 Dec 2023 14:47:47 +0000 Subject: [PATCH 04/25] Remove all traces of the previous route caching system --- src/FilamentFabricatorManager.php | 43 ++++++++---------------------- src/Services/PageRoutesService.php | 9 +++++++ 2 files changed, 20 insertions(+), 32 deletions(-) diff --git a/src/FilamentFabricatorManager.php b/src/FilamentFabricatorManager.php index bc771f7..7b45059 100644 --- a/src/FilamentFabricatorManager.php +++ b/src/FilamentFabricatorManager.php @@ -10,6 +10,7 @@ use Z3d0X\FilamentFabricator\Models\Contracts\Page as PageContract; use Z3d0X\FilamentFabricator\Models\Page; use Z3d0X\FilamentFabricator\PageBlocks\PageBlock; +use Z3d0X\FilamentFabricator\Services\PageRoutesService; class FilamentFabricatorManager { @@ -33,7 +34,7 @@ class FilamentFabricatorManager protected array $pageUrls = []; - public function __construct() + public function __construct(protected PageRoutesService $routesService) { /** @var Collection */ $pageBlocks = collect([]); @@ -172,44 +173,22 @@ public function getRoutingPrefix(): ?string return null; } - return Str::start(config('filament-fabricator.routing.prefix'), '/'); - } + $prefix = Str::start($prefix, '/'); - public function getPageUrls(): array - { - return Cache::rememberForever('filament-fabricator::page-urls', function () { - $this->getPageModel()::query() - ->select('id', 'slug', 'title') - ->whereNull('parent_id') - ->with(['allChildren']) - ->get() - ->each(fn (PageContract $page) => $this->setPageUrl($page)); // @phpstan-ignore-line + if ($prefix === '/') { + return $prefix; + } - return $this->pageUrls; - }); + return rtrim($prefix, '/'); } - public function getPageUrlFromId(int $id, bool $prefixSlash = false): ?string + public function getPageUrls(): array { - $url = $this->getPageUrls()[$id]; - - if ($routingPrefix = $this->getRoutingPrefix()) { - $url = Str::start($url, $routingPrefix); - } - - return $url; + return $this->routesService->getAllUrls(); } - protected function setPageUrl(PageContract $page, string $parentUrl = null): string + public function getPageUrlFromId(int $id, bool $prefixSlash = false, array $args = []): ?string { - $pageUrl = $parentUrl ? $parentUrl . '/' . trim($page->slug, " \n\r\t\v\x00/") : trim($page->slug); - - if (filled($page->allChildren)) { - foreach ($page->allChildren as $child) { - $this->setPageUrl($child, $pageUrl); - } - } - - return $this->pageUrls[$page->id] = Str::start($pageUrl, '/'); + return $this->getPageModel()::query()->find($id)?->getUrl($args); } } diff --git a/src/Services/PageRoutesService.php b/src/Services/PageRoutesService.php index 606d53d..ccc8ee7 100644 --- a/src/Services/PageRoutesService.php +++ b/src/Services/PageRoutesService.php @@ -57,6 +57,15 @@ public function findPageOrFail(string $uri): Page { return FilamentFabricator::getPageModel()::findOrFail($id); } + /** + * Get the list of all the registered URLs + * @return string[] + */ + public function getAllUrls(): array { + $mapping = $this->getUriToIdMapping(); + return array_values(array_keys($mapping)); + } + /** * Get the URI -> ID mapping * @return array From 7b8e4446bb9b76d07c1df5b0f2430e0225cb92cb Mon Sep 17 00:00:00 2001 From: Voltra Date: Sun, 3 Dec 2023 14:48:02 +0000 Subject: [PATCH 05/25] Use the routing prefix when generating URLs --- src/Models/Concerns/HandlesPageUrls.php | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/Models/Concerns/HandlesPageUrls.php b/src/Models/Concerns/HandlesPageUrls.php index 1bc4c70..558ccfc 100644 --- a/src/Models/Concerns/HandlesPageUrls.php +++ b/src/Models/Concerns/HandlesPageUrls.php @@ -4,6 +4,7 @@ use Illuminate\Support\Facades\Cache; use Illuminate\Support\Str; +use Z3d0X\FilamentFabricator\Facades\FilamentFabricator; trait HandlesPageUrls { /** @@ -28,17 +29,26 @@ public function getUrlCacheKey(array $args = []): string { public function getUrl(array $args = []): string { $cacheKey = $this->getUrlCacheKey($args); + //TODO: Clear and re-compute cached routes when the routing prefix changes + return Cache::rememberForever($cacheKey, function () use($args) { /** * @var ?Page $parent */ $parent = $this->parent; - $parentUri = is_null($parent) ? '/' : $parent->getUrl($args); + $parentUri = is_null($parent) ? (FilamentFabricator::getRoutingPrefix() ?? '/') : $parent->getUrl($args); $parentUri = Str::start($parentUri, '/'); $selfUri = $this->slug; $selfUri = Str::start($selfUri, '/'); - return $parentUri === '/' ? $selfUri : "{$parentUri}{$selfUri}"; + + if ($parentUri === '/') { + return $selfUri; + } + + $parentUri = rtrim($parentUri, '/'); + + return "{$parentUri}{$selfUri}"; }); } From 8d925b9b815cf685c809c5dbe009cca551cbd6bd Mon Sep 17 00:00:00 2001 From: Voltra Date: Sun, 3 Dec 2023 14:50:24 +0000 Subject: [PATCH 06/25] Fix manager DI resolution --- src/FilamentFabricatorServiceProvider.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/FilamentFabricatorServiceProvider.php b/src/FilamentFabricatorServiceProvider.php index b09eb45..ec008a6 100644 --- a/src/FilamentFabricatorServiceProvider.php +++ b/src/FilamentFabricatorServiceProvider.php @@ -64,7 +64,7 @@ public function packageRegistered(): void parent::packageRegistered(); $this->app->scoped('filament-fabricator', function () { - return new FilamentFabricatorManager(); + return resolve(FilamentFabricatorManager::class); }); } From 0852c1096b5cfb14b91df110268d6b01525470e9 Mon Sep 17 00:00:00 2001 From: Voltra Date: Wed, 6 Dec 2023 19:02:35 +0000 Subject: [PATCH 07/25] Fix typo in method names --- src/Models/Concerns/HandlesPageUrls.php | 2 +- src/Models/Contracts/HasPageUrls.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Models/Concerns/HandlesPageUrls.php b/src/Models/Concerns/HandlesPageUrls.php index 558ccfc..5009e40 100644 --- a/src/Models/Concerns/HandlesPageUrls.php +++ b/src/Models/Concerns/HandlesPageUrls.php @@ -56,7 +56,7 @@ public function getUrl(array $args = []): string { * Get all the available argument sets for the available cache keys * @return array[] */ - public function getAllCacheKeyArgs(): array { + public function getAllUrlCacheKeysArgs(): array { return [ $this->getDefaultUrlCacheArgs(), ]; diff --git a/src/Models/Contracts/HasPageUrls.php b/src/Models/Contracts/HasPageUrls.php index bbd2dda..3adecce 100644 --- a/src/Models/Contracts/HasPageUrls.php +++ b/src/Models/Contracts/HasPageUrls.php @@ -22,7 +22,7 @@ public function getUrl(array $args = []): string; * Get all the available argument sets for the available cache keys * @return array[] */ - public function getAllCacheKeyArgs(): array; + public function getAllUrlCacheKeysArgs(): array; /** * Get all the available URLs for this entity From 809ff7e09d3055087a1667c218eedc91dbafed9a Mon Sep 17 00:00:00 2001 From: Voltra Date: Wed, 6 Dec 2023 19:08:39 +0000 Subject: [PATCH 08/25] Fix cache update to make it more portable across drivers --- src/Services/PageRoutesService.php | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/Services/PageRoutesService.php b/src/Services/PageRoutesService.php index ccc8ee7..929a855 100644 --- a/src/Services/PageRoutesService.php +++ b/src/Services/PageRoutesService.php @@ -171,8 +171,7 @@ protected function unsetOldUrlsOf(Page $page, array &$mapping) */ protected function replaceIdToUriMapping(array $mapping): void { - Cache::forget(static::ID_TO_URI_MAPPING); - Cache::rememberForever(static::ID_TO_URI_MAPPING, fn() => $mapping); + Cache::forever(static::ID_TO_URI_MAPPING, $mapping); } /** @@ -182,7 +181,6 @@ protected function replaceIdToUriMapping(array $mapping): void */ protected function replaceUriToIdMapping(array $mapping): void { - Cache::forget(static::URI_TO_ID_MAPPING); - Cache::rememberForever(static::URI_TO_ID_MAPPING, fn() => $mapping); + Cache::forever(static::URI_TO_ID_MAPPING, $mapping); } } From 205b1ea8ef258f34096eb00b7276ce5f3deaa0aa Mon Sep 17 00:00:00 2001 From: ZedoX Date: Fri, 12 Jan 2024 01:12:35 +0800 Subject: [PATCH 09/25] fix: phpstan --- src/Facades/FilamentFabricator.php | 5 +++-- src/FilamentFabricatorManager.php | 6 ++++-- src/Http/Controllers/PageController.php | 1 + src/Models/Concerns/HandlesPageUrls.php | 1 + src/Services/PageRoutesService.php | 17 +++++++++++++---- 5 files changed, 22 insertions(+), 8 deletions(-) diff --git a/src/Facades/FilamentFabricator.php b/src/Facades/FilamentFabricator.php index a6106ca..9f214cf 100644 --- a/src/Facades/FilamentFabricator.php +++ b/src/Facades/FilamentFabricator.php @@ -2,6 +2,7 @@ namespace Z3d0X\FilamentFabricator\Facades; +use Illuminate\Database\Eloquent\Model; use Illuminate\Support\Facades\Facade; use Z3d0X\FilamentFabricator\Models\Contracts\Page as PageContract; @@ -25,8 +26,8 @@ * @method static array getScripts() * @method static array getStyles() * @method static ?string getFavicon() - * @method static class-string getPageModel() - * @method static string getRoutingPrefix() + * @method static class-string getPageModel() + * @method static ?string getRoutingPrefix() * @method static array getPageUrls() * @method static ?string getPageUrlFromId(int $id, bool $prefixSlash = false) * diff --git a/src/FilamentFabricatorManager.php b/src/FilamentFabricatorManager.php index 7b45059..a9d43e3 100644 --- a/src/FilamentFabricatorManager.php +++ b/src/FilamentFabricatorManager.php @@ -4,7 +4,6 @@ use Closure; use Illuminate\Support\Collection; -use Illuminate\Support\Facades\Cache; use Illuminate\Support\Str; use Z3d0X\FilamentFabricator\Layouts\Layout; use Z3d0X\FilamentFabricator\Models\Contracts\Page as PageContract; @@ -189,6 +188,9 @@ public function getPageUrls(): array public function getPageUrlFromId(int $id, bool $prefixSlash = false, array $args = []): ?string { - return $this->getPageModel()::query()->find($id)?->getUrl($args); + /** @var ?PageContract $page */ + $page = $this->getPageModel()::query()->find($id); + + return $page?->getUrl($args); } } diff --git a/src/Http/Controllers/PageController.php b/src/Http/Controllers/PageController.php index d38ee95..4d8072a 100644 --- a/src/Http/Controllers/PageController.php +++ b/src/Http/Controllers/PageController.php @@ -6,6 +6,7 @@ use Z3d0X\FilamentFabricator\Facades\FilamentFabricator; use Z3d0X\FilamentFabricator\Layouts\Layout; use Z3d0X\FilamentFabricator\Models\Contracts\Page; +use Z3d0X\FilamentFabricator\Services\PageRoutesService; class PageController { diff --git a/src/Models/Concerns/HandlesPageUrls.php b/src/Models/Concerns/HandlesPageUrls.php index 5009e40..20c5e99 100644 --- a/src/Models/Concerns/HandlesPageUrls.php +++ b/src/Models/Concerns/HandlesPageUrls.php @@ -5,6 +5,7 @@ use Illuminate\Support\Facades\Cache; use Illuminate\Support\Str; use Z3d0X\FilamentFabricator\Facades\FilamentFabricator; +use Z3d0X\FilamentFabricator\Models\Contracts\Page; trait HandlesPageUrls { /** diff --git a/src/Services/PageRoutesService.php b/src/Services/PageRoutesService.php index 929a855..688a388 100644 --- a/src/Services/PageRoutesService.php +++ b/src/Services/PageRoutesService.php @@ -2,10 +2,11 @@ namespace Z3d0X\FilamentFabricator\Services; -use Z3d0X\FilamentFabricator\Facades\FilamentFabricator; -use Z3d0X\FilamentFabricator\Models\Contracts\Page; +use Illuminate\Database\Eloquent\Model; use Illuminate\Support\Facades\Cache; use Illuminate\Support\Str; +use Z3d0X\FilamentFabricator\Facades\FilamentFabricator; +use Z3d0X\FilamentFabricator\Models\Contracts\Page; class PageRoutesService { protected const URI_TO_ID_MAPPING = 'filament-fabricator::PageRoutesService::uri-to-id'; @@ -31,6 +32,12 @@ public function getPageIdFromUri(string $uri): int public function getPageFromUri(string $uri): ?Page { $id = $this->getPageIdFromUri($uri); + + if ($id === -1) { + return null; + } + + /** @var Page&Model */ return FilamentFabricator::getPageModel()::find($id); } @@ -50,10 +57,12 @@ public function updateUrlsOf(Page $page): void /** * Get an instance of your Page model from a URI, or throw if there is none * @param string $uri - * @return Page + * @return Page&Model */ - public function findPageOrFail(string $uri): Page { + public function findPageOrFail(string $uri): Page&Model { $id = $this->getPageIdFromUri($uri); + + /** @var Page&Model */ return FilamentFabricator::getPageModel()::findOrFail($id); } From c355b927e0aebbb85ef0c66cf89cccd260083890 Mon Sep 17 00:00:00 2001 From: ZedoX Date: Fri, 12 Jan 2024 01:13:57 +0800 Subject: [PATCH 10/25] fix: codestyle --- src/FilamentFabricatorServiceProvider.php | 1 + src/Models/Concerns/HandlesPageUrls.php | 23 +++++++++---- src/Models/Contracts/HasPageUrls.php | 5 ++- src/Models/Page.php | 1 - src/Observers/PageRoutesObserver.php | 5 ++- src/Services/PageRoutesService.php | 39 ++++++++++++----------- 6 files changed, 43 insertions(+), 31 deletions(-) diff --git a/src/FilamentFabricatorServiceProvider.php b/src/FilamentFabricatorServiceProvider.php index ec008a6..d39a2e6 100644 --- a/src/FilamentFabricatorServiceProvider.php +++ b/src/FilamentFabricatorServiceProvider.php @@ -75,6 +75,7 @@ public function bootingPackage(): void * @var PageRoutesService $routesService */ $routesService = resolve(PageRoutesService::class); + return $routesService->findPageOrFail($value); }); diff --git a/src/Models/Concerns/HandlesPageUrls.php b/src/Models/Concerns/HandlesPageUrls.php index 20c5e99..d2fdb9b 100644 --- a/src/Models/Concerns/HandlesPageUrls.php +++ b/src/Models/Concerns/HandlesPageUrls.php @@ -7,32 +7,37 @@ use Z3d0X\FilamentFabricator\Facades\FilamentFabricator; use Z3d0X\FilamentFabricator\Models\Contracts\Page; -trait HandlesPageUrls { +trait HandlesPageUrls +{ /** * Get the default arguments for URL generation */ - public function getDefaultUrlCacheArgs(): array { + public function getDefaultUrlCacheArgs(): array + { return []; } /** * Get the cache key for the URL determined by this entity and the provided arguments */ - public function getUrlCacheKey(array $args = []): string { + public function getUrlCacheKey(array $args = []): string + { // $keyArgs = collect($this->getDefaultUrlArgs())->merge($args)->all(); $id = $this->id; + return "filament-fabricator::page-url--$id"; } /** * Get the URL determined by this entity and the provided arguments */ - public function getUrl(array $args = []): string { + public function getUrl(array $args = []): string + { $cacheKey = $this->getUrlCacheKey($args); //TODO: Clear and re-compute cached routes when the routing prefix changes - return Cache::rememberForever($cacheKey, function () use($args) { + return Cache::rememberForever($cacheKey, function () use ($args) { /** * @var ?Page $parent */ @@ -55,9 +60,11 @@ public function getUrl(array $args = []): string { /** * Get all the available argument sets for the available cache keys + * * @return array[] */ - public function getAllUrlCacheKeysArgs(): array { + public function getAllUrlCacheKeysArgs(): array + { return [ $this->getDefaultUrlCacheArgs(), ]; @@ -65,9 +72,11 @@ public function getAllUrlCacheKeysArgs(): array { /** * Get all the available URLs for this entity + * * @return string[] */ - public function getAllUrls(): array { + public function getAllUrls(): array + { return array_map([$this, 'getUrl'], $this->getAllUrlCacheKeysArgs()); } } diff --git a/src/Models/Contracts/HasPageUrls.php b/src/Models/Contracts/HasPageUrls.php index 3adecce..1d6570b 100644 --- a/src/Models/Contracts/HasPageUrls.php +++ b/src/Models/Contracts/HasPageUrls.php @@ -2,7 +2,8 @@ namespace Z3d0X\FilamentFabricator\Models\Contracts; -interface HasPageUrls { +interface HasPageUrls +{ /** * Get the default arguments for URL generation */ @@ -20,12 +21,14 @@ public function getUrl(array $args = []): string; /** * Get all the available argument sets for the available cache keys + * * @return array[] */ public function getAllUrlCacheKeysArgs(): array; /** * Get all the available URLs for this entity + * * @return string[] */ public function getAllUrls(): array; diff --git a/src/Models/Page.php b/src/Models/Page.php index ab2b019..86da31d 100644 --- a/src/Models/Page.php +++ b/src/Models/Page.php @@ -5,7 +5,6 @@ use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Relations\BelongsTo; use Illuminate\Database\Eloquent\Relations\HasMany; -use Illuminate\Support\Facades\Cache; use Z3d0X\FilamentFabricator\Models\Concerns\HandlesPageUrls; use Z3d0X\FilamentFabricator\Models\Contracts\Page as Contract; diff --git a/src/Observers/PageRoutesObserver.php b/src/Observers/PageRoutesObserver.php index c73685f..b9bc81c 100644 --- a/src/Observers/PageRoutesObserver.php +++ b/src/Observers/PageRoutesObserver.php @@ -9,8 +9,7 @@ class PageRoutesObserver { public function __construct( protected PageRoutesService $pageRoutesService - ) - { + ) { } /** @@ -52,4 +51,4 @@ public function forceDeleted(Page $page): void { //TODO: implement this } -} \ No newline at end of file +} diff --git a/src/Services/PageRoutesService.php b/src/Services/PageRoutesService.php index 688a388..d6e4998 100644 --- a/src/Services/PageRoutesService.php +++ b/src/Services/PageRoutesService.php @@ -8,25 +8,26 @@ use Z3d0X\FilamentFabricator\Facades\FilamentFabricator; use Z3d0X\FilamentFabricator\Models\Contracts\Page; -class PageRoutesService { +class PageRoutesService +{ protected const URI_TO_ID_MAPPING = 'filament-fabricator::PageRoutesService::uri-to-id'; + protected const ID_TO_URI_MAPPING = 'filament-fabricator::PageRoutesService::id-to-uri'; /** * Get the ID of the Page model to which the given URI is associated, -1 if non matches - * @param string $uri - * @return int */ public function getPageIdFromUri(string $uri): int { $mapping = $this->getUriToIdMapping(); $uri = Str::start($uri, '/'); + return $mapping[$uri] ?? -1; } /** * Get an instance of your Page model from a URI, or NULL if none matches - * @param string $uri + * * @return ?Page */ public function getPageFromUri(string $uri): ?Page @@ -43,11 +44,10 @@ public function getPageFromUri(string $uri): ?Page /** * Update the cached URLs for the given page (as well as all its descendants') - * @param Page $page */ public function updateUrlsOf(Page $page): void { - FilamentFabricator::getPageModel()::withoutEvents(function() use($page) { + FilamentFabricator::getPageModel()::withoutEvents(function () use ($page) { $mapping = $this->getUriToIdMapping(); $this->updateUrlsAndDescendantsOf($page, $mapping); $this->replaceUriToIdMapping($mapping); @@ -56,10 +56,9 @@ public function updateUrlsOf(Page $page): void /** * Get an instance of your Page model from a URI, or throw if there is none - * @param string $uri - * @return Page&Model */ - public function findPageOrFail(string $uri): Page&Model { + public function findPageOrFail(string $uri): Page&Model + { $id = $this->getPageIdFromUri($uri); /** @var Page&Model */ @@ -68,15 +67,19 @@ public function findPageOrFail(string $uri): Page&Model { /** * Get the list of all the registered URLs + * * @return string[] */ - public function getAllUrls(): array { + public function getAllUrls(): array + { $mapping = $this->getUriToIdMapping(); + return array_values(array_keys($mapping)); } /** * Get the URI -> ID mapping + * * @return array */ protected function getUriToIdMapping(): array @@ -97,6 +100,7 @@ protected function getUriToIdMapping(): array /** * Get the ID -> URI[] mapping + * * @return array */ protected function getIdToUrisMapping(): array @@ -114,19 +118,20 @@ protected function getIdToUrisMapping(): array /** * Get the cached URIs for the given page - * @param Page $page + * * @return string[] */ protected function getUrisForPage(Page $page): array { $mapping = $this->getIdToUrisMapping(); + return $mapping[$page->id] ?? []; } /** * Update routine for the given page - * @param Page $page - * @param array $mapping - The URI -> ID mapping (as a reference, to be modified in-place) + * + * @param array $mapping - The URI -> ID mapping (as a reference, to be modified in-place) * @return void */ protected function updateUrlsAndDescendantsOf(Page $page, array &$mapping) @@ -153,8 +158,8 @@ protected function updateUrlsAndDescendantsOf(Page $page, array &$mapping) /** * Remove old URLs of the given page from the cached mappings - * @param Page $page - * @param array $mapping - The URI -> ID mapping (as a reference, to be modified in-place) + * + * @param array $mapping - The URI -> ID mapping (as a reference, to be modified in-place) * @return void */ protected function unsetOldUrlsOf(Page $page, array &$mapping) @@ -175,8 +180,6 @@ protected function unsetOldUrlsOf(Page $page, array &$mapping) /** * Completely replaced the cached ID -> URI[] mapping - * @param array $mapping - * @return void */ protected function replaceIdToUriMapping(array $mapping): void { @@ -185,8 +188,6 @@ protected function replaceIdToUriMapping(array $mapping): void /** * Completely replace the cached URI -> ID mapping - * @param array $mapping - * @return void */ protected function replaceUriToIdMapping(array $mapping): void { From 4dbb41efaa1d573474b042aacaa785dbdb51ee7b Mon Sep 17 00:00:00 2001 From: Voltra Date: Sat, 13 Jan 2024 16:25:17 +0000 Subject: [PATCH 11/25] Make the manager constructor keep its previous arity --- src/FilamentFabricatorManager.php | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/FilamentFabricatorManager.php b/src/FilamentFabricatorManager.php index a9d43e3..23522f0 100644 --- a/src/FilamentFabricatorManager.php +++ b/src/FilamentFabricatorManager.php @@ -33,8 +33,16 @@ class FilamentFabricatorManager protected array $pageUrls = []; - public function __construct(protected PageRoutesService $routesService) + /** + * @note It's only separated to not cause a major version change. + * In the next major release, feel free to make it a constructor promoted property + */ + protected PageRoutesService $routesService; + + public function __construct(PageRoutesService $routesService = null) { + $this->routesService = $routesService ?? resolve(PageRoutesService::class); + /** @var Collection */ $pageBlocks = collect([]); From a39138fdcd5853e343eb4721057d20c45aff4fb9 Mon Sep 17 00:00:00 2001 From: Voltra Date: Sat, 13 Jan 2024 17:22:50 +0000 Subject: [PATCH 12/25] Add a command to clear (and refresh) the pages' cache --- src/Commands/ClearRoutesCacheCommand.php | 58 +++++++++++++++++++++++ src/FilamentFabricatorServiceProvider.php | 1 + 2 files changed, 59 insertions(+) create mode 100644 src/Commands/ClearRoutesCacheCommand.php diff --git a/src/Commands/ClearRoutesCacheCommand.php b/src/Commands/ClearRoutesCacheCommand.php new file mode 100644 index 0000000..f20cfd5 --- /dev/null +++ b/src/Commands/ClearRoutesCacheCommand.php @@ -0,0 +1,58 @@ +option('refresh'); + + /** + * @var PageContract[] $pages + */ + $pages = FilamentFabricator::getPageModel()::query() + ->whereNull('parent_id') + ->with('allChildren') + ->get(); + + foreach ($pages as $page) { + $this->clearPageCache($page, $shouldRefresh); + + } + + return static::SUCCESS; + } + + protected function clearPageCache(PageContract $page, bool $shouldRefresh = false) + { + $argSets = $page->getAllUrlCacheKeysArgs(); + + foreach ($argSets as $args) { + $key = $page->getUrlCacheKey($args); + Cache::forget($key); + + if ($shouldRefresh) { + // Caches the URL before returning it + /* $noop = */ $page->getUrl($args); + } + } + + $childPages = $page->allChildren; + + if (! empty($childPages)) { + foreach ($childPages as $childPage) { + $this->clearPageCache($childPage, $shouldRefresh); + } + } + } +} diff --git a/src/FilamentFabricatorServiceProvider.php b/src/FilamentFabricatorServiceProvider.php index d39a2e6..dacc786 100644 --- a/src/FilamentFabricatorServiceProvider.php +++ b/src/FilamentFabricatorServiceProvider.php @@ -42,6 +42,7 @@ protected function getCommands(): array $commands = [ Commands\MakeLayoutCommand::class, Commands\MakePageBlockCommand::class, + Commands\ClearRoutesCacheCommand::class, ]; $aliases = []; From 8fa3af08ec1dd31a942087a7651726243e788a66 Mon Sep 17 00:00:00 2001 From: Voltra Date: Sat, 13 Jan 2024 17:40:31 +0000 Subject: [PATCH 13/25] Add 'deleting' model observer listener --- src/Models/Concerns/HandlesPageUrls.php | 2 +- src/Models/Contracts/Page.php | 3 +- src/Observers/PageRoutesObserver.php | 51 +++++++++++++++++++++---- 3 files changed, 47 insertions(+), 9 deletions(-) diff --git a/src/Models/Concerns/HandlesPageUrls.php b/src/Models/Concerns/HandlesPageUrls.php index d2fdb9b..81e63f1 100644 --- a/src/Models/Concerns/HandlesPageUrls.php +++ b/src/Models/Concerns/HandlesPageUrls.php @@ -35,7 +35,7 @@ public function getUrl(array $args = []): string { $cacheKey = $this->getUrlCacheKey($args); - //TODO: Clear and re-compute cached routes when the routing prefix changes + //NOTE: Users must run the command that clears the routes cache if the routing prefix ever changes return Cache::rememberForever($cacheKey, function () use ($args) { /** diff --git a/src/Models/Contracts/Page.php b/src/Models/Contracts/Page.php index b0c1f83..f102a92 100644 --- a/src/Models/Contracts/Page.php +++ b/src/Models/Contracts/Page.php @@ -11,7 +11,8 @@ * @property-read string $slug * @property-read string $layout * @property-read array $blocks - * @property-read int $parent_id + * @property-read int|null $parent_id + * @property-read \Z3d0X\FilamentFabricator\Models\Contracts\Page|null $parent * @property-read \Illuminate\Database\Eloquent\Collection|\Z3d0X\FilamentFabricator\Models\Contracts\Page[] $children * @property-read \Illuminate\Database\Eloquent\Collection|\Z3d0X\FilamentFabricator\Models\Contracts\Page[] $allChildren * @property-read \Illuminate\Support\Carbon $created_at diff --git a/src/Observers/PageRoutesObserver.php b/src/Observers/PageRoutesObserver.php index b9bc81c..da134d2 100644 --- a/src/Observers/PageRoutesObserver.php +++ b/src/Observers/PageRoutesObserver.php @@ -2,7 +2,8 @@ namespace Z3d0X\FilamentFabricator\Observers; -use Z3d0X\FilamentFabricator\Models\Contracts\Page; +use Illuminate\Database\Eloquent\Model; +use Z3d0X\FilamentFabricator\Models\Contracts\Page as PageContract; use Z3d0X\FilamentFabricator\Services\PageRoutesService; class PageRoutesObserver @@ -15,7 +16,7 @@ public function __construct( /** * Handle the Page "created" event. */ - public function created(Page $page): void + public function created(PageContract $page): void { $this->pageRoutesService->updateUrlsOf($page); } @@ -23,23 +24,51 @@ public function created(Page $page): void /** * Handle the Page "updated" event. */ - public function updated(Page $page): void + public function updated(PageContract $page): void { $this->pageRoutesService->updateUrlsOf($page); } + /** + * Handle the Page "deleting" event. + */ + public function deleting(PageContract $page): void + { + /* + Doubly-linked list style deletion: + - Rattach the children to the parent of the page being deleted + - Promote the pages to a "root" page if the page being deleted has no parent + */ + + $shouldAssociate = $page->parent_id !== null; + + $children = $page->children; + + foreach ($children as $childPage) { + /** + * @var Model|PageContract $childPage + */ + if ($shouldAssociate) { + $page->parent()->associate($childPage); + } else { + $childPage->update([ + 'parent_id' => null, + ]); + } + } + } + /** * Handle the Page "deleted" event. */ - public function deleted(Page $page): void + public function deleted(PageContract $page): void { - //TODO: implement this } /** * Handle the Page "restored" event. */ - public function restored(Page $page): void + public function restored(PageContract $page): void { $this->pageRoutesService->updateUrlsOf($page); } @@ -47,7 +76,15 @@ public function restored(Page $page): void /** * Handle the Page "force deleted" event. */ - public function forceDeleted(Page $page): void + public function forceDeleting(PageContract $page): void + { + //TODO: implement this + } + + /** + * Handle the Page "force deleted" event. + */ + public function forceDeleted(PageContract $page): void { //TODO: implement this } From fe7689cd1a81be8ce3e8bc70652d454f5104977a Mon Sep 17 00:00:00 2001 From: ZedoX Date: Thu, 18 Jan 2024 09:46:38 +0500 Subject: [PATCH 14/25] fix: phpstan --- src/Commands/ClearRoutesCacheCommand.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Commands/ClearRoutesCacheCommand.php b/src/Commands/ClearRoutesCacheCommand.php index f20cfd5..1bca18d 100644 --- a/src/Commands/ClearRoutesCacheCommand.php +++ b/src/Commands/ClearRoutesCacheCommand.php @@ -49,7 +49,7 @@ protected function clearPageCache(PageContract $page, bool $shouldRefresh = fals $childPages = $page->allChildren; - if (! empty($childPages)) { + if (filled($childPages)) { foreach ($childPages as $childPage) { $this->clearPageCache($childPage, $shouldRefresh); } From ac469511ad1617de80987675d83b31092245dd7e Mon Sep 17 00:00:00 2001 From: Voltra Date: Fri, 9 Feb 2024 22:18:02 +0000 Subject: [PATCH 15/25] Add tests and fix the observer implementation issues --- src/Observers/PageRoutesObserver.php | 15 +- src/Services/PageRoutesService.php | 45 ++- tests/Observers/PageRoutesObserver.test.php | 347 ++++++++++++++++++++ tests/TestCase.php | 9 +- 4 files changed, 397 insertions(+), 19 deletions(-) create mode 100644 tests/Observers/PageRoutesObserver.test.php diff --git a/src/Observers/PageRoutesObserver.php b/src/Observers/PageRoutesObserver.php index da134d2..5dd1f02 100644 --- a/src/Observers/PageRoutesObserver.php +++ b/src/Observers/PageRoutesObserver.php @@ -34,27 +34,24 @@ public function updated(PageContract $page): void */ public function deleting(PageContract $page): void { + $this->pageRoutesService->removeUrlsOf($page); + /* Doubly-linked list style deletion: - Rattach the children to the parent of the page being deleted - Promote the pages to a "root" page if the page being deleted has no parent */ - $shouldAssociate = $page->parent_id !== null; - + $page->load('children'); $children = $page->children; foreach ($children as $childPage) { /** * @var Model|PageContract $childPage */ - if ($shouldAssociate) { - $page->parent()->associate($childPage); - } else { - $childPage->update([ - 'parent_id' => null, - ]); - } + $childPage->update([ + 'parent_id' => $page->parent_id, + ]); } } diff --git a/src/Services/PageRoutesService.php b/src/Services/PageRoutesService.php index d6e4998..65ce6ea 100644 --- a/src/Services/PageRoutesService.php +++ b/src/Services/PageRoutesService.php @@ -54,6 +54,26 @@ public function updateUrlsOf(Page $page): void }); } + /** + * Remove the cached URLs for the given page + */ + public function removeUrlsOf(Page $page): void + { + $this->forgetPageLocalCache($page); + + $idToUrlsMapping = $this->getIdToUrisMapping(); + $urls = $idToUrlsMapping[$page->id]; + $idToUrlsMapping[$page->id] = []; + unset($idToUrlsMapping[$page->id]); + $this->replaceIdToUriMapping($idToUrlsMapping); + + $uriToIdMapping = $this->getUriToIdMapping(); + foreach ($urls as $uri) { + unset($uriToIdMapping[$uri]); + } + $this->replaceUriToIdMapping($uriToIdMapping); + } + /** * Get an instance of your Page model from a URI, or throw if there is none */ @@ -131,7 +151,7 @@ protected function getUrisForPage(Page $page): array /** * Update routine for the given page * - * @param array $mapping - The URI -> ID mapping (as a reference, to be modified in-place) + * @param array $mapping - The URI -> ID mapping (as a reference, to be modified in-place) * @return void */ protected function updateUrlsAndDescendantsOf(Page $page, array &$mapping) @@ -143,6 +163,7 @@ protected function updateUrlsAndDescendantsOf(Page $page, array &$mapping) $id = $mapping[$uri] ?? -1; if ($id === $page->id) { + // Skip if the URI is already mapped to the right ID continue; } @@ -150,8 +171,8 @@ protected function updateUrlsAndDescendantsOf(Page $page, array &$mapping) $mapping[$uri] = $page->id; } - // $page->load(['children:id,slug']); - foreach ($page->children as $childPage) { + $page->load(['allChildren']); + foreach ($page->allChildren as $childPage) { $this->updateUrlsAndDescendantsOf($childPage, $mapping); } } @@ -159,25 +180,35 @@ protected function updateUrlsAndDescendantsOf(Page $page, array &$mapping) /** * Remove old URLs of the given page from the cached mappings * - * @param array $mapping - The URI -> ID mapping (as a reference, to be modified in-place) + * @param array $mapping - The URI -> ID mapping (as a reference, to be modified in-place) * @return void */ protected function unsetOldUrlsOf(Page $page, array &$mapping) { + $this->forgetPageLocalCache($page); + $oldUrlSet = collect($this->getUrisForPage($page))->lazy()->sort()->all(); - $allUrlSet = collect($page->getAllUrls())->lazy()->sort()->all(); + $newUrlSet = collect($page->getAllUrls())->lazy()->sort()->all(); - $oldUrls = array_diff($oldUrlSet, $allUrlSet); + $oldUrls = array_diff($oldUrlSet, $newUrlSet); foreach ($oldUrls as $oldUrl) { unset($mapping[$oldUrl]); } $idToUrlsMapping = $this->getIdToUrisMapping(); - $idToUrlsMapping[$page->id] = $allUrlSet; + $idToUrlsMapping[$page->id] = $newUrlSet; $this->replaceIdToUriMapping($idToUrlsMapping); } + protected function forgetPageLocalCache(Page $page) + { + $cacheKeys = array_map([$page, 'getUrlCacheKey'], $page->getAllUrlCacheKeysArgs()); + foreach ($cacheKeys as $cacheKey) { + Cache::forget($cacheKey); + } + } + /** * Completely replaced the cached ID -> URI[] mapping */ diff --git a/tests/Observers/PageRoutesObserver.test.php b/tests/Observers/PageRoutesObserver.test.php new file mode 100644 index 0000000..dc3a3f0 --- /dev/null +++ b/tests/Observers/PageRoutesObserver.test.php @@ -0,0 +1,347 @@ +delete(); + }); + + describe('#created($page)', function () { + it('properly adds all the page\'s URLs to the mapping', function () { + $beforeUrls = FilamentFabricator::getPageUrls(); + + /** + * @var Page $page + */ + $page = Page::create([ + 'title' => 'My title', + 'slug' => 'my-slug', + 'blocks' => [], + 'parent_id' => null, + ]); + + $afterUrls = FilamentFabricator::getPageUrls(); + + expect($afterUrls)->not->toEqual($beforeUrls); + + $pageUrls = $page->getAllUrls(); + + expect($afterUrls)->toContain(...$pageUrls); + }); + + it('properly works on child pages', function () { + /** + * @var Page $page + */ + $page = Page::create([ + 'title' => 'My title', + 'slug' => 'my-slug', + 'blocks' => [], + 'parent_id' => null, + ]); + + /** + * @var Page $page + */ + $child = Page::create([ + 'title' => 'My stuff', + 'slug' => 'my-stuff', + 'blocks' => [], + 'parent_id' => $page->id, + ]); + + $allUrls = FilamentFabricator::getPageUrls(); + $allUrls = collect($allUrls) + ->sort() + ->values() + ->toArray(); + + $expectedUrls = collect([ + '/my-slug', + '/my-slug/my-stuff', + ])->sort() + ->values() + ->toArray(); + + expect($allUrls)->toEqual($expectedUrls); + + /** + * @var Page $page + */ + $descendant = Page::create([ + 'title' => 'Abc xyz', + 'slug' => 'abc-xyz', + 'blocks' => [], + 'parent_id' => $child->id, + ]); + + $allUrls = FilamentFabricator::getPageUrls(); + $allUrls = collect($allUrls) + ->sort() + ->values() + ->toArray(); + + $expectedUrls = collect([ + '/my-slug', + '/my-slug/my-stuff', + '/my-slug/my-stuff/abc-xyz', + ])->sort() + ->values() + ->toArray(); + + expect($allUrls)->toEqual($expectedUrls); + }); + }); + + describe('#updated($page)', function () { + it('removes the old URLs from the mapping', function () { + /** + * @var Page $page + */ + $page = Page::create([ + 'title' => 'My title', + 'slug' => 'my-slug', + 'blocks' => [], + 'parent_id' => null, + ]); + + $oldUrls = $page->getAllUrls(); + + $page->slug = 'not-my-slug'; + $page->save(); + + $allUrls = FilamentFabricator::getPageUrls(); + + expect($allUrls)->not->toContain(...$oldUrls); + }); + + it('adds the new URLs to the mapping', function () { + /** + * @var Page $page + */ + $page = Page::create([ + 'title' => 'My title', + 'slug' => 'my-slug', + 'blocks' => [], + 'parent_id' => null, + ]); + + $page->slug = 'not-my-slug'; + $page->save(); + + $newUrls = $page->getAllUrls(); + + $allUrls = FilamentFabricator::getPageUrls(); + + expect($allUrls)->toContain(...$newUrls); + }); + + it('properly updates all child (and descendant) routes', function () { + /** + * @var Page $page + */ + $page = Page::create([ + 'title' => 'My title', + 'slug' => 'my-slug', + 'blocks' => [], + 'parent_id' => null, + ]); + + $child1 = Page::create([ + 'title' => 'My child 1', + 'slug' => 'child-1', + 'blocks' => [], + 'parent_id' => $page->id, + ]); + + $child2 = Page::create([ + 'title' => 'My child 2', + 'slug' => 'child-2', + 'blocks' => [], + 'parent_id' => $page->id, + ]); + + $child3 = Page::create([ + 'title' => 'My child 3', + 'slug' => 'child-3', + 'blocks' => [], + 'parent_id' => $page->id, + ]); + + $childOfChild = Page::create([ + 'title' => 'Subchild 1', + 'slug' => 'subchild-1', + 'blocks' => [], + 'parent_id' => $child2->id, + ]); + + /** + * @var Page[] $descendants + */ + $descendants = [$child1, $child2, $child3, $childOfChild]; + $oldUrlSets = array_map(fn (Page $page) => $page->getAllUrls(), $descendants); + + $page->slug = 'not-my-slug'; + $page->save(); + + foreach ($descendants as $descendant) { + $descendant->refresh(); + } + + $newUrlSets = array_map(fn (Page $page) => $page->getAllUrls(), $descendants); + + expect($newUrlSets)->not->toEqual($oldUrlSets); + + $allUrls = FilamentFabricator::getPageUrls(); + $allUrls = collect($allUrls) + ->sort() + ->values() + ->toArray(); + + $expectedUrls = collect([ + '/not-my-slug', + '/not-my-slug/child-1', + '/not-my-slug/child-2', + '/not-my-slug/child-3', + '/not-my-slug/child-2/subchild-1', + ])->sort() + ->values() + ->toArray(); + + expect($allUrls)->toEqual($expectedUrls); + + $child2->slug = 'not-child-2-xyz'; + $child2->save(); + + foreach ($descendants as $descendant) { + $descendant->refresh(); + } + + $allUrls = FilamentFabricator::getPageUrls(); + $allUrls = collect($allUrls) + ->sort() + ->values() + ->toArray(); + + $expectedUrls = collect([ + '/not-my-slug', + '/not-my-slug/child-1', + '/not-my-slug/not-child-2-xyz', + '/not-my-slug/child-3', + '/not-my-slug/not-child-2-xyz/subchild-1', + ])->sort() + ->values() + ->toArray(); + }); + }); + + describe('#deleting($page)', function () { + it('removes the page\'s URLs from the mapping', function () { + /** + * @var Page $page + */ + $page = Page::create([ + 'title' => 'My title', + 'slug' => 'my-slug', + 'blocks' => [], + 'parent_id' => null, + ]); + + $beforeUrls = FilamentFabricator::getPageUrls(); + + $page->delete(); + + $afterUrls = FilamentFabricator::getPageUrls(); + + expect($afterUrls)->not->toEqual($beforeUrls); + + expect($afterUrls)->toBeEmpty(); + }); + + it('sets the childrens\' parent to null if $page had no parent', function () { + /** + * @var Page $page + */ + $page = Page::create([ + 'title' => 'My title', + 'slug' => 'my-slug', + 'blocks' => [], + 'parent_id' => null, + ]); + + $child = Page::create([ + 'title' => 'My child page', + 'slug' => 'my-child-page', + 'blocks' => [], + 'parent_id' => $page->id, + ]); + + $page->delete(); + + $child->refresh(); + + expect($child->parent_id)->toBeNull(); + + $urls = FilamentFabricator::getPageUrls(); + + expect($urls)->toEqual(['/my-child-page']); + }); + + it('attaches the children to $page\'s parent if it had one', function () { + /** + * @var Page $page + */ + $page = Page::create([ + 'title' => 'My title', + 'slug' => 'my-slug', + 'blocks' => [], + 'parent_id' => null, + ]); + + /** + * @var Page $child + */ + $child = Page::create([ + 'title' => 'My child page', + 'slug' => 'my-child-page', + 'blocks' => [], + 'parent_id' => $page->id, + ]); + + /** + * @var Page $descendant + */ + $descendant = Page::create([ + 'title' => 'My sub page', + 'slug' => 'my-sub-page', + 'blocks' => [], + 'parent_id' => $child->id, + ]); + + $child->delete(); + $descendant->refresh(); + $page->refresh(); + + expect($descendant->parent_id)->toBe($page->id); + + $urls = collect(FilamentFabricator::getPageUrls()) + ->sort() + ->values() + ->toArray(); + + $expected = collect([ + '/my-slug', + '/my-slug/my-sub-page', + ]) + ->sort() + ->values() + ->toArray(); + + expect($urls)->toEqual($expected); + }); + }); +}); diff --git a/tests/TestCase.php b/tests/TestCase.php index 102661b..d181566 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -41,9 +41,12 @@ public function getEnvironmentSetUp($app) { config()->set('database.default', 'testing'); - /* - $migration = include __DIR__.'/../database/migrations/create_filament-fabricator_table.php.stub'; + + $migration = include __DIR__.'/../database/migrations/create_pages_table.php.stub'; $migration->up(); - */ + + $migration = include __DIR__.'/../database/migrations/fix_slug_unique_constraint_on_pages_table.php.stub'; + $migration->up(); + } } From 7914f407e71afb70a0f9617b39e4a44175de6742 Mon Sep 17 00:00:00 2001 From: Voltra Date: Fri, 9 Feb 2024 22:20:00 +0000 Subject: [PATCH 16/25] Tweak unit tests setup --- phpunit.xml.dist | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phpunit.xml.dist b/phpunit.xml.dist index 0d4b012..aa658b0 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -20,7 +20,7 @@ > - tests + tests/ From b5cc4b2c52815604cd67d48a6a440aee56c68924 Mon Sep 17 00:00:00 2001 From: Voltra Date: Sat, 10 Feb 2024 13:42:56 +0000 Subject: [PATCH 17/25] Extend page URLs API to refactor out code --- src/Models/Concerns/HandlesPageUrls.php | 10 ++++++++++ src/Models/Contracts/HasPageUrls.php | 7 +++++++ src/Services/PageRoutesService.php | 2 +- 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/Models/Concerns/HandlesPageUrls.php b/src/Models/Concerns/HandlesPageUrls.php index 81e63f1..a294d65 100644 --- a/src/Models/Concerns/HandlesPageUrls.php +++ b/src/Models/Concerns/HandlesPageUrls.php @@ -79,4 +79,14 @@ public function getAllUrls(): array { return array_map([$this, 'getUrl'], $this->getAllUrlCacheKeysArgs()); } + + /** + * Get all the cache keys for the available URLs for this entity + * + * @return string[] + */ + public function getAllUrlCacheKeys(): array + { + return array_map([$this, 'getUrlCacheKey'], $this->getAllUrlCacheKeysArgs()); + } } diff --git a/src/Models/Contracts/HasPageUrls.php b/src/Models/Contracts/HasPageUrls.php index 1d6570b..d2d0962 100644 --- a/src/Models/Contracts/HasPageUrls.php +++ b/src/Models/Contracts/HasPageUrls.php @@ -32,4 +32,11 @@ public function getAllUrlCacheKeysArgs(): array; * @return string[] */ public function getAllUrls(): array; + + /** + * Get all the cache keys for the available URLs for this entity + * + * @return string[] + */ + public function getAllUrlCacheKeys(): array; } diff --git a/src/Services/PageRoutesService.php b/src/Services/PageRoutesService.php index 65ce6ea..0cc87d0 100644 --- a/src/Services/PageRoutesService.php +++ b/src/Services/PageRoutesService.php @@ -203,7 +203,7 @@ protected function unsetOldUrlsOf(Page $page, array &$mapping) protected function forgetPageLocalCache(Page $page) { - $cacheKeys = array_map([$page, 'getUrlCacheKey'], $page->getAllUrlCacheKeysArgs()); + $cacheKeys = $page->getAllUrlCacheKeys(); foreach ($cacheKeys as $cacheKey) { Cache::forget($cacheKey); } From 128d819811d8d37519a034cc5cbf6afe626a9dc4 Mon Sep 17 00:00:00 2001 From: Voltra Date: Sat, 10 Feb 2024 14:36:02 +0000 Subject: [PATCH 18/25] Add tests for the routes cache clear command --- src/Commands/ClearRoutesCacheCommand.php | 7 + .../Commands/ClearRoutesCacheCommand.test.php | 124 ++++++++++++++++++ 2 files changed, 131 insertions(+) create mode 100644 tests/Commands/ClearRoutesCacheCommand.test.php diff --git a/src/Commands/ClearRoutesCacheCommand.php b/src/Commands/ClearRoutesCacheCommand.php index 1bca18d..d8e11b5 100644 --- a/src/Commands/ClearRoutesCacheCommand.php +++ b/src/Commands/ClearRoutesCacheCommand.php @@ -6,6 +6,7 @@ use Illuminate\Support\Facades\Cache; use Z3d0X\FilamentFabricator\Facades\FilamentFabricator; use Z3d0X\FilamentFabricator\Models\Contracts\Page as PageContract; +use Z3d0X\FilamentFabricator\Services\PageRoutesService; class ClearRoutesCacheCommand extends Command { @@ -13,6 +14,11 @@ class ClearRoutesCacheCommand extends Command protected $description = 'Clear the routes\' cache'; + public function __construct(protected PageRoutesService $pageRoutesService) + { + parent::__construct(); + } + public function handle(): int { $shouldRefresh = $this->option('refresh'); @@ -35,6 +41,7 @@ public function handle(): int protected function clearPageCache(PageContract $page, bool $shouldRefresh = false) { + $this->pageRoutesService->removeUrlsOf($page); $argSets = $page->getAllUrlCacheKeysArgs(); foreach ($argSets as $args) { diff --git a/tests/Commands/ClearRoutesCacheCommand.test.php b/tests/Commands/ClearRoutesCacheCommand.test.php new file mode 100644 index 0000000..1abbabe --- /dev/null +++ b/tests/Commands/ClearRoutesCacheCommand.test.php @@ -0,0 +1,124 @@ +toBeInstanceOf(ClearRoutesCacheCommand::class); + }); + + it('clears all route caches', function () { + /** + * @var Page $page + */ + $page = Page::create([ + 'title' => 'My title', + 'slug' => 'my-slug', + 'blocks' => [], + 'parent_id' => null, + ]); + + /** + * @var Page $child + */ + $child = Page::create([ + 'title' => 'My child page', + 'slug' => 'my-child-page', + 'blocks' => [], + 'parent_id' => $page->id, + ]); + + artisan('filament-fabricator:clear-routes-cache') + ->assertSuccessful(); + + expect(Cache::get('filament-fabricator::PageRoutesService::uri-to-id'))->toBeEmpty(); + expect(Cache::get('filament-fabricator::PageRoutesService::id-to-uri'))->toBeEmpty(); + + $cacheKeys = [...$page->getAllUrlCacheKeys(), ...$child->getAllUrlCacheKeys()]; + + expect($cacheKeys)->not->toBeEmpty(); + + expect( + collect($cacheKeys) + ->every(fn (string $cacheKey) => ! Cache::has($cacheKey)) + )->toBeTrue(); + }); + + it('refreshes the cache properly', function (string $flag, string $newPrefix) { + /** + * @var Page $page + */ + $page = Page::create([ + 'title' => 'My title', + 'slug' => 'my-slug', + 'blocks' => [], + 'parent_id' => null, + ]); + + /** + * @var Page $child + */ + $child = Page::create([ + 'title' => 'My child page', + 'slug' => 'my-child-page', + 'blocks' => [], + 'parent_id' => $page->id, + ]); + + $urls = [...$page->getAllUrls(), ...$child->getAllUrls()]; + + $prevUTI = Cache::get('filament-fabricator::PageRoutesService::uri-to-id'); + $prevITU = Cache::get('filament-fabricator::PageRoutesService::id-to-uri'); + + Config::set('filament-fabricator.routing.prefix', $newPrefix); + + artisan('filament-fabricator:clear-routes-cache', [ + $flag => true, + ]) + ->assertSuccessful(); + + $newUrls = [...$page->getAllUrls(), ...$child->getAllUrls()]; + + expect($newUrls)->not->toEqualCanonicalizing($urls); + + expect($newUrls)->not->toBeEmpty(); + + expect( + collect($newUrls) + ->every(fn (string $url) => str_starts_with($url, "/$newPrefix")) + )->toBeTrue(); + + $newUTI = Cache::get('filament-fabricator::PageRoutesService::uri-to-id'); + + expect($newUTI)->not->toEqualCanonicalizing($prevUTI); + expect( + collect($newUTI) + ->keys() + ->every(fn(string $uri) => str_starts_with($uri, "/$newPrefix")) + ); + + $newITU = Cache::get('filament-fabricator::PageRoutesService::id-to-uri'); + + expect($newITU)->not->toEqualCanonicalizing($prevITU); + expect( + collect($newITU) + ->values() + ->flatten() + ->every(fn(string $uri) => str_starts_with($uri, "/$newPrefix")) + ); + })->with([ + ['--refresh', 'newprefix'], + ['-R', 'np'], + ]); +}); From 65ad4e76d0115014a3d91e2a6ceb52e6dfaf55c8 Mon Sep 17 00:00:00 2001 From: Voltra Date: Sat, 10 Feb 2024 14:54:27 +0000 Subject: [PATCH 19/25] Add tests for the routes cache clear command --- src/Commands/ClearRoutesCacheCommand.php | 5 ++- .../Commands/ClearRoutesCacheCommand.test.php | 37 +++++++++++++------ 2 files changed, 30 insertions(+), 12 deletions(-) diff --git a/src/Commands/ClearRoutesCacheCommand.php b/src/Commands/ClearRoutesCacheCommand.php index d8e11b5..b5be09a 100644 --- a/src/Commands/ClearRoutesCacheCommand.php +++ b/src/Commands/ClearRoutesCacheCommand.php @@ -21,7 +21,7 @@ public function __construct(protected PageRoutesService $pageRoutesService) public function handle(): int { - $shouldRefresh = $this->option('refresh'); + $shouldRefresh = (bool) $this->option('refresh'); /** * @var PageContract[] $pages @@ -34,6 +34,9 @@ public function handle(): int foreach ($pages as $page) { $this->clearPageCache($page, $shouldRefresh); + if ($shouldRefresh) { + $this->pageRoutesService->updateUrlsOf($page); + } } return static::SUCCESS; diff --git a/tests/Commands/ClearRoutesCacheCommand.test.php b/tests/Commands/ClearRoutesCacheCommand.test.php index 1abbabe..abf21e4 100644 --- a/tests/Commands/ClearRoutesCacheCommand.test.php +++ b/tests/Commands/ClearRoutesCacheCommand.test.php @@ -4,6 +4,7 @@ use Illuminate\Support\Facades\Config; use Z3d0X\FilamentFabricator\Commands\ClearRoutesCacheCommand; use Z3d0X\FilamentFabricator\Models\Page; +use Z3d0X\FilamentFabricator\Services\PageRoutesService; use function Pest\Laravel\artisan; @@ -19,6 +20,11 @@ }); it('clears all route caches', function () { + /** + * @var PageRoutesService $service + */ + $service = resolve(PageRoutesService::class); + /** * @var Page $page */ @@ -39,6 +45,8 @@ 'parent_id' => $page->id, ]); + $service->getAllUrls(); // ensure everything is cached beforehand + artisan('filament-fabricator:clear-routes-cache') ->assertSuccessful(); @@ -56,6 +64,11 @@ }); it('refreshes the cache properly', function (string $flag, string $newPrefix) { + /** + * @var PageRoutesService $service + */ + $service = resolve(PageRoutesService::class); + /** * @var Page $page */ @@ -76,10 +89,13 @@ 'parent_id' => $page->id, ]); - $urls = [...$page->getAllUrls(), ...$child->getAllUrls()]; + $urls = collect([...$page->getAllUrls(), ...$child->getAllUrls()])->sort()->toArray(); $prevUTI = Cache::get('filament-fabricator::PageRoutesService::uri-to-id'); + $prevUTI = collect($prevUTI)->sort()->toArray(); + $prevITU = Cache::get('filament-fabricator::PageRoutesService::id-to-uri'); + $prevITU = collect($prevITU)->sort()->toArray(); Config::set('filament-fabricator.routing.prefix', $newPrefix); @@ -88,34 +104,33 @@ ]) ->assertSuccessful(); - $newUrls = [...$page->getAllUrls(), ...$child->getAllUrls()]; - + $newUrls = collect([...$page->getAllUrls(), ...$child->getAllUrls()])->sort()->toArray(); expect($newUrls)->not->toEqualCanonicalizing($urls); - expect($newUrls)->not->toBeEmpty(); - expect( collect($newUrls) ->every(fn (string $url) => str_starts_with($url, "/$newPrefix")) )->toBeTrue(); $newUTI = Cache::get('filament-fabricator::PageRoutesService::uri-to-id'); - - expect($newUTI)->not->toEqualCanonicalizing($prevUTI); + $newUTI = collect($newUTI)->sort()->toArray(); + expect($newUTI)->not->toEqual($prevUTI); + expect($newUTI)->not->toBeEmpty(); expect( collect($newUTI) ->keys() - ->every(fn(string $uri) => str_starts_with($uri, "/$newPrefix")) + ->every(fn (string $uri) => str_starts_with($uri, "/$newPrefix")) ); $newITU = Cache::get('filament-fabricator::PageRoutesService::id-to-uri'); - - expect($newITU)->not->toEqualCanonicalizing($prevITU); + $newITU = collect($newITU)->sort()->toArray(); + expect($newITU)->not->toEqual($prevITU); + expect($newITU)->not->toBeEmpty(); expect( collect($newITU) ->values() ->flatten() - ->every(fn(string $uri) => str_starts_with($uri, "/$newPrefix")) + ->every(fn (string $uri) => str_starts_with($uri, "/$newPrefix")) ); })->with([ ['--refresh', 'newprefix'], From 095ba9fc4ba413aceed410affdec262c2c64bfda Mon Sep 17 00:00:00 2001 From: Voltra Date: Sat, 10 Feb 2024 15:00:36 +0000 Subject: [PATCH 20/25] Make the default cache args invariant explicit --- src/Models/Contracts/HasPageUrls.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Models/Contracts/HasPageUrls.php b/src/Models/Contracts/HasPageUrls.php index d2d0962..88050c8 100644 --- a/src/Models/Contracts/HasPageUrls.php +++ b/src/Models/Contracts/HasPageUrls.php @@ -6,6 +6,7 @@ interface HasPageUrls { /** * Get the default arguments for URL generation + * @invariant HasPageUrls#getDefaultUrlCacheArgs() must always return the same value, regardless of app/user configuration */ public function getDefaultUrlCacheArgs(): array; From 2ad06f2902013409758b74ad3c518691de411091 Mon Sep 17 00:00:00 2001 From: Voltra Date: Thu, 15 Feb 2024 17:44:52 +0000 Subject: [PATCH 21/25] Hook the routes cache command to core artisan commands --- config/filament-fabricator.php | 14 +++++ src/FilamentFabricatorServiceProvider.php | 7 +++ src/Listeners/OptimizeWithLaravel.php | 66 +++++++++++++++++++++++ 3 files changed, 87 insertions(+) create mode 100644 src/Listeners/OptimizeWithLaravel.php diff --git a/config/filament-fabricator.php b/config/filament-fabricator.php index a3a9fcb..6575906 100644 --- a/config/filament-fabricator.php +++ b/config/filament-fabricator.php @@ -39,6 +39,20 @@ 'enable-view-page' => false, + /** + * Whether to hook into artisan's core commands to clear and refresh page route caches along with the rest. + * Disable for manual control over cache. + * + * This is the list of commands that will be hooked into: + * - cache:clear -> clear routes cache + * - config:cache -> refresh routes cache + * - config:clear -> clear routes cache + * - optimize -> refresh routes cache + * - optimize:clear -> clear routes cache + * - route:clear -> clear routes cache + */ + 'hook-to-commands' => true, + /* * This is the name of the table that will be created by the migration and * used by the above page-model shipped with this package. diff --git a/src/FilamentFabricatorServiceProvider.php b/src/FilamentFabricatorServiceProvider.php index d7b1830..5059e39 100644 --- a/src/FilamentFabricatorServiceProvider.php +++ b/src/FilamentFabricatorServiceProvider.php @@ -2,7 +2,9 @@ namespace Z3d0X\FilamentFabricator; +use Illuminate\Console\Events\CommandFinished; use Illuminate\Filesystem\Filesystem; +use Illuminate\Support\Facades\Event; use Illuminate\Support\Facades\Route; use Illuminate\Support\Str; use ReflectionClass; @@ -12,6 +14,7 @@ use Symfony\Component\Finder\SplFileInfo; use Z3d0X\FilamentFabricator\Facades\FilamentFabricator; use Z3d0X\FilamentFabricator\Layouts\Layout; +use Z3d0X\FilamentFabricator\Listeners\OptimizeWithLaravel; use Z3d0X\FilamentFabricator\Observers\PageRoutesObserver; use Z3d0X\FilamentFabricator\PageBlocks\PageBlock; use Z3d0X\FilamentFabricator\Services\PageRoutesService; @@ -103,6 +106,10 @@ public function packageBooted() parent::packageBooted(); FilamentFabricator::getPageModel()::observe(PageRoutesObserver::class); + + if ((bool) config('filament-fabricator.hook-to-commands')) { + Event::listen(CommandFinished::class, OptimizeWithLaravel::class); + } } protected function registerComponentsFromDirectory(string $baseClass, array $register, ?string $directory, ?string $namespace): void diff --git a/src/Listeners/OptimizeWithLaravel.php b/src/Listeners/OptimizeWithLaravel.php new file mode 100644 index 0000000..6dffeb1 --- /dev/null +++ b/src/Listeners/OptimizeWithLaravel.php @@ -0,0 +1,66 @@ +shouldHandleEvent($event)) { + return; + } + + if ($this->shouldRefresh($event)) { + $this->refresh(); + } else { + $this->clear(); + } + } + + public function shouldHandleEvent(CommandFinished $event) + { + return $event->exitCode === Command::SUCCESS + && in_array($event->command, static::COMMANDS); + } + + public function shouldRefresh(CommandFinished $event) + { + return in_array($event->command, static::REFRESH_COMMANDS); + } + + public function refresh() + { + $this->callCommand([ + '--refresh' => true, + ]); + } + + public function clear() + { + $this->callCommand(); + } + + public function callCommand(array $params = []) + { + Artisan::call(ClearRoutesCacheCommand::class, $params); + } +} From 334704ffc0cfc673758694c9f8194d4a2916113a Mon Sep 17 00:00:00 2001 From: Voltra Date: Tue, 27 Feb 2024 18:26:17 +0000 Subject: [PATCH 22/25] Fix corner-case bug of changing a page's parent, add laravel-style comments --- src/Models/Concerns/HandlesPageUrls.php | 22 +- src/Observers/PageRoutesObserver.php | 55 ++-- src/Services/PageRoutesService.php | 163 +++++++++--- tests/Observers/PageRoutesObserver.test.php | 277 ++++++++++++++++++++ 4 files changed, 470 insertions(+), 47 deletions(-) diff --git a/src/Models/Concerns/HandlesPageUrls.php b/src/Models/Concerns/HandlesPageUrls.php index a294d65..405a422 100644 --- a/src/Models/Concerns/HandlesPageUrls.php +++ b/src/Models/Concerns/HandlesPageUrls.php @@ -19,6 +19,8 @@ public function getDefaultUrlCacheArgs(): array /** * Get the cache key for the URL determined by this entity and the provided arguments + * + * @param array $args */ public function getUrlCacheKey(array $args = []): string { @@ -30,6 +32,8 @@ public function getUrlCacheKey(array $args = []): string /** * Get the URL determined by this entity and the provided arguments + * + * @param array $args */ public function getUrl(array $args = []): string { @@ -42,16 +46,29 @@ public function getUrl(array $args = []): string * @var ?Page $parent */ $parent = $this->parent; + + // If there's no parent page, then the "parent" URI is just the routing prefix. $parentUri = is_null($parent) ? (FilamentFabricator::getRoutingPrefix() ?? '/') : $parent->getUrl($args); + + // Every URI in cache has a leading slash, this ensures it's + // present even if the prefix doesn't have it set explicitly $parentUri = Str::start($parentUri, '/'); + // This page's part of the URL (i.e. its URI) is defined as the slug. + // For the same reasons as above, we need to add a leading slash. $selfUri = $this->slug; $selfUri = Str::start($selfUri, '/'); + // If the parent URI is the root, then we have nothing to glue on. + // Therefore the page's URL is simply its URI. + // This avoids having two consecutive slashes. if ($parentUri === '/') { return $selfUri; } + // Remove any trailing slash in the parent URI since + // every URIs we'll use has a leading slash. + // This avoids having two consecutive slashes. $parentUri = rtrim($parentUri, '/'); return "{$parentUri}{$selfUri}"; @@ -61,10 +78,13 @@ public function getUrl(array $args = []): string /** * Get all the available argument sets for the available cache keys * - * @return array[] + * @return array[] */ public function getAllUrlCacheKeysArgs(): array { + // By default, the entire list of available URL cache keys + // is simply a list containing the default one since we can't + // magically infer all the possible state for the library user's customizations. return [ $this->getDefaultUrlCacheArgs(), ]; diff --git a/src/Observers/PageRoutesObserver.php b/src/Observers/PageRoutesObserver.php index 5dd1f02..c39eefb 100644 --- a/src/Observers/PageRoutesObserver.php +++ b/src/Observers/PageRoutesObserver.php @@ -16,32 +16,46 @@ public function __construct( /** * Handle the Page "created" event. */ - public function created(PageContract $page): void + public function created(PageContract&Model $page): void { + // Creating the page simply requires setting the URLs in all caches. + // This will be done properly through the update procedure. $this->pageRoutesService->updateUrlsOf($page); } /** * Handle the Page "updated" event. */ - public function updated(PageContract $page): void + public function updated(PageContract&Model $page): void { + // If the parent_id has changed, and if the relationship has already been loaded + // then after an update we might not read the right parent. That's why we always + // load it on update, this ensures we clear the old URLs properly (they were cached) + // and set the new ones properly (we have the right parent to do so). + if ($page->wasChanged('parent_id')) { + $page->load('parent'); + } + $this->pageRoutesService->updateUrlsOf($page); } /** * Handle the Page "deleting" event. */ - public function deleting(PageContract $page): void + public function deleting(PageContract&Model $page): void { + // We do the logic in `deleting` instead of `deleted` since we need access to the object + // both in memory and in database (e.g. to load relationship data). + + // Before properly deleting it, remove its URLs from + // all the mappings and caches. $this->pageRoutesService->removeUrlsOf($page); - /* - Doubly-linked list style deletion: - - Rattach the children to the parent of the page being deleted - - Promote the pages to a "root" page if the page being deleted has no parent - */ + // Doubly-linked list style deletion: + // - Re-attache the given page children to the parent of the given page + // - Promote the pages to a "root page" (i.e. page with no parent) if the given page had no parent + // Only load one level of children since they're the ones that will be re-attached $page->load('children'); $children = $page->children; @@ -49,8 +63,15 @@ public function deleting(PageContract $page): void /** * @var Model|PageContract $childPage */ + + // We use `?? null` followed by `?: null` to go around the cast to integer + // and make sure we have `null` instead of `0` when there's no parent. + $parentId = $page->parent_id ?? null; + $parentId = $parentId ?: null; + + // Using update, instead of associate or dissociate, we trigger DB events (which we need) $childPage->update([ - 'parent_id' => $page->parent_id, + 'parent_id' => $parentId, ]); } } @@ -58,14 +79,16 @@ public function deleting(PageContract $page): void /** * Handle the Page "deleted" event. */ - public function deleted(PageContract $page): void + public function deleted(PageContract&Model $page): void { + // Since `deleting` is called before `deleted`, and + // since everything is handled there, do nothing. } /** * Handle the Page "restored" event. */ - public function restored(PageContract $page): void + public function restored(PageContract&Model $page): void { $this->pageRoutesService->updateUrlsOf($page); } @@ -73,16 +96,18 @@ public function restored(PageContract $page): void /** * Handle the Page "force deleted" event. */ - public function forceDeleting(PageContract $page): void + public function forceDeleting(PageContract&Model $page): void { - //TODO: implement this + // You always go through `deleting` before going through `forceDeleting`. + // Since everything is properly handled in `deleting`, do nothing here. } /** * Handle the Page "force deleted" event. */ - public function forceDeleted(PageContract $page): void + public function forceDeleted(PageContract&Model $page): void { - //TODO: implement this + // You always go through `deleted` before going through `forceDeleted`. + // Since everything is properly handled in `deleted`, do nothing here. } } diff --git a/src/Services/PageRoutesService.php b/src/Services/PageRoutesService.php index 0cc87d0..8ff4a0b 100644 --- a/src/Services/PageRoutesService.php +++ b/src/Services/PageRoutesService.php @@ -8,6 +8,17 @@ use Z3d0X\FilamentFabricator\Facades\FilamentFabricator; use Z3d0X\FilamentFabricator\Models\Contracts\Page; +// The service is a coordinator between all route caches +// There are three layers of caches that have to be in sync: +// - PageRoutesService::URI_TO_ID_MAPPING that maps from URI to ID (many-to-one) +// - PageRoutesService::ID_TO_URI_MAPPING that maps from ID to URIS (one-to-many) +// - Page::getUrl (and thus Page::getAllUrl) +// +// Syncinc and consistencies should be handle via the service. +// As such, it's responsible for maintaining internale consistency +// and hiding/encapsulating implementation details. +// +// It relies on the extension points defined by Z3d0X\FilamentFabricator\Models\Contracts\HasPageUrls class PageRoutesService { protected const URI_TO_ID_MAPPING = 'filament-fabricator::PageRoutesService::uri-to-id'; @@ -16,9 +27,15 @@ class PageRoutesService /** * Get the ID of the Page model to which the given URI is associated, -1 if non matches + * + * @return int The page's ID, or -1 on failure */ public function getPageIdFromUri(string $uri): int { + // Query the (URI -> ID) mapping based on the user provided URI. + // The mapping expect a URI that starts with a / + // thus we "normalize" the URI by ensuring it starts with one. + // Not doing so would result in a false negative. $mapping = $this->getUriToIdMapping(); $uri = Str::start($uri, '/'); @@ -28,17 +45,18 @@ public function getPageIdFromUri(string $uri): int /** * Get an instance of your Page model from a URI, or NULL if none matches * - * @return ?Page + * @return null|(Page&Model) */ - public function getPageFromUri(string $uri): ?Page + public function getPageFromUri(string $uri): null|(Page&Model) { $id = $this->getPageIdFromUri($uri); + // We know the getPageIdFromUri uses -1 as a "sentinel" value + // for when the page is not found, so return null in those cases if ($id === -1) { return null; } - /** @var Page&Model */ return FilamentFabricator::getPageModel()::find($id); } @@ -47,6 +65,11 @@ public function getPageFromUri(string $uri): ?Page */ public function updateUrlsOf(Page $page): void { + // We mutate the mapping without events to ensure we don't have "concurrent" + // modifications of the same mapping. This allows us to skip the use of locks + // in an environment where only unrelated pages can be modified by separate + // users at the same time, which is a responsibility the library users + // should enforce themselves. FilamentFabricator::getPageModel()::withoutEvents(function () use ($page) { $mapping = $this->getUriToIdMapping(); $this->updateUrlsAndDescendantsOf($page, $mapping); @@ -59,19 +82,24 @@ public function updateUrlsOf(Page $page): void */ public function removeUrlsOf(Page $page): void { - $this->forgetPageLocalCache($page); - + // First remove the entries from the (ID -> URI) mapping $idToUrlsMapping = $this->getIdToUrisMapping(); $urls = $idToUrlsMapping[$page->id]; - $idToUrlsMapping[$page->id] = []; + $idToUrlsMapping[$page->id] = null; unset($idToUrlsMapping[$page->id]); $this->replaceIdToUriMapping($idToUrlsMapping); + // Then remove the entries from the (URI -> ID) mapping $uriToIdMapping = $this->getUriToIdMapping(); foreach ($urls as $uri) { unset($uriToIdMapping[$uri]); } $this->replaceUriToIdMapping($uriToIdMapping); + + // Finally, clear the page's local caches of its own URL. + // This means that Page::getAllUrls() and such will now compute + // fresh values. + $this->forgetPageLocalCache($page); } /** @@ -81,7 +109,8 @@ public function findPageOrFail(string $uri): Page&Model { $id = $this->getPageIdFromUri($uri); - /** @var Page&Model */ + // If the page doesn't exists, we know getPageIdFromUri + // will return -1. Thus findOrFail will fail as expected. return FilamentFabricator::getPageModel()::findOrFail($id); } @@ -92,9 +121,15 @@ public function findPageOrFail(string $uri): Page&Model */ public function getAllUrls(): array { - $mapping = $this->getUriToIdMapping(); + $uriToIdMapping = $this->getUriToIdMapping(); - return array_values(array_keys($mapping)); + // $uriToIdMapping is an associative array that maps URIs to IDs. + // Thus, the list of URLs is the keys of that array. + // Since PHP handles keys very weirdly when using array_keys, + // we simply get its array_values to have a truly regular array + // instead of an associative array where the keys are all numbers + // but possibly non-sorted. + return array_values(array_keys($uriToIdMapping)); } /** @@ -104,17 +139,25 @@ public function getAllUrls(): array */ protected function getUriToIdMapping(): array { + // The mapping will be cached for most requests. + // The very first person hitting the cache when it's not readily available + // will sadly have to recompute the whole thing. return Cache::rememberForever(static::URI_TO_ID_MAPPING, function () { - $idsToUris = $this->getIdToUrisMapping(); - $mapping = []; - - foreach ($idsToUris as $id => $uris) { + // Even though we technically have 2 separate caches + // we want them to not really be independent. + // Here we ensure our initial state depends on the other + // cache's initial state. + $idsToUrisMapping = $this->getIdToUrisMapping(); + $uriToIdMapping = []; + + // We simply "reverse" the one-to-many mapping to a many-to-one + foreach ($idsToUrisMapping as $id => $uris) { foreach ($uris as $uri) { - $mapping[$uri] = $id; + $uriToIdMapping[$uri] = $id; } } - return $mapping; + return $uriToIdMapping; }); } @@ -125,10 +168,18 @@ protected function getUriToIdMapping(): array */ protected function getIdToUrisMapping(): array { + // The mapping will be cached for most requests. + // The very first person hitting the cache when it's not readily available + // will sadly have to recompute the whole thing. + // This could be a critical section and bottleneck depending on the use cases. + // Any optimization to this can greatly improve the entire package's performances + // in one fell swoop. return Cache::rememberForever(static::ID_TO_URI_MAPPING, function () { $pages = FilamentFabricator::getPageModel()::all(); $mapping = []; $pages->each(function (Page $page) use (&$mapping) { + // Note that this also has the benefits of computing + // the page's local caches. $mapping[$page->id] = $page->getAllUrls(); }); @@ -151,49 +202,82 @@ protected function getUrisForPage(Page $page): array /** * Update routine for the given page * - * @param array $mapping - The URI -> ID mapping (as a reference, to be modified in-place) + * @param array $uriToIdMapping - The URI -> ID mapping (as a reference, to be modified in-place) * @return void */ - protected function updateUrlsAndDescendantsOf(Page $page, array &$mapping) + protected function updateUrlsAndDescendantsOf(Page $page, array &$uriToIdMapping) { - $this->unsetOldUrlsOf($page, $mapping); + // First ensure consistency by removing any trace of the old URLs + // for the given page. Whether local or in the URI to ID mapping. + $this->unsetOldUrlsOf($page, $uriToIdMapping); + + // These URLs will always be fresh since we unset the old ones just above $urls = $page->getAllUrls(); foreach ($urls as $uri) { - $id = $mapping[$uri] ?? -1; + $id = $uriToIdMapping[$uri] ?? -1; + // If while iterating the fresh URLs we encounter one + // that is already mapped to the right page ID + // then there's nothing to do for this URL + // and thus continue onward with the next one. if ($id === $page->id) { - // Skip if the URI is already mapped to the right ID continue; } - unset($mapping[$uri]); - $mapping[$uri] = $page->id; + // Otherwise, we have a URI that already exists + // and is mapped to the wrong ID, or it wasn't + // in the mapping yet. In both cases we just need + // to add it to the mapping at the correct spot. + $uriToIdMapping[$uri] = $page->id; } + // Since we're recursing down the tree, we preload the relationships + // once, and traverse down the tree. This helps with performances. + // TODO: Make it work with loadMissing instead of load to reduce the number of useless DB queries $page->load(['allChildren']); foreach ($page->allChildren as $childPage) { - $this->updateUrlsAndDescendantsOf($childPage, $mapping); + // A change in a parent page will always result + // in a change to its descendant. As such, + // we need to recompute everything that's + // a descendant of this page. + $this->updateUrlsAndDescendantsOf($childPage, $uriToIdMapping); } } /** * Remove old URLs of the given page from the cached mappings * - * @param array $mapping - The URI -> ID mapping (as a reference, to be modified in-place) + * @param array $uriToIdMapping - The URI -> ID mapping (as a reference, to be modified in-place) * @return void */ - protected function unsetOldUrlsOf(Page $page, array &$mapping) + protected function unsetOldUrlsOf(Page $page, array &$uriToIdMapping) { + // When we're hitting this path, caches haven't been invalidated yet. + // Thus we don't need to query the mappings to get the old URLs. + $oldUrlSet = collect($page->getAllUrls())->lazy()->sort()->all(); + + // Once we're done collecting the previous URLs, and since we want + // to unset ALL old URLs for this given page, we might as well + // forget its local caches here. $this->forgetPageLocalCache($page); - $oldUrlSet = collect($this->getUrisForPage($page))->lazy()->sort()->all(); + // Since we just forgot the page's local caches, this doesn't + // return the old set of URLs, but instead computes and caches + // the new URLs based on the page's currently loaded data. $newUrlSet = collect($page->getAllUrls())->lazy()->sort()->all(); + // The old URLs are those that are present in the $oldUrlSet + // but are not present in $newUrlSet. Hence the use of array_diff + // whose role is to return exactly that. Also note we sorted the arrays + // in order to make sure the diff algorithm has every chances to be + // optimal in performance. $oldUrls = array_diff($oldUrlSet, $newUrlSet); + // Simply go through the list of old URLs and remove them from the mapping. + // This is one of the reasons we pass it by reference. foreach ($oldUrls as $oldUrl) { - unset($mapping[$oldUrl]); + unset($uriToIdMapping[$oldUrl]); } $idToUrlsMapping = $this->getIdToUrisMapping(); @@ -201,8 +285,13 @@ protected function unsetOldUrlsOf(Page $page, array &$mapping) $this->replaceIdToUriMapping($idToUrlsMapping); } + /** + * Forget all URL caches tied to the page (cf. Page::getAllUrlCacheKeys) + */ protected function forgetPageLocalCache(Page $page) { + // The page local caches are simply those behind the + // URL cache keys. Compute the keys, forget the caches. $cacheKeys = $page->getAllUrlCacheKeys(); foreach ($cacheKeys as $cacheKey) { Cache::forget($cacheKey); @@ -211,17 +300,29 @@ protected function forgetPageLocalCache(Page $page) /** * Completely replaced the cached ID -> URI[] mapping + * + * @param array $idToUriMapping */ - protected function replaceIdToUriMapping(array $mapping): void + protected function replaceIdToUriMapping(array $idToUriMapping): void { - Cache::forever(static::ID_TO_URI_MAPPING, $mapping); + // Replace the ID -> URI[] mapping with the given one. + // This is done "atomically" with regards to the cache. + // Note that concurrent read and writes can result in lost updates. + // And thus in an invalid state. + Cache::forever(static::ID_TO_URI_MAPPING, $idToUriMapping); } /** * Completely replace the cached URI -> ID mapping + * + * @param array $uriToIdMapping */ - protected function replaceUriToIdMapping(array $mapping): void + protected function replaceUriToIdMapping(array $uriToIdMapping): void { - Cache::forever(static::URI_TO_ID_MAPPING, $mapping); + // Replace the URI -> ID mapping with the given one. + // This is done "atomically" with regards to the cache. + // Note that concurrent read and writes can result in lost updates. + // And thus in an invalid state. + Cache::forever(static::URI_TO_ID_MAPPING, $uriToIdMapping); } } diff --git a/tests/Observers/PageRoutesObserver.test.php b/tests/Observers/PageRoutesObserver.test.php index dc3a3f0..ab6984d 100644 --- a/tests/Observers/PageRoutesObserver.test.php +++ b/tests/Observers/PageRoutesObserver.test.php @@ -237,6 +237,283 @@ ->values() ->toArray(); }); + + it('properly updates itself and descendants when changing which page is the parent (BelongsTo#associate and BelongsTo#dissociate)', function () { + /** + * @var Page $page + */ + $page = Page::create([ + 'title' => 'My title', + 'slug' => 'my-slug', + 'blocks' => [], + 'parent_id' => null, + ]); + + /** + * @var Page $child1 + */ + $child1 = Page::create([ + 'title' => 'My child 1', + 'slug' => 'child-1', + 'blocks' => [], + 'parent_id' => $page->id, + ]); + + /** + * @var Page $child2 + */ + $child2 = Page::create([ + 'title' => 'My child 2', + 'slug' => 'child-2', + 'blocks' => [], + 'parent_id' => $page->id, + ]); + + /** + * @var Page $child3 + */ + $child3 = Page::create([ + 'title' => 'My child 3', + 'slug' => 'child-3', + 'blocks' => [], + 'parent_id' => $page->id, + ]); + + /** + * @var Page $childOfChild + */ + $childOfChild = Page::create([ + 'title' => 'Subchild 1', + 'slug' => 'subchild-1', + 'blocks' => [], + 'parent_id' => $child2->id, + ]); + + /** + * @var Page[] $descendants + */ + $descendants = [$child1, $child2, $child3, $childOfChild]; + $oldUrlSets = array_map(fn (Page $page) => $page->getAllUrls(), $descendants); + + // $child2->parent_id = $child1->id; + $child2->parent()->associate($child1); + /* $child2->update([ + 'parent_id' => $child1->id, + ]); */ + $child2->save(); + + // $child3->parent_id = null; + $child3->parent()->dissociate(); + /* $child2->update([ + 'parent_id' => null, + ]); */ + $child3->save(); + + foreach ($descendants as $descendant) { + $descendant->refresh(); + } + + $newUrlSets = array_map(fn (Page $page) => $page->getAllUrls(), $descendants); + + expect($newUrlSets)->not->toEqual($oldUrlSets); + + $allUrls = FilamentFabricator::getPageUrls(); + $allUrls = collect($allUrls) + ->sort() + ->values() + ->toArray(); + + $expectedUrls = collect([ + '/my-slug', + '/my-slug/child-1', + '/my-slug/child-1/child-2', + '/child-3', + '/my-slug/child-1/child-2/subchild-1', + ])->sort() + ->values() + ->toArray(); + + expect($allUrls)->toEqual($expectedUrls); + }); + + it('properly updates itself and descendants when changing which page is the parent (Model#update)', function () { + /** + * @var Page $page + */ + $page = Page::create([ + 'title' => 'My title', + 'slug' => 'my-slug', + 'blocks' => [], + 'parent_id' => null, + ]); + + /** + * @var Page $child1 + */ + $child1 = Page::create([ + 'title' => 'My child 1', + 'slug' => 'child-1', + 'blocks' => [], + 'parent_id' => $page->id, + ]); + + /** + * @var Page $child2 + */ + $child2 = Page::create([ + 'title' => 'My child 2', + 'slug' => 'child-2', + 'blocks' => [], + 'parent_id' => $page->id, + ]); + + /** + * @var Page $child3 + */ + $child3 = Page::create([ + 'title' => 'My child 3', + 'slug' => 'child-3', + 'blocks' => [], + 'parent_id' => $page->id, + ]); + + /** + * @var Page $childOfChild + */ + $childOfChild = Page::create([ + 'title' => 'Subchild 1', + 'slug' => 'subchild-1', + 'blocks' => [], + 'parent_id' => $child2->id, + ]); + + /** + * @var Page[] $descendants + */ + $descendants = [$child1, $child2, $child3, $childOfChild]; + $oldUrlSets = array_map(fn (Page $page) => $page->getAllUrls(), $descendants); + + $child2->update([ + 'parent_id' => $child1->id, + ]); + + $child3->update([ + 'parent_id' => null, + ]); + + foreach ($descendants as $descendant) { + $descendant->refresh(); + } + + $newUrlSets = array_map(fn (Page $page) => $page->getAllUrls(), $descendants); + + expect($newUrlSets)->not->toEqual($oldUrlSets); + + $allUrls = FilamentFabricator::getPageUrls(); + $allUrls = collect($allUrls) + ->sort() + ->values() + ->toArray(); + + $expectedUrls = collect([ + '/my-slug', + '/my-slug/child-1', + '/my-slug/child-1/child-2', + '/child-3', + '/my-slug/child-1/child-2/subchild-1', + ])->sort() + ->values() + ->toArray(); + + expect($allUrls)->toEqual($expectedUrls); + }); + + it('properly updates itself and descendants when changing which page is the parent (manual change and Model#save)', function () { + /** + * @var Page $page + */ + $page = Page::create([ + 'title' => 'My title', + 'slug' => 'my-slug', + 'blocks' => [], + 'parent_id' => null, + ]); + + /** + * @var Page $child1 + */ + $child1 = Page::create([ + 'title' => 'My child 1', + 'slug' => 'child-1', + 'blocks' => [], + 'parent_id' => $page->id, + ]); + + /** + * @var Page $child2 + */ + $child2 = Page::create([ + 'title' => 'My child 2', + 'slug' => 'child-2', + 'blocks' => [], + 'parent_id' => $page->id, + ]); + + /** + * @var Page $child3 + */ + $child3 = Page::create([ + 'title' => 'My child 3', + 'slug' => 'child-3', + 'blocks' => [], + 'parent_id' => $page->id, + ]); + + /** + * @var Page $childOfChild + */ + $childOfChild = Page::create([ + 'title' => 'Subchild 1', + 'slug' => 'subchild-1', + 'blocks' => [], + 'parent_id' => $child2->id, + ]); + + $descendants = [$child1, $child2, $child3, $childOfChild]; + $oldUrlSets = array_map(fn (Page $page) => $page->getAllUrls(), $descendants); + + $child2->parent_id = $child1->id; + $child2->save(); + + $child3->parent_id = null; + $child3->save(); + + foreach ($descendants as $descendant) { + $descendant->refresh(); + } + + $newUrlSets = array_map(fn (Page $page) => $page->getAllUrls(), $descendants); + + expect($newUrlSets)->not->toEqual($oldUrlSets); + + $allUrls = FilamentFabricator::getPageUrls(); + $allUrls = collect($allUrls) + ->sort() + ->values() + ->toArray(); + + $expectedUrls = collect([ + '/my-slug', + '/my-slug/child-1', + '/my-slug/child-1/child-2', + '/child-3', + '/my-slug/child-1/child-2/subchild-1', + ])->sort() + ->values() + ->toArray(); + + expect($allUrls)->toEqual($expectedUrls); + }); }); describe('#deleting($page)', function () { From cd6f9c4a3c646a506a0534292d1768f337fc17a3 Mon Sep 17 00:00:00 2001 From: Voltra Date: Tue, 27 Feb 2024 18:29:17 +0000 Subject: [PATCH 23/25] Fix invalid syntax for nullable intersection --- src/Services/PageRoutesService.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Services/PageRoutesService.php b/src/Services/PageRoutesService.php index 8ff4a0b..2ed4c16 100644 --- a/src/Services/PageRoutesService.php +++ b/src/Services/PageRoutesService.php @@ -47,7 +47,7 @@ public function getPageIdFromUri(string $uri): int * * @return null|(Page&Model) */ - public function getPageFromUri(string $uri): null|(Page&Model) + public function getPageFromUri(string $uri): ?Page { $id = $this->getPageIdFromUri($uri); From 1052a494de01781ef7d76ba790e48a395e0baa8e Mon Sep 17 00:00:00 2001 From: Voltra Date: Tue, 27 Feb 2024 19:43:15 +0000 Subject: [PATCH 24/25] Test for internal consistency, format, typos --- src/FilamentFabricatorManager.php | 2 +- src/Models/Contracts/HasPageUrls.php | 1 + src/Services/PageRoutesService.php | 16 +- tests/Observers/PageRoutesObserver.test.php | 196 ++++++++++++-------- tests/TestCase.php | 7 +- 5 files changed, 129 insertions(+), 93 deletions(-) diff --git a/src/FilamentFabricatorManager.php b/src/FilamentFabricatorManager.php index 0f4a852..11f7527 100644 --- a/src/FilamentFabricatorManager.php +++ b/src/FilamentFabricatorManager.php @@ -39,7 +39,7 @@ class FilamentFabricatorManager */ protected PageRoutesService $routesService; - public function __construct(PageRoutesService $routesService = null) + public function __construct(?PageRoutesService $routesService = null) { $this->routesService = $routesService ?? resolve(PageRoutesService::class); diff --git a/src/Models/Contracts/HasPageUrls.php b/src/Models/Contracts/HasPageUrls.php index 88050c8..d245a5d 100644 --- a/src/Models/Contracts/HasPageUrls.php +++ b/src/Models/Contracts/HasPageUrls.php @@ -6,6 +6,7 @@ interface HasPageUrls { /** * Get the default arguments for URL generation + * * @invariant HasPageUrls#getDefaultUrlCacheArgs() must always return the same value, regardless of app/user configuration */ public function getDefaultUrlCacheArgs(): array; diff --git a/src/Services/PageRoutesService.php b/src/Services/PageRoutesService.php index 2ed4c16..85fc33a 100644 --- a/src/Services/PageRoutesService.php +++ b/src/Services/PageRoutesService.php @@ -14,8 +14,8 @@ // - PageRoutesService::ID_TO_URI_MAPPING that maps from ID to URIS (one-to-many) // - Page::getUrl (and thus Page::getAllUrl) // -// Syncinc and consistencies should be handle via the service. -// As such, it's responsible for maintaining internale consistency +// Syncing and consistency should be handle via the service. +// As such, it's responsible for maintaining internal consistency // and hiding/encapsulating implementation details. // // It relies on the extension points defined by Z3d0X\FilamentFabricator\Models\Contracts\HasPageUrls @@ -63,7 +63,7 @@ public function getPageFromUri(string $uri): ?Page /** * Update the cached URLs for the given page (as well as all its descendants') */ - public function updateUrlsOf(Page $page): void + public function updateUrlsOf(Page&Model $page): void { // We mutate the mapping without events to ensure we don't have "concurrent" // modifications of the same mapping. This allows us to skip the use of locks @@ -110,7 +110,7 @@ public function findPageOrFail(string $uri): Page&Model $id = $this->getPageIdFromUri($uri); // If the page doesn't exists, we know getPageIdFromUri - // will return -1. Thus findOrFail will fail as expected. + // will return -1. Thus, findOrFail will fail as expected. return FilamentFabricator::getPageModel()::findOrFail($id); } @@ -205,7 +205,7 @@ protected function getUrisForPage(Page $page): array * @param array $uriToIdMapping - The URI -> ID mapping (as a reference, to be modified in-place) * @return void */ - protected function updateUrlsAndDescendantsOf(Page $page, array &$uriToIdMapping) + protected function updateUrlsAndDescendantsOf(Page&Model $page, array &$uriToIdMapping) { // First ensure consistency by removing any trace of the old URLs // for the given page. Whether local or in the URI to ID mapping. @@ -237,6 +237,10 @@ protected function updateUrlsAndDescendantsOf(Page $page, array &$uriToIdMapping // TODO: Make it work with loadMissing instead of load to reduce the number of useless DB queries $page->load(['allChildren']); foreach ($page->allChildren as $childPage) { + /** + * @var Page&Model $childPage + */ + // A change in a parent page will always result // in a change to its descendant. As such, // we need to recompute everything that's @@ -254,7 +258,7 @@ protected function updateUrlsAndDescendantsOf(Page $page, array &$uriToIdMapping protected function unsetOldUrlsOf(Page $page, array &$uriToIdMapping) { // When we're hitting this path, caches haven't been invalidated yet. - // Thus we don't need to query the mappings to get the old URLs. + // Thus, we don't need to query the mappings to get the old URLs. $oldUrlSet = collect($page->getAllUrls())->lazy()->sort()->all(); // Once we're done collecting the previous URLs, and since we want diff --git a/tests/Observers/PageRoutesObserver.test.php b/tests/Observers/PageRoutesObserver.test.php index ab6984d..dca2218 100644 --- a/tests/Observers/PageRoutesObserver.test.php +++ b/tests/Observers/PageRoutesObserver.test.php @@ -24,13 +24,18 @@ 'parent_id' => null, ]); - $afterUrls = FilamentFabricator::getPageUrls(); + $sortUrls = fn (array $urls) => collect($urls) + ->sort() + ->values() + ->toArray(); + + $afterUrls = $sortUrls(FilamentFabricator::getPageUrls()); expect($afterUrls)->not->toEqual($beforeUrls); - $pageUrls = $page->getAllUrls(); + $pageUrls = $sortUrls($page->getAllUrls()); - expect($afterUrls)->toContain(...$pageUrls); + expect($afterUrls)->toEqual($pageUrls); }); it('properly works on child pages', function () { @@ -54,20 +59,26 @@ 'parent_id' => $page->id, ]); - $allUrls = FilamentFabricator::getPageUrls(); - $allUrls = collect($allUrls) + $sortUrls = fn (array $urls) => collect($urls) ->sort() ->values() ->toArray(); - $expectedUrls = collect([ + $allUrls = FilamentFabricator::getPageUrls(); + $allUrls = $sortUrls($allUrls); + + $fromPages = $sortUrls([ + ...$page->getAllUrls(), + ...$child->getAllUrls(), + ]); + + $expectedUrls = $sortUrls([ '/my-slug', '/my-slug/my-stuff', - ])->sort() - ->values() - ->toArray(); + ]); expect($allUrls)->toEqual($expectedUrls); + expect($fromPages)->toEqual($expectedUrls); /** * @var Page $page @@ -80,20 +91,22 @@ ]); $allUrls = FilamentFabricator::getPageUrls(); - $allUrls = collect($allUrls) - ->sort() - ->values() - ->toArray(); + $allUrls = $sortUrls($allUrls); - $expectedUrls = collect([ + $fromPages = $sortUrls([ + ...$page->getAllUrls(), + ...$child->getAllUrls(), + ...$descendant->getAllUrls(), + ]); + + $expectedUrls = $sortUrls([ '/my-slug', '/my-slug/my-stuff', '/my-slug/my-stuff/abc-xyz', - ])->sort() - ->values() - ->toArray(); + ]); expect($allUrls)->toEqual($expectedUrls); + expect($fromPages)->toEqual($expectedUrls); }); }); @@ -133,11 +146,19 @@ $page->slug = 'not-my-slug'; $page->save(); - $newUrls = $page->getAllUrls(); + $sortUrls = fn (array $urls) => collect($urls) + ->sort() + ->values() + ->toArray(); + + $expected = $sortUrls(['/not-my-slug']); - $allUrls = FilamentFabricator::getPageUrls(); + $newUrls = $sortUrls($page->getAllUrls()); - expect($allUrls)->toContain(...$newUrls); + $allUrls = $sortUrls(FilamentFabricator::getPageUrls()); + + expect($allUrls)->toEqual($expected); + expect($newUrls)->toEqual($expected); }); it('properly updates all child (and descendant) routes', function () { @@ -179,10 +200,16 @@ 'parent_id' => $child2->id, ]); + $sortUrls = fn (array $urls) => collect($urls) + ->sort() + ->values() + ->toArray(); + /** * @var Page[] $descendants */ $descendants = [$child1, $child2, $child3, $childOfChild]; + $pages = [$page, ...$descendants]; $oldUrlSets = array_map(fn (Page $page) => $page->getAllUrls(), $descendants); $page->slug = 'not-my-slug'; @@ -196,23 +223,20 @@ expect($newUrlSets)->not->toEqual($oldUrlSets); - $allUrls = FilamentFabricator::getPageUrls(); - $allUrls = collect($allUrls) - ->sort() - ->values() - ->toArray(); + $allUrls = $sortUrls(FilamentFabricator::getPageUrls()); + + $fromPages = $sortUrls(collect($pages)->flatMap(fn (Page $page) => $page->getAllUrls())->toArray()); - $expectedUrls = collect([ + $expectedUrls = $sortUrls([ '/not-my-slug', '/not-my-slug/child-1', '/not-my-slug/child-2', '/not-my-slug/child-3', '/not-my-slug/child-2/subchild-1', - ])->sort() - ->values() - ->toArray(); + ]); expect($allUrls)->toEqual($expectedUrls); + expect($fromPages)->toEqual($expectedUrls); $child2->slug = 'not-child-2-xyz'; $child2->save(); @@ -221,21 +245,19 @@ $descendant->refresh(); } - $allUrls = FilamentFabricator::getPageUrls(); - $allUrls = collect($allUrls) - ->sort() - ->values() - ->toArray(); + $allUrls = $sortUrls(FilamentFabricator::getPageUrls()); + $fromPages = $sortUrls(collect($pages)->flatMap(fn (Page $page) => $page->getAllUrls())->toArray()); - $expectedUrls = collect([ + $expectedUrls = $sortUrls([ '/not-my-slug', '/not-my-slug/child-1', '/not-my-slug/not-child-2-xyz', '/not-my-slug/child-3', '/not-my-slug/not-child-2-xyz/subchild-1', - ])->sort() - ->values() - ->toArray(); + ]); + + expect($allUrls)->toEqual($expectedUrls); + expect($fromPages)->toEqual($expectedUrls); }); it('properly updates itself and descendants when changing which page is the parent (BelongsTo#associate and BelongsTo#dissociate)', function () { @@ -289,24 +311,22 @@ 'parent_id' => $child2->id, ]); + $sortUrls = fn (array $urls) => collect($urls) + ->sort() + ->values() + ->toArray(); + /** * @var Page[] $descendants */ $descendants = [$child1, $child2, $child3, $childOfChild]; + $pages = [$page, ...$descendants]; $oldUrlSets = array_map(fn (Page $page) => $page->getAllUrls(), $descendants); - // $child2->parent_id = $child1->id; $child2->parent()->associate($child1); - /* $child2->update([ - 'parent_id' => $child1->id, - ]); */ $child2->save(); - // $child3->parent_id = null; $child3->parent()->dissociate(); - /* $child2->update([ - 'parent_id' => null, - ]); */ $child3->save(); foreach ($descendants as $descendant) { @@ -315,25 +335,22 @@ $newUrlSets = array_map(fn (Page $page) => $page->getAllUrls(), $descendants); + $fromPages = $sortUrls(collect($pages)->flatMap(fn (Page $page) => $page->getAllUrls())->toArray()); + expect($newUrlSets)->not->toEqual($oldUrlSets); - $allUrls = FilamentFabricator::getPageUrls(); - $allUrls = collect($allUrls) - ->sort() - ->values() - ->toArray(); + $allUrls = $sortUrls(FilamentFabricator::getPageUrls()); - $expectedUrls = collect([ + $expectedUrls = $sortUrls([ '/my-slug', '/my-slug/child-1', '/my-slug/child-1/child-2', '/child-3', '/my-slug/child-1/child-2/subchild-1', - ])->sort() - ->values() - ->toArray(); + ]); expect($allUrls)->toEqual($expectedUrls); + expect($fromPages)->toEqual($expectedUrls); }); it('properly updates itself and descendants when changing which page is the parent (Model#update)', function () { @@ -387,10 +404,16 @@ 'parent_id' => $child2->id, ]); + $sortUrls = fn (array $urls) => collect($urls) + ->sort() + ->values() + ->toArray(); + /** * @var Page[] $descendants */ $descendants = [$child1, $child2, $child3, $childOfChild]; + $pages = [$page, ...$descendants]; $oldUrlSets = array_map(fn (Page $page) => $page->getAllUrls(), $descendants); $child2->update([ @@ -407,25 +430,22 @@ $newUrlSets = array_map(fn (Page $page) => $page->getAllUrls(), $descendants); + $fromPages = $sortUrls(collect($pages)->flatMap(fn (Page $page) => $page->getAllUrls())->toArray()); + expect($newUrlSets)->not->toEqual($oldUrlSets); - $allUrls = FilamentFabricator::getPageUrls(); - $allUrls = collect($allUrls) - ->sort() - ->values() - ->toArray(); + $allUrls = $sortUrls(FilamentFabricator::getPageUrls()); - $expectedUrls = collect([ + $expectedUrls = $sortUrls([ '/my-slug', '/my-slug/child-1', '/my-slug/child-1/child-2', '/child-3', '/my-slug/child-1/child-2/subchild-1', - ])->sort() - ->values() - ->toArray(); + ]); expect($allUrls)->toEqual($expectedUrls); + expect($fromPages)->toEqual($expectedUrls); }); it('properly updates itself and descendants when changing which page is the parent (manual change and Model#save)', function () { @@ -479,7 +499,13 @@ 'parent_id' => $child2->id, ]); + $sortUrls = fn (array $urls) => collect($urls) + ->sort() + ->values() + ->toArray(); + $descendants = [$child1, $child2, $child3, $childOfChild]; + $pages = [$page, ...$descendants]; $oldUrlSets = array_map(fn (Page $page) => $page->getAllUrls(), $descendants); $child2->parent_id = $child1->id; @@ -494,25 +520,23 @@ $newUrlSets = array_map(fn (Page $page) => $page->getAllUrls(), $descendants); + $fromPages = $sortUrls(collect($pages)->flatMap(fn (Page $page) => $page->getAllUrls())->toArray()); + expect($newUrlSets)->not->toEqual($oldUrlSets); $allUrls = FilamentFabricator::getPageUrls(); - $allUrls = collect($allUrls) - ->sort() - ->values() - ->toArray(); + $allUrls = $sortUrls($allUrls); - $expectedUrls = collect([ + $expectedUrls = $sortUrls([ '/my-slug', '/my-slug/child-1', '/my-slug/child-1/child-2', '/child-3', '/my-slug/child-1/child-2/subchild-1', - ])->sort() - ->values() - ->toArray(); + ]); expect($allUrls)->toEqual($expectedUrls); + expect($fromPages)->toEqual($expectedUrls); }); }); @@ -550,6 +574,9 @@ 'parent_id' => null, ]); + /** + * @var Page $child + */ $child = Page::create([ 'title' => 'My child page', 'slug' => 'my-child-page', @@ -565,7 +592,10 @@ $urls = FilamentFabricator::getPageUrls(); - expect($urls)->toEqual(['/my-child-page']); + $expected = ['/my-child-page']; + + expect($urls)->toEqual($expected); + expect($child->getAllUrls())->toEqual($expected); }); it('attaches the children to $page\'s parent if it had one', function () { @@ -599,26 +629,28 @@ 'parent_id' => $child->id, ]); + $sortUrls = fn (array $urls) => collect($urls) + ->sort() + ->values() + ->toArray(); + $child->delete(); $descendant->refresh(); $page->refresh(); expect($descendant->parent_id)->toBe($page->id); - $urls = collect(FilamentFabricator::getPageUrls()) - ->sort() - ->values() - ->toArray(); + $urls = $sortUrls(FilamentFabricator::getPageUrls()); - $expected = collect([ + $expected = $sortUrls([ '/my-slug', '/my-slug/my-sub-page', - ]) - ->sort() - ->values() - ->toArray(); + ]); + + $fromPages = $sortUrls(collect([$page, $descendant])->flatMap(fn (Page $page) => $page->getAllUrls())->toArray()); expect($urls)->toEqual($expected); + expect($fromPages)->toEqual($expected); }); }); }); diff --git a/tests/TestCase.php b/tests/TestCase.php index d181566..2040ff0 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -41,12 +41,11 @@ public function getEnvironmentSetUp($app) { config()->set('database.default', 'testing'); - - $migration = include __DIR__.'/../database/migrations/create_pages_table.php.stub'; + $migration = include __DIR__ . '/../database/migrations/create_pages_table.php.stub'; $migration->up(); - $migration = include __DIR__.'/../database/migrations/fix_slug_unique_constraint_on_pages_table.php.stub'; + $migration = include __DIR__ . '/../database/migrations/fix_slug_unique_constraint_on_pages_table.php.stub'; $migration->up(); - + } } From 5b4c30dec1aac55eba61eb4772298d1e87ed57b1 Mon Sep 17 00:00:00 2001 From: Voltra Date: Fri, 7 Jun 2024 11:57:43 +0200 Subject: [PATCH 25/25] Allow string-based IDs in the new route caching system --- src/FilamentFabricatorManager.php | 2 +- src/Services/PageRoutesService.php | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/FilamentFabricatorManager.php b/src/FilamentFabricatorManager.php index 606867c..b4d15b7 100644 --- a/src/FilamentFabricatorManager.php +++ b/src/FilamentFabricatorManager.php @@ -195,7 +195,7 @@ public function getPageUrls(): array return $this->routesService->getAllUrls(); } - public function getPageUrlFromId(int $id, bool $prefixSlash = false, array $args = []): ?string + public function getPageUrlFromId(int|string $id, bool $prefixSlash = false, array $args = []): ?string { /** @var ?PageContract $page */ $page = $this->getPageModel()::query()->find($id); diff --git a/src/Services/PageRoutesService.php b/src/Services/PageRoutesService.php index 85fc33a..ead360b 100644 --- a/src/Services/PageRoutesService.php +++ b/src/Services/PageRoutesService.php @@ -28,9 +28,9 @@ class PageRoutesService /** * Get the ID of the Page model to which the given URI is associated, -1 if non matches * - * @return int The page's ID, or -1 on failure + * @return int|string The page's ID, or -1 on failure */ - public function getPageIdFromUri(string $uri): int + public function getPageIdFromUri(string $uri): int|string { // Query the (URI -> ID) mapping based on the user provided URI. // The mapping expect a URI that starts with a / @@ -135,7 +135,7 @@ public function getAllUrls(): array /** * Get the URI -> ID mapping * - * @return array + * @return array */ protected function getUriToIdMapping(): array { @@ -164,7 +164,7 @@ protected function getUriToIdMapping(): array /** * Get the ID -> URI[] mapping * - * @return array + * @return array */ protected function getIdToUrisMapping(): array { @@ -305,7 +305,7 @@ protected function forgetPageLocalCache(Page $page) /** * Completely replaced the cached ID -> URI[] mapping * - * @param array $idToUriMapping + * @param array $idToUriMapping */ protected function replaceIdToUriMapping(array $idToUriMapping): void { @@ -319,7 +319,7 @@ protected function replaceIdToUriMapping(array $idToUriMapping): void /** * Completely replace the cached URI -> ID mapping * - * @param array $uriToIdMapping + * @param array $uriToIdMapping */ protected function replaceUriToIdMapping(array $uriToIdMapping): void {