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.

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

Add a way for them to inject their camo images #96

Closed
wants to merge 2 commits into from

Conversation

notriddle
Copy link
Member

Fixes #95

@kornelski
Copy link
Contributor

Well, you were first! But this solves proxying of images with absolute URLs, but not images with relative URLs. So at minimum it'd be good to have it as a preprocessing step, so it can see relative image URLs.

@notriddle
Copy link
Member Author

notriddle commented May 4, 2018

Well, you were first! But this solves proxying of images with absolute URLs, but not images with relative URLs. So at minimum it'd be good to have it as a preprocessing step, so it can see relative image URLs.

If your URL base is http://example.com/ and you're given a document like <img src="/image.png"> <img src="http://example.com/image.png">, wouldn't you want to give both images the same camo URL?

I'm basically imagining URL processing as a pipeline, where the first step converts relative URLs to absolute ones, and the second step (which, since it happens after URL resolution, can be simplified to only deal with absolute URLs) converts direct image URLs to camo image URLs.

If it doesn't work for your needs, though, we can probably just merge both 😄

@kornelski
Copy link
Contributor

kornelski commented May 4, 2018

I'm rendering readmes from GitHub, so my URL base is https://github.com/user/repo/ and links need to use this as a base, but I want to rewrite image.png to something like https://github.com/user/repo/blob/raw/image.png (because the usual base URL is for an HTML page about the image). And if an image is https://thirdparty.example.com/image.png, I want to rewrite it to https://my-proxy/https://thirdparty.example.com/image.png.

So if the base URL is applied first, I'll have to strip it later when deciding how to handle images.

In your PR I like the idea of it being a generic attribute tranformer. There's no need to limit it to URLs.

@notriddle
Copy link
Member Author

Good point. Let's merge both, then.

@kornelski
Copy link
Contributor

I've renamed url_filter_map to attribute_filter to generalize it :)

@notriddle notriddle closed this May 4, 2018
@notriddle notriddle deleted the attr_post_process branch May 4, 2018 02:19
bors bot added a commit that referenced this pull request May 4, 2018
97: Context-sensitive relative URL rewrite r=notriddle a=kornelski

Similar to #96, but with a critical difference that this is a preprocessing step, before relative URLs are resolved. This allows different relative URLs for images than links.

I've named it `url_filter_map`, after a similar iterator filter in std.

I've opted for storing callback directly rather than via trait, but this required a custom `Debug`, since `derive` couldn't skip the callback.


Co-authored-by: Kornel <kornel@geekhood.net>
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