Skip to content

Enable URLs with querystring#117

Open
rochoa wants to merge 4 commits intotilemill-project:masterfrom
CartoDB:urls-with-querystring
Open

Enable URLs with querystring#117
rochoa wants to merge 4 commits intotilemill-project:masterfrom
CartoDB:urls-with-querystring

Conversation

@rochoa
Copy link
Copy Markdown

@rochoa rochoa commented Nov 3, 2014

No description provided.

@springmeyer
Copy link
Copy Markdown
Member

Great, thanks. Would appreciate a quick summary of what this fixes specifically.

@rochoa
Copy link
Copy Markdown
Author

rochoa commented Nov 3, 2014

Sorry, I should have been more clear about what the issue was or create an issue before.

Basically this allows to use URLs like: http://i.imgur.com/yvRISk8.png?foo=bar.

@rochoa
Copy link
Copy Markdown
Author

rochoa commented Nov 3, 2014

Check:

> var OLD_REGEX = /[-a-zA-Z0-9@:%_\+.~#?&//=]{2,256}\.[a-z]{2,4}\b(\/[-a-zA-Z0-9@:%_\+.~#?&//=]*)?/gi
undefined
> var NEW_REGEX = /[-a-zA-Z0-9@:%_\+.~#?&//=]{2,256}\.[a-z]{2,4}\b(\/[-a-zA-Z0-9@:%_\+.~#?&//=]*)/gi
undefined
> 'http://i.imgur.com/yvRISk8.png?foo=bar'.match(OLD_REGEX)
[ 'http://i.imgur.com/yvRISk8.png' ]
> 'http://i.imgur.com/yvRISk8.png?foo=bar'.match(NEW_REGEX)
[ 'http://i.imgur.com/yvRISk8.png?foo=bar' ]

I don't know if it could be better to move the extract urls from carto to it's own method so we can unit test it but at the same time I don't like to expose that method in the module/public api. Maybe move it to utils.js?

@jchamberlain
Copy link
Copy Markdown

I know this is old, but I'd love to see this or something equivalent go in. In our app, all user-uploaded files are stored in an S3 bucket that's not publicly accessible. Rather than making an exception for images intended for maps, we inject presigned URLs into the CartoCSS for marker files, etc. Problem is, presigned URLs have query strings which are currently being stripped.

@rochoa
Copy link
Copy Markdown
Author

rochoa commented Dec 9, 2016

I just updated the branch to solve conflicts on CHANGELOG file.

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.

3 participants