302 response code from drupal_redirect_form() violates HTTP/1.1 spec

subscribing

Bumping to 7.X where this should be implemented, and backported to 6.X if necessary.

You should add a comment referring to RFC.

Why adding form_post=1 to the action URL?

Thanks for bringing this issue up!

I have the same question as #6: why is any change to the form action url needed? Is there something in the spec that says a 303 must return a different URL? Why can't it return a redirect to the same URL the way that a 302 does?

Note: Many pre-HTTP/1.1 user agents do not understand the 303
status. When interoperability with such clients is a concern, the
302 status code may be used instead, since most user agents react
to a 302 response as described here for 303.

I'm concerned about this. Does this mean that by issuing a 303, we become incompatible with pre-HTTP/1.1 user agents and is that a problem? However, if we're currently incompatible with CDNs because we're not implementing HTTP/1.1 correctly, then I do think we need to do something, we just may need more logic here to switch the response code based on user agent.

Please correct issue title if I didn't get it quite right.

Well, we should issue 303, because it was designed to do so. On the other hand, I really doubt that this is causing issue with CDNs, I think the original poster was just confused (the extraneous ?form_post=1 is probably the part that fix the CDN issue for him/her).

I think at the time D8 is released, concern on non-HTTP/1.1 will be even less relevant than today, so:

• Bimping version
• Adding RFC mention
• Rerolling

Fixing obvious syntax error

array_push($form_state['redirect'], 303);

This needs to be smarter, as we actually support all of the drupal_goto() parameters in $form_state['redirect'], including the response code.

I recommend something along the lines of:

$form_state['redirect'] += array($_GET['q'], array(), 303);

In case $form_state value is checked somewhere after drupal_goto() call, I placed drupal_goto parameters in a new variable.
Also, I had to modify simpletest.test to expect 303 redirect

How about changing the default HTTP response code for drupal_goto() to 303? It seems that 303 is a more appropriate status code in the most common use-cases, also when redirecting a GET request.

For compatibility with HTTP/1.0 clients we can make the default 302 on HTTP/1.0 and 303 in newer clients. Note that even though HTTP/1.1 is almost 13 years old, there are still quite a few HTTP/1.0 clients around, including our own client in drupal_http_request().

303 code should be issued only as a response to POST-ed form submission.
drupal_goto() is used for other use cases as well (path redirects, redirects based on browser languages etc.), so - no, default status code should remain 302, and this is ok

303 code should be issued only as a response to POST-ed form submission.

That is not what the RFC says, is it?

AFAICT the semantics of 302 and 303 is almost the same. 302 seems to indicate that the resource has been moved temporarily (i.e. for some period of time) while 303 seems to be relevant in the case where the user is redirected due to some dynamic circumstances, i.e. when requesting the same URL again immediately after does not necessarily redirect to the same place.

If we cannot agree on changing the default status code for all requests, I suggest we should do it at least during POST requests. As described in this ticket, there is almost never a good reason for responding with 302 to a POST request, except for compatibility with HTTP/1.0 clients.

Pp. 10.3.4 303 See Other:

This method
exists primarily to allow the output of a POST-activated script to
redirect the user agent to a selected resource.

303 was not designed to replace 302, these status codes just have different meanings.

The quoted part of the RFC does not forbid or advice against the use of 303 in GET request.

I know that 302 and 303 have different meanings (my last post was an attempt to explain the difference as far as I interpret the RFC), but I was arguing that 303 may be more relevant in many use cases, even in GET requests.

Can you explain mentioned use cases? I still can't get why you suggest to change default status code of drupal_goto()

In my interpretation of the HTTP spec, the two status codes have these meanings (roughly speaking):

302

The resource A that has the URL B is temporarily accessible under the URL C.

303

The resource A that has the URL B is not available but please refer to the resource D under the URL C instead.
Note in particular this sentence in the spec: “The new URI is not a substitute reference for the originally requested resource”.

302 generally requires a “closer relationship” between the source and destination URLs than 303.

I looked through Drupal core to find examples of where drupal_goto() is used in GET requests. Here are some typical use-cases (not a complete list):

1. Redirect after doing some work: E.g. admin/reports/status/run-cron (implemented in system_run_cron()) runs cron, generates a status message with drupal_set_message() and then redirects to admin/config/system/cron. In this case, the two URLs do not refer to the same resource. There really is not resource identified by admin/reports/status/run-cron. I think this suits the 303 status code perfectly.
2. Redirect instead of returning 403 or 404: E.g. admin/config/regional/language/delete/en (implemented in language_admin_delete_form()) checks that you cannot delete the current language ("en"), so it calls drupal_set_message() and redirects to admin/config/regional/language (for other languages that the current a confirm form is displayed). In this case, the original URL reflects a resource that is currently not accessible, so it could return a 404 page. But to be user-friendly, it redirects somewhere else. Again, there is no identity between the resources on those two URLs, so the 303 status code is a good match.
   Likewise in comment_reply() that handles comment/reply/%node/%parent_comment. The function does various sanity checks (e.g. does the %node and %parent_comment match — if not, it really should return 404, because the URL has never been valid) and access control checks (e.g. is the user authorized to post comments etc. — if not, it really should return 403), and if any of these fail, it calls drupal_set_message() and redirect to node/%node. Also here 303 is a good match.
   Note that I am not arguing that we should actually send 403 or 404 in these cases. AFAIK these codes cannot be combined with a Location redirect header, so a 3xx code is probably better.
3. Temporary redirect too more specific page: If there is only one node type configured (with the machine name "foo"), node/add redirects to node/add/foo. If you consider node/add the “starting page for node creation”, I think it makes sense to state that this starting page is temporarily accessible via node/add/foo. So here a 302 redirect makes perfect sense.

#14: rfc2616-303-redirect-355157-14.patch queued for re-testing.

Retesting against latest HEAD since it has been over a year.

@kscheirer: Thanks for bringing this back to life
$_GET['q'] has gone since the last testbot run, + multiple other changes
Rerolling

Another try

As a side note, patch in #27 contains fix in EntityTranslationController.php, which used $form_state['redirect'] in a weird way:

$form_state['redirect'] = array('path' => $path);

As far as I see, this is the only place in the core source, where $form_state['redirect'] is a one-element associative array.

$form_state['redirect'] = array('path' => $path);
...
call_user_func_array('drupal_goto', $form_state['redirect']);

call_user_func_array() expects indexed, not associative array.
This works (due to all-forgiving PHP nature), but only until you don't attempt to modify $form_state['redirect']

Pretty sure this no longer applies... :-) Still worth doing though.

Rerolling

Would be good to add some explicit test coverage for the expected response code to one of the existing Form API tests.

Not sure whether this can be tested in phpunit. A WebTest needs to temporarily set/override $this->additionalCurlOptions to disable curlExec()'s automated following of HTTP redirects, so you can $this->assertResponse() the response prior to the redirect.

+++ b/core/lib/Drupal/Core/Form/FormSubmitter.php
@@ -169,7 +169,9 @@ public function redirectForm($form_state) {
+      // According to RFC 2616, 303 See Other status code must be used
+      // to redirect user agent (and not default 302 Found).

Let's reference the new RFC instead. Also, the Response class has constants for all of the legal return codes. Let's use those to make the code a bit more self-documenting. (Response::HTTP_SEE_OTHER in this case)

Added reference to the new RFC, plus replaced constants with Response::SEE_OTHER.
@sun: returned status code is already tested, can this be a followup issue?

As it turns out, there *is* explicit test for return status (ammended FormSubmitterTest)

My bad

Thanks!

The inline comment reads a bit lengthy/wordy and could be shortened to just one line, but ok.

Committed f531d9d and pushed to 8.x. Thanks!