Refactor image fetching #1190
Merged
Refactor image fetching #1190
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jtojnar
force-pushed
the
img-refactor
branch
3 times, most recently
from
2ebc3cf
to
e187305
Compare
jtojnar
force-pushed
the
img-refactor
branch
5 times, most recently
from
213e5e2
to
91508f8
Compare
jtojnar
commented
Sep 13, 2020
| if (0.8 < $aspectRatio && $aspectRatio < 1.3) { | ||
| $this->logger->debug('icon: using feed logo: ' . $faviconUrl); | ||
|
|
||
| return $faviconUrl; |
There was a problem hiding this comment.
Eventually, we will want to return the blob here as well so that it does not have to be downloaded again but we have already decreased the number of downloads a lot and I do not want to delay this any longer.
Thanks Psalm for noticing. Weird that this has ever worked. It was broken for five years: #639
No need to pass the item through.
No weird pass-by-reference any more.
To allow distinguishing between an icon coming from the feed item and one from the feed itself, we added a new optional getSourceIcon() method. When the getIcon() method returns null for an item or the item icon cannot be fetched, the source icon will be used as a fallback. Both getSourceIcon() and getIcon() can return null now, if an icon is not available.
Only when both width and height are supplied and best fit.
If the image has no dimensions, it can hardly be resized.
We need the blob for content type detection so rather than downloading it twice, let's pass it to loadImage directly.
When SVG element was embedded in a HTML document, it was detected as SVG image instead of a HTML page, causing issues for favicon fetching. Fixes: #1191
Some sites like golem.de use rectangular logos in feeds, which are unsuitable for feed icon. Let's fall back to favicon instead. Fixes: #1183
Reddit for some reason returns a JSON string "{}"
when trying to fetch `message/inbox`, which causes the ItemsIterator to freak out
and loop without an end.
jtojnar
force-pushed
the
img-refactor
branch
from
30e3d8c
to
2eac971
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Avoid repeating requests and clean up the old spaghetti.
Customization changes
To allow distinguishing between an icon coming from the feed item and one from the feed itself, we added a new optional
getSourceIcon()method. When thegetIcon()method returnsnullfor an item or the item icon cannot be fetched, the source icon will be used as a fallback.Both
getSourceIcon()andgetIcon()can returnnullnow when an icon is not available.