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

AssetRepository::findByUrl() doesn't work when file is in root of container #244

Closed andreas-eisenmann closed 4 months ago

andreas-eisenmann commented 4 months ago

Call \Statamic\Eloquent\Assets\AssetRepository::findByUrl(string $url) e.g. with http://localhost/storage/foo.jpgfor $url, assuming there's a container assets connected to a disk having a config like that in config/filesystems.php:

  'disks' => [
        'assets' => [
            'driver' => 'local',
            'url' => '/storage',
            // ...
        ],
  ],

And of course our site url is configured as http://localhost.

Now when findById() is being called in in line 58

https://github.com/statamic/eloquent-driver/blob/090ffac3047824c77ac254bb058c5bce7c7726e6/src/Assets/AssetRepository.php#L58

we have following state:

And "{$container}::{$path}" will evaluate to assets::/foo.jpg.

In \Statamic\Eloquent\Assets\AssetRepository::findById(string $id) in line 19, $folder is an empty string (here we go šŸ›):

https://github.com/statamic/eloquent-driver/blob/090ffac3047824c77ac254bb058c5bce7c7726e6/src/Assets/AssetRepository.php#L19

This query will lead to an empty result because in assets_meta all assets in the root of a container are being stored with / as value for the folder column:

https://github.com/statamic/eloquent-driver/blob/090ffac3047824c77ac254bb058c5bce7c7726e6/src/Assets/AssetRepository.php#L23-L27

Side note: my full roundtrip is the glide:generate tag where I want to render the asset by its id assets::foo.jpg; unfortunately there's a huge roundtrip where Glide resolves the asset id to an Asset object in \Statamic\Tags\Glide::generateImage(), this object is being casted to its url and now here we load the asset again. The point is that I have no chance for a workaround e.g. by directly calling \Statamic\Eloquent\Assets\AssetRepository::findById(string $id).

Let's peek into the code of statamic core:

https://github.com/statamic/cms/blob/d76a4b4d0743e809b0f63485e9d40be980787f4c/src/Assets/Asset.php#L368

I think you should also apply ltrim($path, '/') on several places, e.g. here:

https://github.com/statamic/eloquent-driver/blob/090ffac3047824c77ac254bb058c5bce7c7726e6/src/Assets/AssetRepository.php#L56

=> should definitely be $path = \ltrim(str_after($url, $containerUrl), '/');

and maybe, to be on the safe side, also here:

https://github.com/statamic/eloquent-driver/blob/090ffac3047824c77ac254bb058c5bce7c7726e6/src/Assets/AssetRepository.php#L16

=> you could add another line $path = \ltrim($path, '/') right after explode()

ryanmitchell commented 4 months ago

Thanks. I would suggest this as the fix: https://github.com/statamic/eloquent-driver/pull/245

andreas-eisenmann commented 4 months ago

Thanks for the šŸš€ fast reply! šŸ˜ŽšŸ¤˜

Why not ltrim($path, '/') like e.g. in the linked statamic core code? One line vs. three lines with starts_with() and substr()... šŸ™‚

ryanmitchell commented 4 months ago

Just personal preference of readability.

ryanmitchell commented 4 months ago

Closed as fixed in 3.1.2