kirby icon indicating copy to clipboard operation
kirby copied to clipboard

Asset::mediaUrl() is broken

Open rzschoch opened this issue 5 years ago • 8 comments

Kirby Team Summary (22.12.2021)

Assets are correctly modified and moved to the media folder when you use one of the modification methods (resize or crop). This has been fixed after the issue has been posted. The original issue is therefore mostly resolved.

But the original is not moved to the media folder though. This is not a problem when you use $asset->url(). It will always point to the original URL. But $asset->mediaUrl() is broken. It tries to load the asset from the media folder instead of the original location. But our image API does not move originals into the media folder.

Proposed solution

  • [ ] Fix Asset::mediaUrl() Review @distantnative's PR as a possible fix again: https://github.com/getkirby/kirby/pull/3540

Original Issue by @rzschoch

Describe the bug
The urls for assets generated via the asset helper are broken and not copied into the media folder. The issue also happens with file versions of an asset.

To Reproduce

<img src="<?= asset('assets/speakers/speaker.jpg')->mediaUrl() ?>">
// image source url: http://localhost:8080/media/assets/assets/speakers/555072720-1582211687/speaker.jpg
// 404 response

or

<img src="<?= asset('assets/speakers/speaker.jpg')->resize(600)->url() ?>">
// image source url: http://localhost:8080/media/assets/assets/speakers/555072720-1582211687/speaker-600x.jpg
// 404 response

Expected behavior
I would expect that the asset is copied to the media folder as soon as it is requested for the first time via its media url.

Kirby Version
3.3.4

Additional context
This seems to be related to: https://github.com/getkirby/kirby/issues/2314 and https://github.com/getkirby/kirby/issues/1728

rzschoch avatar Feb 22 '20 18:02 rzschoch

@bastianallgeier I keep wondering whether we actually can support file modification methods on the asset class. It seems to be implemented, but I don't think it's actually functional. Might be the more solid way forward to remove support from the asset class altogether.

distantnative avatar Jul 17 '21 17:07 distantnative

But isn‘t FileModifications the reason why we have Asset in the first place? So you can create thumbs etc. from files outside of the content dir.

lukasbestle avatar Jul 18 '21 08:07 lukasbestle

Indeed, but looking at (likely duplicates)

  • https://github.com/getkirby/kirby/issues/2314
  • https://github.com/getkirby/kirby/issues/1728

I am not sure it ever really worked properly - and so far we haven't found a fix in a long time this being reported. Maybe the new Filesystem setup helps. better to understand and find the issue. But my point is, if we cannot actually make it work, maybe better to retire the broken feature.

distantnative avatar Jul 18 '21 08:07 distantnative

I have tried a little, also based on the descriptions in the other issues, and I cannot reproduce all the reported issues with Kirby 3.5.7.1 or 3.6.0-alpha.1. What I have found:

asset('assets/speaker.jpg')->mediaUrl()
// http://sandbox.test/media/assets/assets/1533364967-1626596498/speaker.jpg
// does not work

asset('assets/speaker.jpg')->resize(250)->url()
// http://sandbox.test/media/assets/assets/1533364967-1626596498/speaker-250x.jpg
// does work

asset('assets/speaker.jpg')->bw()->url()
// http://sandbox.test/media/assets/assets/1533364967-1626596498/speaker-bw.jpg
// does work

asset('assets/speaker.jpg')->bw()->mediaUrl()
// -
// does not work

So to me it seems the main problem is tied to ->mediaUrl() and assets.

I was for a moment confused by the double assets/assets segment - also raised in one of the other issues. But here I think this works as intended: the media path for all assets created via assets() is media/assets, what follows is then the path of the individual asset that's passed, so in this case assets again.

distantnative avatar Jul 18 '21 08:07 distantnative

Yep, I think the behavior in general looks good, only mediaUrl() is broken.

lukasbestle avatar Jul 18 '21 09:07 lukasbestle

I think I have found fixes (see PR).

distantnative avatar Jul 18 '21 09:07 distantnative

@rzschoch Your second example is working for me in current versions. We have been wondering though, why you are using ->mediaUrl() and not just ->url() in the first example. Any particular reason?

distantnative avatar Jul 18 '21 14:07 distantnative

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs.

github-actions[bot] avatar Sep 18 '22 00:09 github-actions[bot]