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.

Sign up for GitHub

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

Jump to bottom

Does not work for redirects that add URL parameters #29

Open
filliph opened this issue Jan 7, 2025 · 1 comment · May be fixed by #30
Open

Does not work for redirects that add URL parameters #29

filliph opened this issue Jan 7, 2025 · 1 comment · May be fixed by #30

Comments

@filliph
Copy link

filliph commented Jan 7, 2025

Description

If you pass a URL whose redirect applies URL parameters, favicons will not fetch because of an incorrect application of self::baseUrl() in public function get().

Proof of concept

require_once('vendor/autoload.php');

$favicon = new \Favicon\Favicon();

var_dump($favicon->get('http://www.google.com'));

Expected output: string(34) "https://www.google.com/favicon.ico"
Observed output: bool(false)

Triage

Line 162 is the problem: https://github.com/ArthurHoaro/favicon/blob/master/src/Favicon/Favicon.php#L162 because the output of $this->endRedirect(self::baseUrl($this->url, false)) for the given URL http://www.google.com is string(34) "https://www.google.com/?gws_rd=ssl"

This, in turn, means that rtrim() fails, because of the extra URL parameter.

If we change L162 to $url = rtrim(self::baseUrl($this->endRedirect(self::baseUrl($this->url, false)), false), '/');, the favicon is fetched correctly.
filliph added a commit to filliph/favicon that referenced this issue Jan 7, 2025
@filliph
Fix redirects that apply URL parameters
0e62a12 
Closes ArthurHoaro#29
@filliph filliph linked a pull request Jan 7, 2025 that will close this issue
Open
@filliph
Copy link
Author

filliph commented Jan 7, 2025 •
edited
Loading

The only thing I'm not sure about is whether the added self::baseUrl() call should include the path or not 🤔 In my local test, it doesn't appear to make a difference whether I use

$url = rtrim(self::baseUrl($this->endRedirect(self::baseUrl($this->url, false)), false), '/');

or

$url = rtrim(self::baseUrl($this->endRedirect(self::baseUrl($this->url, false)), true), '/');

when testing for Google specifically as mentioned in my OP.

I can amend the PR if you think it should include the path at the end of the redirect loop 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix redirects that apply URL parameters filliph/favicon
1 participant
@filliph