Add an option to repo->redirect() to modify the HTTP status code #113

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
@phluid61
Contributor

phluid61 commented Jul 31, 2013

Allows the user to override the HTTP Status Code used when issuing a redirect.

Default uses 302; modification allows:

  • 301 -- permanent, may GET the alternate location (even if the request was a POST)
  • 302 -- temporary, may GET the alternate location
  • 303 -- a related resource, not an alternate location of the requested resource
  • 307 -- temporary, must GET the alternate location (even if the request was a POST)
  • 308 -- permanent, must GET the alternate location (experimental/draft)
Adds an option to repo->redirect()
Allows the user to override the HTTP Status Code used when issuing a redirect.

Default uses 302; modification allows:

* 301 -- permanent, may GET the alternate location (even if the request was a POST)
* 302 -- temporary, may GET the alternate location
* 303 -- a related resource, _not_ an alternate location of the requested resource
* 307 -- temporary, _must_ GET the alternate location (even if the request was a POST)
* 308 -- permanent, _must_ GET the alternate location (experimental/draft)
@sebastfr

This comment has been minimized.

Show comment
Hide comment
@sebastfr

sebastfr Aug 1, 2013

Hello,

Thanks for your pull request but I have some comments:

  • $message is optional: if left blank, Apache uses the default/expected strings (so 'Found' for 302, etc.)
  • I don't think aliases are necessary
  • In HTTP jargon, the "numbers" are usually referred to as "status code" or "code" not "mode"

So a simple improvement could be in the form of:

sub redirect
{
        my( $self, $url, %opts ) = @_;

        # Write HTTP headers if appropriate
        if( $self->{"offline"} )
        {
                print STDERR "ODD! redirect called in offline script.\n";
                return;
        }

        my $status = $opts{status_code} || 302;

        EPrints::Apache::AnApache::send_status_line( $self->{"request"}, $status );
        EPrints::Apache::AnApache::header_out( 
                $self->{"request"},
                "Location",
                $url );

        EPrints::Apache::AnApache::send_http_header( $self->{"request"}, %opts );
        
        return $status;
}

Hello,

Thanks for your pull request but I have some comments:

  • $message is optional: if left blank, Apache uses the default/expected strings (so 'Found' for 302, etc.)
  • I don't think aliases are necessary
  • In HTTP jargon, the "numbers" are usually referred to as "status code" or "code" not "mode"

So a simple improvement could be in the form of:

sub redirect
{
        my( $self, $url, %opts ) = @_;

        # Write HTTP headers if appropriate
        if( $self->{"offline"} )
        {
                print STDERR "ODD! redirect called in offline script.\n";
                return;
        }

        my $status = $opts{status_code} || 302;

        EPrints::Apache::AnApache::send_status_line( $self->{"request"}, $status );
        EPrints::Apache::AnApache::header_out( 
                $self->{"request"},
                "Location",
                $url );

        EPrints::Apache::AnApache::send_http_header( $self->{"request"}, %opts );
        
        return $status;
}

This comment has been minimized.

Show comment
Hide comment
@phluid61

phluid61 Aug 1, 2013

My original intention was to use symbolic identifiers for the redirection modes, abstracting the meaning from the codes, along the lines of "the requested resource permanently resides at another location" ('permanent'), similar for 'temporary', and a third for "I'm not showing you the thing you asked for, but there's something related over there" ('other'). The numerical forms were an afterthought, in case someone wanted to use the newer 307/308 variants (as I couldn't think of good, simple "mode" names for them). I'm quite happy to take it to the next level, and just let people specify the status code directly.

Should the status_code be deleted from opts before it's passed to send_http_header? I thought its presence would result in a strange Status-Code: 303 header in the HTTP response.

My original intention was to use symbolic identifiers for the redirection modes, abstracting the meaning from the codes, along the lines of "the requested resource permanently resides at another location" ('permanent'), similar for 'temporary', and a third for "I'm not showing you the thing you asked for, but there's something related over there" ('other'). The numerical forms were an afterthought, in case someone wanted to use the newer 307/308 variants (as I couldn't think of good, simple "mode" names for them). I'm quite happy to take it to the next level, and just let people specify the status code directly.

Should the status_code be deleted from opts before it's passed to send_http_header? I thought its presence would result in a strange Status-Code: 303 header in the HTTP response.

@sebastfr

This comment has been minimized.

Show comment
Hide comment
@sebastfr

sebastfr Aug 1, 2013

Contributor

I agree with you about deleting $opts{status_code}, it makes things cleaner.

Out of curiousity, what's your case study for this work?

Contributor

sebastfr commented Aug 1, 2013

I agree with you about deleting $opts{status_code}, it makes things cleaner.

Out of curiousity, what's your case study for this work?

dramatically simplified patch
%opts can now contain a {status_code}, which should be an (integer) HTTP
status code.
@phluid61

This comment has been minimized.

Show comment
Hide comment
@phluid61

phluid61 Aug 1, 2013

Contributor

We rearranged some of our repositories, and wanted to issue 301 (permanent) redirects instead of 302 in the hope that search engines might stop showing old links in search results (also it's more "correct", for what that's worth). $repo->redirect was an obvious place to help make the change.

Contributor

phluid61 commented Aug 1, 2013

We rearranged some of our repositories, and wanted to issue 301 (permanent) redirects instead of 302 in the hope that search engines might stop showing old links in search results (also it's more "correct", for what that's worth). $repo->redirect was an obvious place to help make the change.

@sebastfr

This comment has been minimized.

Show comment
Hide comment
@sebastfr

sebastfr Aug 1, 2013

Contributor

Fixed by 425b409 - Note that on the way I also changed get_secure/get_online to is_secure/is_online and made appropriate calls to the API (self->request) instead of using internal hash values (self->{request}).

Contributor

sebastfr commented Aug 1, 2013

Fixed by 425b409 - Note that on the way I also changed get_secure/get_online to is_secure/is_online and made appropriate calls to the API (self->request) instead of using internal hash values (self->{request}).

@sebastfr sebastfr closed this Aug 1, 2013

@phluid61 phluid61 deleted the QUTlib:redirect_options branch Aug 1, 2013

sebastfr added a commit that referenced this pull request Jun 3, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment