Currently, drupal_goto invokes header('Location: ') which by default generates a HTTP 302 redirect.

However, in some situations, especially considering the search engine stance on HTTP 302 redirects, it is necessary or advisable to use 301 redirects. This is obtained in PHP by passing the third parameter (int http_response_code) to header. The second parameter should in that case be TRUE, which is its default value.

I think the best way to implement this is an optional fourth parameter to drupal_goto:

function drupal_goto($path = '', $query = NULL, $fragment = NULL, $http_response_code = 302) {
[...]
  header('Location: '. $url, TRUE, $http_response_code);
[...]
CommentFileSizeAuthor
#30 common.inc_52.patch1.9 KBgreggles
#16 common.inc_40.patch1.88 KBfgm
#11 common.inc_39.patch1.59 KBfgm
#10 common.inc_38.patch1.67 KBfgm
#6 common.inc_33.patch1.67 KBfgm
#1 common.inc_29.patch1.18 KBfgm

Comments

fgm’s picture

fgm
Primary language French
Location Paris, France
commented
Status: Active » Needs review
StatusFileSize
new common.inc_29.patch1.18 KB

submitting patch.

fgm’s picture

fgm
Primary language French
Location Paris, France
commented
Version: 4.7.2 » x.y.z

Forgot to mention this applies to both 4.7.3 and HEAD.

Steven’s picture

Steven commented

The response code parameter should be documented along with its valid/useful values.

fgm’s picture

fgm
Primary language French
Location Paris, France
commented

The official definition is here:
http://tools.ietf.org/html/rfc2616#section-10.3

All of the class 300 statuses may be used. Filerequest performs its 304 redirection manually, and G2 Glossary needs a 301, as would actually most uses of drupal_goto, I think.

To sum them up:

  • 301 Moved Permanently - what should probably be the default, although PHP defaults to 302
  • 302 Found - notably used for spamdexing, current default for drupal_goto
  • 303 See Other - might be of use in FAPI
  • 304 Not Modified - could be used by the caching API with interesting results
  • 305 Use Proxy
  • 306 (Unused) - reserved
  • 307 Temporary Redirect - similar to 302, but apparently yet unused for spamdexing
fgm’s picture

fgm
Primary language French
Location Paris, France
commented

Thinking of it again, a nice use case for status 307 would be the "site maintenance" situation: temporary redirect to the site maintenance page. Not automatically refreshed; probably not recorded by search engines as a replacement of the normal pages...

fgm’s picture

fgm
Primary language French
Location Paris, France
commented
StatusFileSize
new common.inc_33.patch1.67 KB

Patch rerolled against today's version of common.inc (1.572) to include documentation for the new parameter.

neclimdul’s picture

neclimdul
He/Him
commented
Status: Needs review » Needs work

This could be a very handy feature. The patch doesn't work though. When I changed the header() call to
header('Location: '. $url, TRUE, $http_response_code);
I was able to fire of 301 redirects. I only tested this with php5 but it looks like this should be the way things work in php4 as well from php.net.

fgm’s picture

fgm
Primary language French
Location Paris, France
commented
Status: Needs work » Needs review

This is exactly what should happen: if you use an ancient version of PHP (lower than 4.3) or if don't specify otherwise, redirects are done with a 301 status. If you use the optional parameter, *and* if the version of PHP used supports it (4.3 or higher), the $response_code is used instead of 301.

See http://fr.php.net/header for details.

neclimdul’s picture

neclimdul
He/Him
commented
Status: Needs review » Needs work

no, the patch does not work. specifying a responce code does not change the responce code given. (and 302 is the default). The patch is missing a parameter in the header() call. Actually in looking at the previous patch it was included.

fgm’s picture

fgm
Primary language French
Location Paris, France
commented
StatusFileSize
new common.inc_38.patch1.67 KB

Well, that's what happens when you write patches by hand because you don't have access to CVS.

Rerolled once more.

fgm’s picture

fgm
Primary language French
Location Paris, France
commented
StatusFileSize
new common.inc_39.patch1.59 KB

And another one bites the dust :-)

neclimdul’s picture

neclimdul
He/Him
commented
Status: Needs work » Reviewed & tested by the community

Looks good and clean. RTBC IMO

fgm’s picture

fgm
Primary language French
Location Paris, France
commented

Killes suggested I added a note to explain what is spamdexing and why it matters to have this "feature" integrated in HEAD and backported to 4.7.3, since this is more a bug fix than a feature addition:

dopry’s picture

dopry commented

Code style looks good.

Allows us to properly do redirects with the right status
code within drupal and still have the exit hook fired.

I haven't tested, but as long as php's header function isn't broken this patch should work.

adding the link to the spamdexing and the http response code documentation might be nice
but isn't necessary by any means.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Needs work

The documentation needs more work, IMO. It doesn't provide practical advice, some of the response code appear to be irrelevant, and terms like 'spamdexing' are not common vocabulary. Also, we write 'TRUE' and not 'true'.

fgm’s picture

fgm
Primary language French
Location Paris, France
commented
Status: Needs work » Needs review
StatusFileSize
new common.inc_40.patch1.88 KB

Rerolled against today's head to include your requirements:

  • documentation split in two parts:
    1. RFC-defined values
    2. practical advice
  • removed "spamdexing" as the primary explanation
  • uppercased "true"
coreb’s picture

coreb commented
Version: x.y.z » 5.x-dev

Moving out of the "x.y.z" queue to a real queue.

Even though it's a feature request, it looks like there may be a relevant patch for the 5.x-dev queue.

joshk’s picture

joshk commented

+1 for style; I know little about HTTP result codes, but found the doc readable.

Should it include some external link reference? I'm thinking of looking this up via api.drupal.org. External references might be nice.

joshk’s picture

joshk commented
Status: Needs review » Reviewed & tested by the community

Applied clean and works as advertised.

Steven’s picture

Steven commented
Status: Reviewed & tested by the community » Fixed

Tweaked the docs a bit and committed to HEAD. Nice work.

fgm’s picture

fgm
Primary language French
Location Paris, France
commented
Version: 5.x-dev » 4.7.x-dev
Status: Fixed » Needs review

Any hope of having it backported to the 4.7.x branch now that it's been validated for 5.x-dev ?

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Needs review » Patch (to be ported)

I believe it needs to be ported, not just reviewed.

It makes sense to me to backport, but that lies with others to decide.

Steven’s picture

Steven commented

This is a new ability added to an API function, which is not added to old releases normally. The only argument for including is that the additional codes simply alter the meaning of the redirect, but that the actual redirect still takes place.

I'll leave this up to Gerhard.

killes@www.drop.org’s picture

killes@www.drop.org commented
Status: Patch (to be ported) » Fixed

I think it is a new feature and we don't need to backport it.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)
hass’s picture

hass commented

A backport is required for "Global Redirect" module...

hass’s picture

hass commented

and i forgotton the path_redirect module...

hass’s picture

hass commented
Status: Closed (fixed) » Active

please backport this patch for 4.7.5.

see my last two postings... it is required for some modules that are broken today. however i don't know why the maintainers don't know about...

Global Redirect Bug:
http://drupal.org/node/105274

Path Redirect Bug:
http://drupal.org/node/105281

fgm’s picture

fgm
Primary language French
Location Paris, France
commented

FWIW, backporting to 4.7.5 would also allow me to close http://drupal.org/node/75794 instead of postponing it until a 5.0 version of G2 Glossary is created.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Active » Needs review
StatusFileSize
new common.inc_52.patch1.9 KB

It's up to Gerhard if we need it or not, but I think it would be a good addition. 4.7 will be around for a while longer and this is a very useful/important feature. You could argue that since 302 redirects can get sites blocked from search engines this is a bug.

My contribution is simply to make reroll the patch so it applies cleanly and set the status appropriately. If nothing else this will allow users of 4.7 to easily apply this patch and use this function.

killes@www.drop.org’s picture

killes@www.drop.org commented
Status: Needs review » Fixed

I already said I won't backport it.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)