search

ash-jc-allen / favicon-fetcher

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

Issue on parsing from element #48

Closed pio-sko closed 1 year ago

pio-sko commented 1 year ago

Hi, I have tried this package for favicons but on some website have got error, i.e if I try to get icon for that site https://www.smartsupp.com/ image

ash-jc-allen commented 1 year ago

Hey @pio-sko! Sorry that you're running into issues with the package, and thanks for flagging this for me 🙂

I've just had a quick look, and it appears look like the error is happening because the HTML tags for the icons aren't using ' or " for any of the attributes. So it's throwing my link detection code off. For example, this is one of the icons:

<link rel=apple-touch-icon sizes=180x180 href=/netlify/favicons/apple-touch-icon.png>

In all honesty, I didn't know it was possible to write HTML like this without using the ' or ", so my code doesn't take that into account haha!

I'm super busy with client projects at the moment, so it's going to be a bit difficult for me to get a fix together for this quickly. But if it helps though, I've tested it out with a couple of the other drivers that the package ships with and they seem to work. Would it be possible for you to switch over to one of the other drivers for the time being? 🙂

pio-sko commented 1 year ago

Thanks for reply, will use other driver for the time being. 🙂

Harrk commented 1 year ago

Depending on whether or not you want an additional package. I chucked in symfony/dom-crawler to consume the body HTML and spit it right back out again. Then the code beyond that can safely consume a standardised format without requiring a rewrite + beats fighting regex.

image
MatthewPageUK commented 1 year ago

Should work..

/href=(["']*)(.*?)(["'> ])/g

ash-jc-allen commented 1 year ago

Thanks for your suggestions @Harrk and @MatthewPageUK!

I've come up with a PR that should solve this issue: https://github.com/ash-jc-allen/favicon-fetcher/pull/56

I'm aiming to get it merged and released in the next day or so as long as everything's okay with it 🙂

ash-jc-allen commented 1 year ago

I've just released Favicon Fetcher v3.0 (https://github.com/ash-jc-allen/favicon-fetcher/releases/tag/v3.0.0). This release should solve the issue 😄

Feel free to give me a shout if you're still having issues with trying to get the favicons!