sylvainjule / kirby-colorextractor

Extract dominant / average color from any image. Kirby 3 and 4.
39 stars 7 forks source link

color extraction fails with absolute urls #3

Closed joernroeder closed 5 years ago

joernroeder commented 6 years ago

My image urls start with /media/… and the file is therefore not found. https://github.com/sylvainjule/kirby-colorextractor/blob/master/lib/functions.php#L9 could be prefixed with the media folder path from kirby->roots() but it's just a shot in the dark…

sylvainjule commented 6 years ago

Do you have a specific setup? Does it happens with the upload hook or the button ?

I still probably lack understanding of k3's images behavior at this point, I don't understand how images would be prefixed with media in an installation but not another.

joernroeder commented 6 years ago

@sylvainjule happens with the button in the panel. my folder structure is similar to the one in devkit https://github.com/k-next/devkit to keep everything except the entrypoint outside the domain root. the media folder will be created in public by kirby. there is also the https://nnnnext.getkirby.com/docs/cheatsheet/roots/media function which should return the path

sylvainjule commented 6 years ago

Thanks, I probably won't be able to take a look tomorrow but I will asap!

joernroeder commented 6 years ago

no problem, thanks!

joernroeder commented 6 years ago

@sylvainjule did you find some time to look into this? i would love to use it for a site which launches on monday. it would be a nice addition, nothing road blocking. no prob if you don't find some time, i'll stick with my current plan then… thx!

sylvainjule commented 6 years ago

I'm having a look now but I can't reproduce this with a fresh devkit... colors are extracted as they should and no errors thrown.

Perhaps you could zip me a truncated version of your setup, changing the content if confidential, etc. so that I can debug on the correct folder structure?

sylvainjule commented 6 years ago

Also could you screenshot me the response / error thrown ?

joernroeder commented 6 years ago

@sylvainjule responses from http://localhost:8000/plugins/colorextractor/process-image are coming back with the following json:

{
    "status":"error",
    "error":"file_get_contents(\/media\/pages\/pageid\/myimage-300x200-q90.jpg): failed to open stream: No such file or directory"
}

for various reasons (reverse proxy, docker, ci) i had to set 'url' => '/' in my config and i think therefore file_get_contents() looks in server root. apart from not finding anything, this is also a potential security issue.

sylvainjule commented 6 years ago

Ok thanks, with this config tweak I get the same error.

But since kirby has been told to rewrite its urls we can't rely on $kirby->url('media') either as it returns the same prefix. Looking for a workaround.

sylvainjule commented 6 years ago

The last commit fixes it for me.

(be careful though, I still need to set a cache for the plugin, I'm relying a lot on $site->index() for now, if your website is content-heavy it might not like it)

joernroeder commented 6 years ago

just pretty quick:

But since kirby has been told to rewrite its urls we can't rely on $kirby->url('media') either as it returns the same prefix.

Url rewriting doesnt matter here imo as its about the absolute file system/disk path via $kirby->roots() https://nnnnext.getkirby.com/docs/framework/cms/app/roots

sylvainjule commented 6 years ago

My bad, within the new docs, what should have been $kirby->roots()->media() is shown as $kirby->url('media'), which returns the url of the folder, not its disk path.

Actually the better way would be to call $thumb->root() or $thumb->mediaRoot(), but I can't go with it right now as there is an issue with the $thumb object, this is why I went for url. Probably best to wait for its fix than getting lost in quickfixes that will need to be changed in the end.

joernroeder commented 6 years ago

Yes, no need to rush here. Let’s wait. Thanks and enjoy the weekend!

sylvainjule commented 5 years ago

Should be fixed, switched to root(). ✅ Don't hesitate to reopen if not.