ash-jc-allen / favicon-fetcher

A Laravel package for fetching favicons from websites.
MIT License
173 stars 13 forks source link

HttpDriver findLinkElement method #10

Closed marispro closed 2 years ago

marispro commented 2 years ago

Hello,

currently findLinkElement() function pattern is $pattern = '/<link.*rel="(icon|shortcut icon)"[^>]*>/i';

however this does not work in all cases, because it might assume that this is favicon link:

when we remove dot and asterisk (.), then it works much better. However, I can not test all possible cases. `$pattern = '/<link rel="(icon|shortcut icon)"[^>]>/i';`

Any ideas why this pattern is how it is?

ash-jc-allen commented 2 years ago

Hey @marispro!

I fully expected there to be some small teething issues with the regex pattern. I think you might have missed off the example in your comment? If you can add the example, I can take a look at updating the pattern 😄

And the .* was added to handle if the <link> element looked something like this:

<link href="/icon/is/here.ico" rel="shortcut icon" />

If we removed it, it would only detect <link> elements that stricter start with: <link rel=. If anything on this can be improved though, I'd be happy to take a look 😄

marispro commented 2 years ago

Hi @ash-jc-allen

As example try to fetch favicon from this site: https://www.morex.lv

At the moment it takes this element out as linkTag, what is not favicon

<link rel="alternate" href="https://www.morex.lv/" hreflang="lv"

Thank you for answer!

ash-jc-allen commented 2 years ago

That's a great spot, thanks! 😄

I've put together a potential bug fix for this now (https://github.com/ash-jc-allen/favicon-fetcher/pull/13) so I should hopefully be able to merge that in today and solve this issue. If you get chance to test out the fix, I'd appreciate any feedback on if it's working now :)

marispro commented 2 years ago

@ash-jc-allen this works, thanks!

ash-jc-allen commented 2 years ago

Hey @marispro! I'm just dropping a quick comment here to let you know that this bug fix has been released in v1.1.1 🙂

https://github.com/ash-jc-allen/favicon-fetcher/releases/tag/v1.1.1