added document root for local url in getimagesize #33
Conversation
discovered a problem with local URLs (relative to webroot) when adding marker icons, where the getimagesize was failing on a path like '/img/myicon.png'. some digging on the web revealed that getimagesize wants the server's document root prepended to the url for the size operation to work, so that is what i added. this made the complaint go away about finding the site-relative image, and the image shows as expected.
|
isnt there a env () cakephp wrapper we could use instead. or fullBaseUrl? |
using the built-in WWW_ROOT constant now instead of using $_SERVER.
|
Yes, I think the WWW_ROOT constant should be equivalent, and it seems to be working just as well as the prior version. Does this seem more in line with standard cakephp 3 usage? |
|
for correct setups yes. if you are using a non root subpath involved not.. but fair enough. |
| @@ -847,7 +847,12 @@ public function icon($url, array $options = []) { | |||
| // The shadow image is larger in the horizontal dimension | |||
| // while the position and offset are the same as for the main image. | |||
| if (empty($options['size'])) { | |||
| $data = getimagesize($url); | |||
| if (substr($url, 0, 1) === '/') { | |||
There was a problem hiding this comment.
This will probably not work for URLS like ``//...` ( https://www.paulirish.com/2010/the-protocol-relative-url/ ).
There was a problem hiding this comment.
(oops, putting this back on the thread)
hmmm. it seems like the protocol relative url is intended to avoid using http if the site was accessed as https, without having to specify the scheme again for the asset in the url. but regardless of whether the site is accessed by http or https, wouldn't the file under the webroot still be available on the file system? the addition of webroot was because the asset doesn't have a http or https in front (which caused getimagesize to fail), so wouldn't the getimagesize still work okay on the local file for the double slash case too?
There was a problem hiding this comment.
You can try, but the above code then woudnt be good.
There was a problem hiding this comment.
Hey, sorry I lost track of this issue for a bit.
I don't see a problem with the implementation yet. If there's a double slash in front of the $url, then the resulting file system path (with WWW_ROOT in front) will also have a double slash in it. But that should be just fine in every file system I know of, Unix or Windows. All the slashes should, in effect, collapse to a single one for local file system access. Is there another issue I'm not seeing?
There was a problem hiding this comment.
It sounds a bit unclean, that is the only concern then I guess.
There was a problem hiding this comment.
I hear that concern, and I have modified the code to call "realpath" on the file system path before passing it to the getimagesize function.
Hopefully this gets it looking reasonable. I have verified that the most recent check-in works properly on my site.
Are there any other issues I should address?
Thanks. -- Chris
There was a problem hiding this comment.
can realpath return false or sth? better throw Runtime exception then :)
There was a problem hiding this comment.
yep, that could happen. i've updated the code again with error checking for that case and a fallback to the simple url.
if realpath fails, we don't want to concatenate a false and a string, so we're checking the result now. if it fails, we fall back to the original url, since we're out of options trying to make the string into a file system path.
|
i believe this pull request branch resolved all issues as of commit: 7f9e58b |
|
I just reproduced it, and for me all worked just fine: And the icons work as expected. What exactly is the problem you are facing, you mentioned "some" error, but you didnt go into specifics. |
|
After looking at your code more closely, I saw that you are not using the Url builder to generate absolute URLs, that was your issue. But I did fix it for you none the less: 1bb59d9 |
|
yes, that version is looking good. thanks. |
|
Sure :) |
discovered a problem with local URLs (relative to webroot) when adding marker icons, where the getimagesize in GoogleMapHelper was failing on a path like '/img/myicon.png'. some digging on the web revealed that getimagesize wants the server's document root prepended to the url for the size operation to work, so that is what i added. this made the complaint go away about finding the site-relative image, and the image shows as expected.