Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FilePreview: File URL links to the media folder directly #3201

Closed
lukasbestle opened this issue Mar 11, 2021 · 9 comments
Closed

FilePreview: File URL links to the media folder directly #3201

lukasbestle opened this issue Mar 11, 2021 · 9 comments
Assignees
Labels
type: enhancement ✨ Suggests an enhancement; improves Kirby
Milestone

Comments

@lukasbestle
Copy link
Member

The Url link in the FilePreview component currently displays the "public" URL (/path/to/page/file.jpg), but links to the media folder (because that's what the default file::url component returns). If users now copy that link, the link will contain the media hash and will therefore not be permanent.

I think the link target should always be the public URL of the file. But of course this could break setups with a custom file::url component. What do you think?

@lukasbestle lukasbestle added needs: discussion 🗣 Requires further discussion to proceed type: enhancement ✨ Suggests an enhancement; improves Kirby labels Mar 11, 2021
@distantnative
Copy link
Member

Is this another case where we shouldn't copy the image over to media anyways without modifications? And just always keep files without modifications in their original location?

@lukasbestle
Copy link
Member Author

lukasbestle commented Jul 18, 2021

No, this is not about where the image is actually stored. If the link target is the public file URL, Kirby will dynamically redirect to the current media URL. So it's just about the link that's displayed in FilePreview.

For content files we do need to store a copy in media as files within content/ are blocked in the default .htaccess for security.

@distantnative
Copy link
Member

I'm trying to make up my mind... but somehow both feels right and wrong.
Maybe the others have better judgement :D

@lukasbestle
Copy link
Member Author

I looked into the code again: Even if the file::url component is overridden, it would still work after the change: Accessing a file with its public URL (/path/to/page/file.jpg) will always trigger a redirect to the mediaUrl() of the file (handled by $app->io()). So linking to the public URL should always work with the advantage of creating a permanent URL that can be copied.

@lukasbestle
Copy link
Member Author

Wait, it's the other way around probably. Our default file::url component uses the mediaUrl, but a custom one could link to wherever and this wouldn't be reflected.

But I think we should solve this by fixing the redirect in $app->io() to use file::url internally, not mediaUrl.

@distantnative
Copy link
Member

Eventually, we will have to tackle the file::url and file::verison muddle

@lukasbestle
Copy link
Member Author

Yeah, to be honest I don't fully grasp how it all interconnects at the moment.

@bastianallgeier
Copy link
Member

While the file::url file::version horror definitely needs to be fixed, this one is really easy to solve in the Panel. We should indeed use the shortcut URL to avoid such copy issues.

distantnative pushed a commit that referenced this issue Jul 23, 2021
@distantnative distantnative self-assigned this Jul 23, 2021
@distantnative distantnative removed the needs: discussion 🗣 Requires further discussion to proceed label Jul 23, 2021
@distantnative distantnative added this to the 3.6.0-alpha.3 milestone Jul 23, 2021
bastianallgeier pushed a commit that referenced this issue Jul 24, 2021
@bastianallgeier
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement ✨ Suggests an enhancement; improves Kirby
Projects
None yet
Development

No branches or pull requests

3 participants