Loading ...
Sorry, an error occurred while loading the content.

Re: [mp2] [patch] CGI::Cookie does not work prior to response phase

Expand Messages
  • Nick Tonkin
    ... But then does nothing if it s not defined. No graceful handling in other words. Or do I miss something? - nick -- ~~~~~~~~~~~~~~~~~~~~ Nick Tonkin {|8^)
    Message 1 of 16 , Apr 10, 2003
    • 0 Attachment
      On Thu, 10 Apr 2003, Nick Tonkin wrote:

      > On Fri, 11 Apr 2003, Stas Bekman wrote:
      >
      > > Nick Tonkin wrote:
      > > [...]
      > >
      > > Your diff is the other way around, - should say what was removed and + what
      > > was added, you just need to swap the order of files while doing diff-- the
      > > newer file last...
      > >
      > > > 1) is the use of Apache->request correct? For mp1 and mp2?
      > >
      > > Yes
      > >
      > > > 2) the code to check for mod_perl does not appear to do any error
      > > > checking, even when evalling. (Again I note that I copied it from
      > > > CGI.pm)
      > >
      > > It's correct, since it checks 'defined $mod_perl::VERSION'

      But then does nothing if it's not defined. No graceful handling in other words.
      Or do I miss something?

      - nick

      --

      ~~~~~~~~~~~~~~~~~~~~
      Nick Tonkin {|8^)>
    • Stas Bekman
      ... Yup, I knew that this will happen. It misses this comment. I ve submitted a patch to Lincoln to add this comment. if (exists $ENV{MOD_PERL}) { eval
      Message 2 of 16 , Apr 10, 2003
      • 0 Attachment
        Nick Tonkin wrote:
        > On Thu, 10 Apr 2003, Nick Tonkin wrote:
        >
        >
        >>On Fri, 11 Apr 2003, Stas Bekman wrote:
        >>
        >>
        >>>Nick Tonkin wrote:
        >>>[...]
        >>>
        >>>Your diff is the other way around, - should say what was removed and + what
        >>>was added, you just need to swap the order of files while doing diff-- the
        >>>newer file last...
        >>>
        >>>
        >>>>1) is the use of Apache->request correct? For mp1 and mp2?
        >>>
        >>>Yes
        >>>
        >>>
        >>>>2) the code to check for mod_perl does not appear to do any error
        >>>> checking, even when evalling. (Again I note that I copied it from
        >>>> CGI.pm)
        >>>
        >>>It's correct, since it checks 'defined $mod_perl::VERSION'
        >>
        >
        > But then does nothing if it's not defined. No graceful handling in other words.
        > Or do I miss something?

        Yup, I knew that this will happen. It misses this comment. I've submitted a
        patch to Lincoln to add this comment.

        if (exists $ENV{MOD_PERL}) {
        eval "require mod_perl";
        # mod_perl handlers may run system() on scripts using CGI.pm
        # - make sure so we don't get fooled by inherited $ENV{MOD_PERL}
        if (defined $mod_perl::VERSION) {
        if ($mod_perl::VERSION >= 1.99) {

        it talks about system() calls made from mod_perl scripts. When this happens
        $ENV{MOD_PERL} exists, but eval fails, $mod_perl::VERSION undefined == we
        don't run under mod_perl.

        __________________________________________________________________
        Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
        http://stason.org/ mod_perl Guide ---> http://perl.apache.org
        mailto:stas@... http://use.perl.org http://apacheweek.com
        http://modperlbook.org http://apache.org http://ticketmaster.com
      • Stas Bekman
        ... Functionalty seems to be alright, I d just do one last tiny change: + my $raw_cookie; + my $r = shift; + $r ||= eval { Apache- request() } if
        Message 3 of 16 , Apr 10, 2003
        • 0 Attachment
          > +# Fetch a list of cookies from the environment or the incoming headers and
          > +# return as a hash. The cookies are parsed as normal escaped URL data.
          > sub fetch {
          > my $class = shift;
          > - my $raw_cookie = $ENV{HTTP_COOKIE} || $ENV{COOKIE};
          > + my $raw_cookie;
          > + my $r;
          > + $r = shift if $MOD_PERL;
          > + $r ||= eval { Apache->request() } if $MOD_PERL;
          > + if ($r) {
          > + $raw_cookie = $r->headers_in->{'Cookie'};
          > + } else {
          > + if (exists $ENV{REQUEST_URI}) {
          > + $raw_cookie = $ENV{HTTP_COOKIE} || $ENV{COOKIE};
          > + } else {
          > + die '%ENV has not been set. Turn SetupEnv on or ' .
          > + 'run $r->subprocess_env()';
          > + }
          > + }
          > return () unless $raw_cookie;
          > return $class->parse($raw_cookie);
          > }

          Functionalty seems to be alright, I'd just do one last tiny change:

          + my $raw_cookie;
          + my $r = shift;
          + $r ||= eval { Apache->request() } if $MOD_PERL;
          + if ($r) {
          + $raw_cookie = $r->headers_in->{'Cookie'};
          + } else {
          + if (exists $ENV{REQUEST_URI}) {
          + $raw_cookie = $ENV{HTTP_COOKIE} || $ENV{COOKIE};
          + } else {
          + die '%ENV has not been set. Turn SetupEnv on or ' .
          + 'run $r->subprocess_env()';
          + }
          + }

          but yours is just fine (though if you decide to go with yours pack the two if
          $MOD_PERL together).

          __________________________________________________________________
          Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
          http://stason.org/ mod_perl Guide ---> http://perl.apache.org
          mailto:stas@... http://use.perl.org http://apacheweek.com
          http://modperlbook.org http://apache.org http://ticketmaster.com
        • Nick Tonkin
          On Fri, 11 Apr 2003, Stas Bekman wrote: +# Fetch a list of cookies from the environment or the incoming headers and +# return as a hash. The cookies
          Message 4 of 16 , Apr 10, 2003
          • 0 Attachment
            On Fri, 11 Apr 2003, Stas Bekman wrote:

            >
            > > +# Fetch a list of cookies from the environment or the incoming headers and
            > > +# return as a hash. The cookies are parsed as normal escaped URL data.
            > > sub fetch {
            > > my $class = shift;
            > > - my $raw_cookie = $ENV{HTTP_COOKIE} || $ENV{COOKIE};
            > > + my $raw_cookie;
            > > + my $r;
            > > + $r = shift if $MOD_PERL;
            > > + $r ||= eval { Apache->request() } if $MOD_PERL;
            > > + if ($r) {
            > > + $raw_cookie = $r->headers_in->{'Cookie'};
            > > + } else {
            > > + if (exists $ENV{REQUEST_URI}) {
            > > + $raw_cookie = $ENV{HTTP_COOKIE} || $ENV{COOKIE};
            > > + } else {
            > > + die '%ENV has not been set. Turn SetupEnv on or ' .
            > > + 'run $r->subprocess_env()';
            > > + }
            > > + }
            > > return () unless $raw_cookie;
            > > return $class->parse($raw_cookie);
            > > }
            >
            > Functionalty seems to be alright, I'd just do one last tiny change:

            Done. Patch attached.

            >
            > + my $raw_cookie;
            > + my $r = shift;
            > + $r ||= eval { Apache->request() } if $MOD_PERL;
            > + if ($r) {
            > + $raw_cookie = $r->headers_in->{'Cookie'};
            > + } else {
            > + if (exists $ENV{REQUEST_URI}) {
            > + $raw_cookie = $ENV{HTTP_COOKIE} || $ENV{COOKIE};
            > + } else {
            > + die '%ENV has not been set. Turn SetupEnv on or ' .
            > + 'run $r->subprocess_env()';
            > + }
            > + }
            >
            > but yours is just fine (though if you decide to go with yours pack the two if
            > $MOD_PERL together).
            >
            > __________________________________________________________________
            > Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
            > http://stason.org/ mod_perl Guide ---> http://perl.apache.org
            > mailto:stas@... http://use.perl.org http://apacheweek.com
            > http://modperlbook.org http://apache.org http://ticketmaster.com
            >


            - nick

            --

            ~~~~~~~~~~~~~~~~~~~~
            Nick Tonkin {|8^)>
          • Stas Bekman
            ... it s set to . Notice that we check: exists $ENV{QUERY_STRING} may be using REQUEST_METHOD is better. ... which is a wrong assumption I suppose, otherwise
            Message 5 of 16 , Apr 10, 2003
            • 0 Attachment
              > Hm. So $ENV{QUERY_STRING} gets set (to what?) when there is no query string?

              it's set to "". Notice that we check:

              exists $ENV{QUERY_STRING}

              may be using REQUEST_METHOD is better.

              > Whatever. I don't care either, I just think it's more intuitive to assume that
              > there's always going to be a PATH_INFO ...

              which is a wrong assumption I suppose, otherwise it's would be always set in
              C. You are probably confusing it with REQUEST_URI, which is always there and
              also an intuitive choice.

              __________________________________________________________________
              Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
              http://stason.org/ mod_perl Guide ---> http://perl.apache.org
              mailto:stas@... http://use.perl.org http://apacheweek.com
              http://modperlbook.org http://apache.org http://ticketmaster.com
            • Stas Bekman
              ... Great! mp1 has a test which uses CGI::Cookie, so you probably want to test that it works. Also porting the test to mp2 s test suite would be great too.
              Message 6 of 16 , Apr 10, 2003
              • 0 Attachment
                > Done. Patch attached.

                Great!

                mp1 has a test which uses CGI::Cookie, so you probably want to test that it
                works. Also porting the test to mp2's test suite would be great too.

                __________________________________________________________________
                Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
                http://stason.org/ mod_perl Guide ---> http://perl.apache.org
                mailto:stas@... http://use.perl.org http://apacheweek.com
                http://modperlbook.org http://apache.org http://ticketmaster.com
              • Nick Tonkin
                ... As you can see that s what I went with. - nick -- ~~~~~~~~~~~~~~~~~~~~ Nick Tonkin {|8^)
                Message 7 of 16 , Apr 10, 2003
                • 0 Attachment
                  On Fri, 11 Apr 2003, Stas Bekman wrote:

                  >
                  > > Hm. So $ENV{QUERY_STRING} gets set (to what?) when there is no query string?
                  >
                  > it's set to "". Notice that we check:
                  >
                  > exists $ENV{QUERY_STRING}
                  >
                  > may be using REQUEST_METHOD is better.
                  >
                  > > Whatever. I don't care either, I just think it's more intuitive to assume that
                  > > there's always going to be a PATH_INFO ...
                  >
                  > which is a wrong assumption I suppose, otherwise it's would be always set in
                  > C. You are probably confusing it with REQUEST_URI, which is always there and
                  > also an intuitive choice.
                  >

                  As you can see that's what I went with.

                  - nick

                  --

                  ~~~~~~~~~~~~~~~~~~~~
                  Nick Tonkin {|8^)>
                • Nick Tonkin
                  ... I don t have mp1 installed here ... and I m sure done for the night. Not sure when I can get any more mod_perl time ... - nick -- ~~~~~~~~~~~~~~~~~~~~ Nick
                  Message 8 of 16 , Apr 10, 2003
                  • 0 Attachment
                    On Fri, 11 Apr 2003, Stas Bekman wrote:

                    >
                    > > Done. Patch attached.
                    >
                    > Great!
                    >
                    > mp1 has a test which uses CGI::Cookie, so you probably want to test that it
                    > works. Also porting the test to mp2's test suite would be great too.

                    I don't have mp1 installed here ... and I'm sure done for the night. Not sure
                    when I can get any more mod_perl time ...

                    - nick

                    --

                    ~~~~~~~~~~~~~~~~~~~~
                    Nick Tonkin {|8^)>
                  • Stas Bekman
                    ... I tested your patch against the mp1 test suite, so we now have Apache- request path tested.
                    Message 9 of 16 , Apr 10, 2003
                    • 0 Attachment
                      Nick Tonkin wrote:
                      > On Fri, 11 Apr 2003, Stas Bekman wrote:
                      >
                      >
                      >>>Done. Patch attached.
                      >>
                      >>Great!
                      >>
                      >>mp1 has a test which uses CGI::Cookie, so you probably want to test that it
                      >>works. Also porting the test to mp2's test suite would be great too.
                      >
                      >
                      > I don't have mp1 installed here ... and I'm sure done for the night. Not sure
                      > when I can get any more mod_perl time ...

                      I tested your patch against the mp1 test suite, so we now have Apache->request
                      path tested.

                      __________________________________________________________________
                      Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
                      http://stason.org/ mod_perl Guide ---> http://perl.apache.org
                      mailto:stas@... http://use.perl.org http://apacheweek.com
                      http://modperlbook.org http://apache.org http://ticketmaster.com
                    Your message has been successfully submitted and would be delivered to recipients shortly.