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

Implement Post/Redirect/Get pattern to avoid form resubmission #465

Open
NicoHood opened this issue Dec 27, 2020 · 7 comments
Open

Implement Post/Redirect/Get pattern to avoid form resubmission #465

NicoHood opened this issue Dec 27, 2020 · 7 comments

Comments

@NicoHood
Copy link
Contributor

NicoHood commented Dec 27, 2020

See: https://stackoverflow.com/q/6320113/14348748
And: https://en.wikipedia.org/wiki/Post/Redirect/Get

It would be nice to have an option to redirect to the same page using a statuscode of 303. The form plugin has a redirect option available, and even the process event supports a redirect_code. But it is not used yet. Also the processing of the parameters does not include the page variable. The function always uses the redirect code 302.

Suggested fix: #466

NicoHood added a commit to NicoHood/grav-plugin-form that referenced this issue Dec 27, 2020
@NicoHood
Copy link
Contributor Author

NicoHood commented Dec 27, 2020

Maybe it is a bit offtopic, but I've seen undocumented code about remember_state , remember_redirect and uniqueid. What can this be used for? Basically I want to avoid resubmitting the same form again, but currently it seems this is still possible with pressing F5 after submitting. Even with the fix above, if you go back in history that is still possible. Is there already an option to avoid that?

@mahagr
Copy link
Member

mahagr commented Jan 8, 2021

302 Found
This is the most popular redirect code, but also an example of industrial practice contradicting the standard. HTTP/1.0 specification (RFC 1945 ) required the client to perform a temporary redirect (the original describing phrase was “Moved Temporarily”), but popular browsers implemented it as a 303 See Other. Therefore, HTTP/1.1 added status codes 303 and 307 to disambiguate between the two behaviors. However, the majority of Web applications and frameworks still use the 302 status code as if it were the 303.
https://www.pmg.com/blog/301-302-303-307-many-redirects/

This said I think we should default on 303 as the intended method is a GET for the redirect. @rhukster ?

If you press F5, the form will reset and the browser may or may not pre-fill the data with your input. So basically submitting the form again will be a different form than the one you submitted before.

@NicoHood
Copy link
Contributor Author

NicoHood commented Jan 8, 2021

When speaking of defaults, I'd also default to page.url here, as it will simply implement the Post/Redirect/Get pattern. That would make sense ;-)

I've updated the PR: 7d1c158

@mahagr
Copy link
Member

mahagr commented Jan 11, 2021

It does make sense, though because of how the page.url works, you may actually end up going to a different page as pagination and any other grav/query parameter would be lost.

@NicoHood
Copy link
Contributor Author

I am not sure if I understand correct.

The current behavior:

  • Redirect to a static page
  • Or specify a twig template to create a dynamic url (but only from is available as variable)

The new behavior:

  • Everything as above
  • Add page as additional variable to twig processing
  • Add the option to specify a redirect code
  • Default redirect code to 303
  • Default redirect url to page.url

The patch only extends the options and is fully backward compatible.

If someone wants something special, he must use a different twig template. I've done that in my code already, as I need to redirect and include a specific anchor: route: '{{ page.url ~ "#modal-rating-close" }}'

@NicoHood
Copy link
Contributor Author

NicoHood commented Jan 11, 2021

I just noticed, that page.url returns the url of the page, that contains the form definition, not the page that is currently loaded. That is a pity, but still no regression. I will try to fix that though. If you have any hints, let me know.

@NicoHood
Copy link
Contributor Author

Update: The issue is a separate one, so this PR is not directly affected. I've tracked the issue here: #478

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

No branches or pull requests

2 participants