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

Method Asset::clearCaches() not being called #307

Closed andreas-eisenmann closed 5 days ago

andreas-eisenmann commented 2 weeks ago

Bug description

I am currently fixed to version 3.x because we still haven't upgraded to Statamic v5. I've just spent several hours of debugging to find the root cause why all meta data gets lost after moving a previously uploaded asset into another folder. I have custom logik (inside a cli command) which first uploads the asset, then writes all meta data and then later moves the asset to another folder. And here is where the trouble begins:

https://github.com/statamic/eloquent-driver/blob/v3.3.3/src/Assets/Asset.php#L62

Blink::once($this->metaCacheKey(), function () { still delivers empty meta data which has been written before when the file has been uploaded.

When an asset is being saved here we have a call to $this->clearCaches(). Unfortunately this method is declared as private in the statamic source code. The Blink-Cache is not invalidated here.

Maybe you've already hit on this bug, because I've seen a new method clearCaches() in the eloquent driver:

https://github.com/statamic/eloquent-driver/blob/v4.4.0/src/Assets/Asset.php#L226C5-L226C35

This method would solve the problem. But: this method will never be called? It's private, so when save() is being called in \Statamic\Assets\Asset::save() \Statamic\Assets\Asset::clearCaches() will be called and not \Statamic\Eloquent\Assets\Asset::clearCaches(). Am I missing something here?

How to reproduce

Logs

No response

Environment

Environment
Application Name: xxxxxxx
Laravel Version: 10.48.4
PHP Version: 8.2.17
Composer Version: 2.6.5
Environment: production
Debug Mode: ENABLED
URL: localhost
Maintenance Mode: OFF

Cache
Config: NOT CACHED
Events: NOT CACHED
Routes: NOT CACHED
Views: NOT CACHED

Drivers
Broadcasting: log
Cache: statamic
Database: pgsql
Logs: stack / single
Mail: smtp
Queue: sync
Session: file

Locales
Installed
Locales Version: 1.9.0
Protected

Localization
Installed: de, en
LaravelLang\Attributes\Plugin: 2.9.4
LaravelLang\HttpStatuses\Plugin: 3.8.2
LaravelLang\Lang\Plugin: 13.12.0
Protected Locales: de, en
Publisher Version: 14.7.1

Livewire
Livewire: v3.4.12

Statamic
Addons: 3
Antlers: runtime
Sites: 1
Stache Watcher: Enabled
Static Caching: Disabled
Version: 4.53.2 PRO

Statamic Addons
anakadote/statamic-recaptcha: 1.0.14
statamic-rad-pack/runway: 6.6.2
statamic/eloquent-driver: 3.3.3

Statamic Eloquent Driver
Asset Containers: eloquent
Assets: eloquent
Blueprints: eloquent
Collection Trees: eloquent
Collections: eloquent
Entries: eloquent
Forms: eloquent
Global Sets: eloquent
Global Variables: eloquent
Navigation Trees: eloquent
Navigations: eloquent
Revisions: eloquent
Taxonomies: eloquent
Terms: eloquent

Additional details

No response

ryanmitchell commented 2 weeks ago

When save() is called on a Statamic\Eloquent\Asset\Asset then clearCaches() on the eloquent asset is called. Are you saying youre seeing something different?

andreas-eisenmann commented 2 weeks ago

Okay save() is being called on an instance of Statamic\Eloquent\Asset\Asset but the actual definition of save() is in Statamic\Assets\Asset so $this refers to \Statamic\Assets\Asset::clearCaches() and not to \Statamic\Eloquent\Assets\Asset::clearCaches() because it is declared as private and cannot be overwritten in any subclass.

If save() would be overwritten/extended in Statamic\Eloquent\Asset\Asset then Statamic\Eloquent\Asset\Asset::clearCaches() would be called. But there's no save() which extends the parent method: https://github.com/statamic/eloquent-driver/blob/v4.4.0/src/Assets/Asset.php#L16

Simplified example:

<?php

class AssetOfStatamicCore {

    public function save(): void
    {
        $this->clearCaches();
    }

    private function clearCaches(): void
    {
        echo "A";
    }

}

class AssetOfEloquentDriver extends AssetOfStatamicCore {

    private function clearCaches(): void
    {
        echo "B";
    }

}

$b = new AssetOfEloquentDriver();
$b->save(); // will output "A"
ryanmitchell commented 2 weeks ago

Yeah I get you - we had a save method at one point so it's been refactored out along the way and this was missed. Leave it with me.

andreas-eisenmann commented 2 weeks ago

Thanks!

jasonvarga commented 2 weeks ago

Does that mean https://github.com/statamic/eloquent-driver/pull/271 did nothing then?

ryanmitchell commented 2 weeks ago

@jasonvarga yeah it seems that logic wasn’t being run.

ryanmitchell commented 2 weeks ago

I've added a failing test here - https://github.com/statamic/eloquent-driver/pull/308 - to show that the current behaviour isnt correct.

If you apply the changes in https://github.com/statamic/cms/pull/10342 then the test passes.

ryanmitchell commented 5 days ago

Fixed in 4.7.0