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

(feature) add sqlpage.query_string() #486

Closed
wants to merge 1 commit into from
Closed

Conversation

guspower
Copy link

@guspower guspower commented Jul 6, 2024

Adds the sqlpage.query_string() SQLPage function.

There are a few auth-related scenarios where it is useful to be able to directly access the query string and propagate it, for example when redirecting to a login page from a url with query string, and then redirecting the user back to that url on successful login.

There may be better ways to do this, but here is a simple query_string function for your consideration.

@lovasoa
Copy link
Collaborator

lovasoa commented Jul 8, 2024

Thank you for the pull request !

I am not a huge fan of duplicating the entire query string (which can be large) into memory just for the use of one new function...

What would you think about taking the variables as an argument, in the form of a json object ? That would make the function more generally useful, and one would still be able to do

sqlpage.query_string(sqlpage.variables('get'))

@guspower
Copy link
Author

guspower commented Jul 8, 2024

Yep that seems reasonable - in fact it was what I started out doing but then realized that I'd have to take a perspective on both ordering and encoding. Let me rework this later in the week and come back to you with something that fits.

@lovasoa
Copy link
Collaborator

lovasoa commented Jul 11, 2024

@guspower , what do you think about doing this instead: #494 ?

@guspower
Copy link
Author

Yep that looks good to me. Much cleaner +1

lovasoa added a commit that referenced this pull request Jul 11, 2024
@lovasoa lovasoa closed this Jul 12, 2024
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