statamic / eloquent-driver

Provides support for storing your Statamic data in a database, rather than flat files.
https://statamic.dev/tips/storing-content-in-a-database
MIT License
104 stars 71 forks source link

The Blink cache for find() in the EntryRepository causes issues #219

Closed SylvesterDamgaard closed 5 months ago

SylvesterDamgaard commented 7 months ago

In cases where you want to fetch the "original" version of the entry

Running this code will not fetch the current version from the database, but the version originally fetched.

$entry = Entry::find($entry->id());

This is a problem in e.g. EntrySaving listeners where we want to compare what has changed. An example is the redirect addon from Rias. In below example the old and new URI is always the same, so no redirects will be created on URI changes as expected.

https://github.com/riasvdv/statamic-redirect/blob/fb1c3bbd97254a24044b1b4babfb679cf3cea8c8/src/Listeners/CacheOldUri.php#L17

public function handle(EntrySaving $entrySaving)
    {
        if (! config('statamic.redirect.enable', true)) {
            return;
        }

        $entry = Entry::find($entrySaving->entry->id());

        if (! $entry || ! $uri = $entry->uri()) {
            return;
        }

        Cache::put('redirect-entry-uri-before', $uri);
    }
ryanmitchell commented 7 months ago

The reason we added the blink cache was to prevent multiple duplicate lookups in the same request, for example when getting a parent item, or children of an entry. I would not be keen to remove that functionality as it definitely will have a huge performance impact.

I would be inclined to say that this should therefore be addressed on the Redirects add-on site.

If https://github.com/statamic/cms/pull/5502 were to merge then getOriginal method could be used. Until then the plug-in could check if the entry has a model ($entrySaving->model()) and if so could $entry = Entry::fromModel($model) to get the 'original' model.

Unless you have a different thought @jasonvarga ?

jasonvarga commented 7 months ago

We could listen for EntrySaved and do Blink::forget('eloquent-entry-$id').

ryanmitchell commented 7 months ago

The blink cache already gets forgotten on save. The add-on is trying to get the difference between the original/stored value and the value thats about to be saved in a saving listener, the issue is that Entry::find() is then getting the blinked value.

jasonvarga commented 7 months ago

Gotcha, sorry. Yeah #5502 would be useful here. Since there's a necessary change either way on the redirects side, I'd suggest they use the model workaround for now until #5502 is able to be merged.

Another option is to forget the blink in the redirect addon too.

+Blink::forget('eloquent-entry-'.$entrySaving->entry->id);
$entry = Entry::find($entrySaving->entry->id());
ryanmitchell commented 7 months ago

Yeah that one-liner is probably easier. I'll PR it and see what Rias thinks. Appreciate your thoughts.