"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[bot]]

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]

I am concerned that changes to PageController could be considered as "breaking", as users may have overridden this?

[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 https://github.com/Z3d0X/filament-fabricator/issues/75#issuecomment-1826744626, 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

[Voltra]

@Ray-vV I would like your input on the current state of the PR, does it work as expected on your previously used testing environment? Especially the deletion part, and the commands since they're the latest components/additions.

[Ray-vV]

@Ray-vV I would like your input on the current state of the PR, does it work as expected on your previously used testing environment? Especially the deletion part, and the commands since they're the latest components/additions.

Will do! I will probably have some time tomorrow. Do you need anything specific or just an idea of how the changes are working for me?

[Voltra]

@Ray-vV I would like your input on the current state of the PR, does it work as expected on your previously used testing environment? Especially the deletion part, and the commands since they're the latest components/additions.

Will do! I will probably have some time tomorrow. Do you need anything specific or just an idea of how the changes are working for me?

I think it'd mostly be about testing cases for:

• Change of the routing prefix & cache invalidation by running the new CLI command
• Deletion of a parent page re-attaches it to that page's parent or makes it a root-level page

[Ray-vV]

Hi, I had some time do to some local testing for the deletion case. As of right now it seems that deleting a parent page still deletes all children consequently.

FYI, my project pages are following this hierarchy:

Worldwide->country->division->city

I'm using a large dataset to generate the pages in the same order. By looping through countries, then divisions, then cities, attaching parent_ids from the page 1 level higher in the hierarchy (for example, all country pages have parent_id that belongs to 'worldwide' page, division parent_ids have the country pages id, etc).

• Tested this by deleting a country record from my PageResource table
• Result was that all children (cities and division pages belonging to the country) had been deleted as well
• I haven't seen any errors in my logs

Then I deleted my worldwide page, which resulted in having 0 pages left.

Just to clarify, the intention was that all children under the deleted parent should be reattached to the parent of the deleted parent? If it has no parent, make it a root level page, right?

Also, I'm unclear on how to test the routing prefix caching command. Should it be ran after every change to one of the pages, or just when a parent which possesses children has been changed? Finally, I have some changes to my PageResource which changes some of the resource values:

For the possibility of prefixing a country code to the URL while still using the Filament Fabricator functionality , I have added a domain column which contains the app_url with the prefixed country code ({country_code}.app_url.com' ). I still use the slug value to append the rest of the URL where necesarry ('country_code}.app_url.com/{division_slug}/{city_slug}'. I'm also just using the original title value as a title, not as a part of the URL. Due to these changes I'm unsure if it will still work as intended for me. If I'm wrong and you'd still like me to test this I definitely can though!

[Voltra]

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

[Z3d0X]

Doesn't seem to be invalidating the cache properly

https://github.com/Z3d0X/filament-fabricator/assets/75579178/d6bfbcea-3704-43ef-951b-e7d65fb5776d

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 https://github.com/Z3d0X/filament-fabricator/pull/119#issuecomment-1902490712 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

[panakour]

any update on this?