"Smart" route URLs caching

[Voltra]

Initial draft of the new route caching system as described in #116

A few quirks need to be ironed out (mainly completely removing any and all traces of the previous route caching system).

The args are mainly here to enable project-dependent customization for generating multiple "independent" URLs per page (e.g. translatable pages)

[what-the-diff]

PR Summary

• Enhanced the management of Page URLs
  We have created various classes and traits to streamline the process of generating, storing, updating, and retrieving URLs for Page models. These efforts improve website load time by leveraging an advanced caching mechanism.

• Introduction of PageRoutesObserver class
  This new class observes events like the creation, update, restoration, and deletion of Page models. It uses the PageRoutesService to freshen the stored URLs for associated pages.

• Introduction of HandlesPageUrls trait
  Added to the Page model, which contains methods for generating and caching page URLs.

• Introduction of PageRoutesService class
  This class manages the updating of the stored URL mappings for Page models. It helps enhance the site's performance by keeping the URLs mapped correctly.

• Upgrade of PageController
  The PageController now uses the PageRoutesService for resolving the page model when a URI is not provided. This change enables more efficient page load times and reduces the chances of page not found errors.

• Renaming and creation of various methods in HandlesPageUrls trait
  Methods for generating page URLs, fetching available URLs, and getting the default arguments for URL generation have been introduced. Also, the getUriKey method was renamed to getUrlCacheKey, which more aptly describes its functionality.

• Services indicated in FilamentFabricatorServiceProvider
  The PageRoutesService dependency and other related alterations were added to the FilamentFabricatorServiceProvider, helping in resolving the PageRoutesService instance and updating the page binding.

• New methods in the PageRoutesService class
  Several methods were created to get the Page associated with a given URI, fetch the URI to ID mapping, get the cached URIs for a specific page, remove old URLs from the cached mappings, and replace the cached ID to URI mapping. These enhancements ensure the URL mappings remain up-to-date, reducing potential errors and improving system efficiency.

[Ray-vV]

@Voltra Thanks for your work on this firstly! I've added your code to my project and noticed I was getting the following error:

Then I noticed the method call inside HandlesPageUrls.php

    public function getAllUrls(): array {
        return array_map([$this, 'getUrl'], $this->getAllUrlCacheKeysArgs());
    }

The call is to $this->getAllUrlCacheKeysArgs(), but that method doesn't exist in the trait.

After changing this call to $this->getAllCacheKeyArgs(), this problem was fixed for me.

 public function getAllUrls(): array {
        return array_map([$this, 'getUrl'], $this->getAllCacheKeyArgs());
    }

I will start testing the code inside my project, so if you'd like additional feedback I could provide that.

[Ray-vV]

See #1

[Voltra]

Whoops, that's what I get for using github codespaces instead of my usual setup, will correct the typo

[Z3d0X]

For this maybe we can introduce an artisan command? then in the docs mention that if routing prefix changes, run the command to clear and re-compute all routes?

[Z3d0X]

How should deleted case be handled?

[Voltra]

For now it seems unclear. There's no way to have a "re-attach to root" strategy, since there's no concept of root. If we still have the page's relationship data, we could use a linked-list inspired deletion procedure (attaching the parent of the children of this page to be this page's parent)

[Voltra]

See the deleting method for that implementation

[Voltra]

I've tried to keep the exact same interface, but just change the implementation itself. The only breaking change is in the manager, which now has a parameter to its constructor. A temporary solution would be to make it nullable (defaulting to null would keep the same arity) and using resolve is the provided value is null

[Z3d0X]

If you could work out locale support from #75 (comment), I'd be happy to make a 3.x release

[Voltra]

If you could work out locale support from #75 (comment), I'd be happy to make a 3.x release

I think it's gonna be difficult and/or tricky to make it 1st-party.

I think it should be a project-dependent configuration of the Page model class. Especially since it heavily relies on how the translation itself is made.

For instance:

namespace App\Models\Overrides;

use Filament\Facades\Filament;
use Filament\SpatieLaravelTranslatablePlugin;
use Spatie\Sitemap\Contracts\Sitemapable;
use Spatie\Sitemap\Tags\Url;
use Spatie\Translatable\HasTranslations;

class Page extends \Z3d0X\FilamentFabricator\Models\Page implements Sitemapable {
    use HasTranslations;

    public array $translatable = [
        'title',
        'slug',
        'blocks',
    ];

    protected $table = 'pages';

    /**
     * {@inheritDoc}
     */
    public function getDefaultUrlCacheArgs(): array
    {
        return [
            'locale' => 'fr',
        ];
    }

    /**
     * {@inheritDoc}
     */
    public function getUrlCacheKey(array $args = []): string
    {
        $keyArgs = collect($this->getDefaultUrlArgs())->merge($args)->all();
        $locale = $keyArgs['locale'];
        $id = $this->id;

        return "page-url--{$locale}--{$id}";
    }

    /**
     * {@inheritDoc}
     */
    public function getAllUrlCacheKeysArgs(): array
    {
        /**
         * @var SpatieLaravelTranslatablePlugin $translatable
         */
        $translatable = Filament::getPlugin((new SpatieLaravelTranslatablePlugin())->getId());

        return array_map(fn (string $locale) => [
            'locale' => $locale,
        ], $translatable?->getDefaultLocales() ?: ['fr', 'en']);
    }

    public function toSitemapTag(): Url|string|array
    {
        $entry = Url::create(url($this->getUrl([
            'locale' => $this->getLocale(),
        ])));

        $argsSets = $this->getAllUrlCacheKeysArgs();

        array_walk($argsSets, function (array $args) use ($entry) {
            $url = $this->getUrl($args);
            $entry->addAlternate(url($url), $args['locale']);
        });

        $entry->setLastModificationDate($this->updated_at)
            ->setChangeFrequency(Url::CHANGE_FREQUENCY_DAILY);

        return $entry;
    }
}

In my case, the locale detection/switching is made via URLs using a custom middleware:

namespace App\Http\Middleware;

use Illuminate\Http\Request;
use Illuminate\Support\Facades\App;
use Symfony\Component\HttpFoundation\Response;

class LocaleDetector
{
    /**
     * Handle an incoming request.
     *
     * @param  \Closure(\Illuminate\Http\Request): (\Symfony\Component\HttpFoundation\Response)  $next
     */
    public function handle(Request $request, \Closure $next): Response
    {
        $path = $request->decodedPath();

        if ($this->matchesLocale($path, 'en')) {
            App::setLocale('en');
        } else {
            App::setLocale(App::getFallbackLocale());
        }

        return $next($request);
    }

    protected function matchesLocale(string $uri, string $locale): bool
    {
        return $uri === $locale
            || str_starts_with($uri, "{$locale}/")
            || str_starts_with($uri, "/{$locale}/");
    }
}

Not all cases will work the same way. You might want to have the locale in the session, or the user's settings. You might need more arguments to generate a route, etc...

That's why these customization/extension points exist in the first place:

• getDefaultUrlCacheArgs
• getUrlCacheKey
• getAllUrlCacheKeysArgs

[Z3d0X]

I think it's gonna be difficult and/or tricky to make it 1st-party.

I think it should be a project-dependent configuration of the Page model class. Especially since it heavily relies on how the translation itself is made.

Agreed. I think a pinned post on discussions on how this plugin can be extended to support translations should suffice.

@Voltra if there isn't anything else to be added I'm merging this. I have reviewed the code once again, you are right, there doesn't seem to be any breaking changes

[Z3d0X]

Doesn't seem to be invalidating the cache properly

2024-01-21.08-25-43.mp4

The filament-fabricator:clear-routes-cache seems to work fine tho.

[Ray-vV]

When you say deleted, you mean inaccessible or actually deleted from the database? I only observe deletion of page models and re-attach/move things around.

As for the prefix, it's:

1. changing the filament-fabricator.routing.prefix
2. checking that pages aren't accessible at the new URLs (since they're still cached under the old ones)
3. executing the command
4. checking that pages are accessible at the new URLs

Deleted as in using the delete action from the PageResource's table. Looks like it's just a standard filament DeleteAction that deletes the record using the model. So in that case, it should work since you're observing deletion of page models?

[Voltra]

It should, but if there's any sort of on-delete-cascade logic that might be what causes them to be deleted

[Ray-vV]

It should, but if there's any sort of on-delete-cascade logic that might be what causes them to be deleted

I have some relations set up in my db but I don't think I have any on delete cascades. I'll check and make a copy db without the relations for testing if it's the case though.

EDIT ( 2-2 2024)
Turns out I DID have on delete cascades set up in the project for the pages, so that'll explain the deletion of my child pages.
Sadly I haven't been able to test properly yet due to other responsibilities. Will get to that when I can.

Also, I'm writing tests for my project, and was wondering how to best go about writing a smoke test for all Fabricator pages? This is really the first time I'm writing tests so bear with me on that, lol. Learning as I go currently.

I've installed Laravel Pest and I noticed that the FilamentFabricator package has a TestCase included. Is this extendable and would it be preferred to use inside tests that involve Fabricator pages?

[Z3d0X]

I can't merge until #119 (comment) is fixed

[Z3d0X]

I've installed Laravel Pest and I noticed that the FilamentFabricator package has a TestCase included. Is this extendable and would it be preferred to use inside tests that involve Fabricator pages?

Short answer nope. This unrelated to this PR, make a post in the Discussions, if you want a more detailed answer

[Voltra]

I'll do some testing in the next few days.

[Voltra]

Was the project setup with Pest or just regular PHPUnit ? I feel like writing unit tests for this part will definitely help (especially with detecting regressions in the future)

[Z3d0X]

@Voltra setup is for pest, you can see the sample test in the test folder.

I could write basic tests for existing features, then you can add any additional tests for this PR

[Voltra]

Would it be OK if I configured the tests to use the *.test.php naming convention for test files and follow the exact same directory structure as in the source directory (e.g. tests/Commands/MakeLayoutCommand.test.php for src/Commands/MakeLayoutCommand.php)?

[Voltra]

There were a few bugs that the unit tests help catch

[Voltra]

Things should be A-OK @Z3d0X

I've also added a config option that hooks into existing laravel artisan commands to clear and refresh the caches at the same time as the others.

[Voltra]

Just got done fixing a corner-case in updates where changing the parent page would not properly change the URLs. Now all cases should be handled properly and thoroughly tested.

[Voltra]

Should be good to merge