Skip to content

Remote Icons #107

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.

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

Open
wants to merge 29 commits into
base: main
Choose a base branch
from
Open

Remote Icons #107

wants to merge 29 commits into from

Conversation

expxx
Copy link
Contributor

@expxx expxx commented Apr 3, 2025

Allow for extension developers to use HTTP(s) links for their icons instead of directly including them.

Upon installing an extension, the previous action was just if it doesn't exist on the filesystem, reject it altogether. This changes it, so if it starts with http, it can pass.

If a link passes through that barrier, the installer will check if it ends with one of the supported filetypes (those being svg, png, gif, jpeg, webp, and jpg. If it doesn't, it uses one of the default icons, otherwise it uses that remote url AS the icon.

@prplwtf prplwtf self-requested a review April 3, 2025 18:41
@prplwtf
Copy link
Member

prplwtf commented Apr 3, 2025

That's a lot of commits! We'll have to do some triage on this one, as I don't significantly like extensions relying on remote sources. My main concern is admin's IPs (and other request info) being exposed to external servers.

@expxx
Copy link
Contributor Author

expxx commented Apr 3, 2025

Hmm. Yeah, that's fair. I know services like PayPal have a "secureimages" type thing that's just a proxy, I wonder if we could do something similar here

@prplwtf
Copy link
Member

prplwtf commented Apr 3, 2025

Upon installing an extension, the previous action was just if it doesn't exist on the filesystem, reject it altogether. This changes it, so if it starts with http, it can pass.

As far as I know, Blueprint will throw a FATAL error when the icon cannot be found. This should only happen when an icon is defined in the first place. If there is no icon defined, Blueprint uses one of it's default icons.

@prplwtf
Copy link
Member

prplwtf commented Apr 3, 2025

Your implementation looks really promising though! I'm going over some of your commits right now and it seems like you accounted for pretty much every edge-case I can think of.

@expxx
Copy link
Contributor Author

expxx commented Apr 3, 2025

Another idea, one of my previous ideas was download the icon from the remote url when you install the addon instead of just embedding it, that could also work

@prplwtf
Copy link
Member

prplwtf commented Apr 3, 2025

Another idea, one of my previous ideas was download the icon from the remote url when you install the addon, that could also work

That's probably a better approach to this, since that'd make Blueprint just proxy the image, which is good. You could theoretically also make an image proxy in PHP, although I'm not sure how slow the images load in that case.

@expxx
Copy link
Contributor Author

expxx commented Apr 3, 2025

Another idea, one of my previous ideas was download the icon from the remote url when you install the addon, that could also work

That's probably a better approach to this, since that'd make Blueprint just proxy the image, which is good. You could theoretically also make an image proxy in PHP, although I'm not sure how slow the images load in that case.

Lemme do some experimentation shenanigans real quick

@expxx
Copy link
Contributor Author

expxx commented Apr 3, 2025

@prplwtf, this should be a reasonable take at it. Image speed seemed to be fine, and even then it's only fetched once every 24h

@expxx
Copy link
Contributor Author

expxx commented Apr 3, 2025

I did unfortunately have to make another method for $blueprint though, I didn't see one to fetch the config for a specific extension, just all of them. So now we have that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants