Asset::mediaUrl() is still broken

jensscherbl commented 9 months ago

Description

Just stumbled upon that same issue and it still doesn’t seem to be resolved in 4.1.

What I’m trying to do

I have stylesheets under /assets/styles in my setup and want to add a hash to the URL for cache busting. I know that there are two plugins available for exactly this and I also have my own workaround, but having broken features like this in the codebase leaves me with a somewhat uneasy feeling.

Expected behavior

My understanding of the Asset class from its name and description is that I can use it for all my static assets, like fonts, images, stylesheets, etc. So in my controller or template, I call asset('assets/styles/all.min.css')->mediaUrl() and (based on the name of the method) expect to get a URL (including the mediaHash) to a copy of that file in the media folder.

However, there are several issues at the moment:

The file is not automatically copied to the resulting path in the media folder.
The resulting path will be /media/assets/assets/styles/all.min.css, since it’s automatically prefixed with assets/.
If I just call assets('styles/all.min.css')->mediaUrl(), it can’t find the file to calculate the mediaHash because the path will only be prefixed with assets/ after calculating the mediaHash.
If I move my assets to the media folder from the start (styles would be located under /media/assets/styles in this scenario) and call assets('styles/all.min.css')->mediaUrl() or assets('assets/styles/all.min.css')->mediaUrl(), it can’t find the file to calculate the mediaHash.
If I call assets('media/assets/styles/all.min.css')->mediaUrl() the file can be found and the mediaHash can be calculated, but the resulting path is wrong since it will be prefixed with media/assets/media/assets/styles/....
The configured assets-root doesn’t seem to be used here at all, which adds to the confusion.

Proposed solution

Not sure if this has any unwanted side effects, but my naive take on how this should work would be:

Use the configured assets-root to prefix the given path in the constructor of the Asset class.
Strip the assets-root from the path when building the mediaPath. Keep prefixing the mediaPath with assets/.
Copy the file to the correct location in the media-folder when Asset::mediaUrl() is called.

To reproduce

See description.

Your setup

Kirby Version
4.1

PHP Version
8.3

distantnative commented 3 months ago

The hard thing here is changing the behavior while keeping the breaking change small. As it is used in various places where people currently are including the assets/ part (e.g. custom Panel assets).

Cleanest way would be to use the assets root and only path the relative part to that. But this is also the one with the larger breaking change.

jensscherbl commented 3 months ago

Hey, thanks for looking into this (even though it’s only a minor issue).

Maybe it can be multiple separate fixes?

I mean, not using the configured assets-root is actually the smallest issue, so this could be another change for the longer-term.

Returning an actually working media URL, on the other hand, could be done without breaking changes, I guess?

getkirby / kirby