Bug Report #4724

Response protocol is set to version HTTP 1.1 instead of 1.0 if exception occures

Added by Dominik Chvila over 1 year ago. Updated about 1 year ago.

Status:ClosedStart date:03/24/2013
Priority:NormalDue date:
Assignee:Lorenzo Pisani% Done:

100%

Category:Core
Target version:3.3.1
Resolution:fixed Points:1

Description

The same issue as in http://dev.kohanaframework.org/issues/4661 connected with http://dev.kohanaframework.org/issues/4662 but for exception output - when any exception is thrown and trace is printed out.

I assume, $response->protocol(Request::current()->protocol()); should be set after $response = Response::factory(); in tray and catch branches of the code of public static function response(Exception $e) in ..\system\classes\Kohana\Kohana\Exception.php


Related issues

Related to Kohana v3.x - Bug Report #4661: Output before and after all content, probably chunk size New 11/15/2012
Related to Kohana v3.x - Bug Report #4662: Response member _protocol is set to bad HTTP protocol ver... Closed 11/16/2012

History

#1 Updated by Dominik Chvila over 1 year ago

Probably my suggestion was not so good, I think, there should be $response->protocol(Request::initial()->protocol());

#2 Updated by Dominik Chvila over 1 year ago

Dominik Chvila wrote:

Probably my suggestion was not so good, I think, there should be $response->protocol(Request::initial()->protocol());

Sorry for bad solution... Request object because of some circumstances sometimes does not exist...

#3 Updated by Lorenzo Pisani over 1 year ago

  • Status changed from New to Assigned
  • Assignee set to Lorenzo Pisani

Did the fix for #4662 solve this bug?

#4 Updated by Dominik Chvila over 1 year ago

Lorenzo Pisani wrote:

Did the fix for #4662 solve this bug?

Hello, I am afraid not, that is why I commented it there:
http://dev.kohanaframework.org/issues/4662#note-3

However, I have had to add some "ifs" in \Kohana\Kohana\Exception.php after $response = Response::factory(); (line 256):

// Set the response protocol
if (Request::initial())
{
    $response->protocol(Request::initial()->protocol());
}

The same in "catch" part - unfortunately I do not remember the exact reason, probably sometimes Request::initial() was not available (= NULL).

#5 Updated by Robert-Jan Dreu -rjd22- over 1 year ago

Dominik: Could you please make a pull request or an detailed explanation on how to fix this for you? It's really hard to locate without test case.

#6 Updated by Dominik Chvila over 1 year ago

Yes, of course I can, but give me a week please ;-) too much work now...
(I will explain it, I have never done pull requests and deeper explanation would be better...)

#7 Updated by Dominik Chvila over 1 year ago

Hello, finally I can comment the issue. As I said, the fix of #4662 unfortunately did not solve #4724 (probably I have found better solution, which solves both issues). The reason is, that exception does not use the same response object. Exception creates new response and does not contain correct version of HTTP protocol.

Lets see:

https://github.com/kohana/core/blob/3.3/master/classes/Kohana/Kohana/Exception.php
lines 256 and 273

            /**
             * The stack trace becomes unmanageable inside PHPUnit.
             *
             * The error view ends up several GB in size, taking
             * serveral minutes to render.
             */
            if (defined('PHPUnit_MAIN_METHOD'))
            {
                $trace = array_slice($trace, 0, 2);
            }

            // Instantiate the error view.
            $view = View::factory(Kohana_Exception::$error_view, get_defined_vars());

            // Prepare the response object.
            $response = Response::factory(); // protocol is missing

            // Set the response status
            $response->status(($e instanceof HTTP_Exception) ? $e->getCode() : 500);

            // Set the response headers
            $response->headers('Content-Type', Kohana_Exception::$error_view_content_type.'; charset='.Kohana::$charset);

            // Set the response body
            $response->body($view->render());
        }
        catch (Exception $e)
        {
            /**
             * Things are going badly for us, Lets try to keep things under control by
             * generating a simpler response object.
             */
            $response = Response::factory(); // protocol is missing
            $response->status(500);
            $response->headers('Content-Type', 'text/plain');
            $response->body(Kohana_Exception::text($e));
        }

        return $response;
    }

At these two lines protocol is missing and later probably set by

    /**
     * Gets or sets the HTTP protocol. The standard protocol to use
     * is `HTTP/1.1`.
     *
     * @param   string   $protocol Protocol to set to the request/response
     * @return  mixed
     */
    public function protocol($protocol = NULL)
    {
        if ($protocol)
        {
            $this->_protocol = strtoupper($protocol);
            return $this;
        }

        if ($this->_protocol === NULL)
        {
            $this->_protocol = HTTP::$protocol;
        }

        return $this->_protocol;
    }

from
https://github.com/kohana/core/blob/3.3/master/classes/Kohana/Response.php
starting at line 171, however, because protocol is not passed via arguments to this function (it seems it is not passed), than protocol is set by defaul using HTTP::$protocol; where protocol is hardcoded to 'HTTP/1.1'.

So I would suggest to change the function mentioned above to the bellow one(same way as in https://github.com/kohana/core/blob/3.3/master/classes/Kohana/Request/Client.php on line 62) and revert fix of http://dev.kohanaframework.org/issues/4662.
It should solve
http://dev.kohanaframework.org/issues/4662
http://dev.kohanaframework.org/issues/4661
http://dev.kohanaframework.org/issues/4638

    /**
     * Gets or sets the HTTP protocol. The standard protocol to use
     * is `HTTP/1.1`.
     *
     * @param   string   $protocol Protocol to set to the request/response
     * @return  mixed
     */
    public function protocol($protocol = NULL)
    {
        if ($protocol)
        {
            $this->_protocol = strtoupper($protocol);
            return $this;
        }

        if ($this->_protocol === NULL && isset($_SERVER['SERVER_PROTOCOL']))
        {
            $this->_protocol = $_SERVER['SERVER_PROTOCOL'];
        }
        else
        {
            $this->_protocol = HTTP::$protocol;
        }

        return $this->_protocol;
    }

Hopefuly, it is clear enough to understand my suggestions and eplanation and I hope it will work :-)
I have tested it, so it should work...

#8 Updated by Lorenzo Pisani over 1 year ago

So I think I would rather remove all these hacks from our classes and instead just set HTTP::$protocol in bootstrap… that would simplify the classes a bit and lets you control what you want the default to be.

#9 Updated by Dominik Chvila over 1 year ago

Yes, HTTP::$protocol = $_SERVER['SERVER_PROTOCOL'] in bootstrap would be good solution as well.

Is $_SERVER['SERVER_PROTOCOL'] always set to any value?

#10 Updated by Dominik Chvila over 1 year ago

One remark: If you have more projects depending on one core, than you have to set protocol for each of the projects in bootstrap...

#11 Updated by Jeremy Bush over 1 year ago

Keep in mind that not all kohana apps are web apps.

#12 Updated by Lorenzo Pisani about 1 year ago

  • Status changed from Assigned to Closed
  • % Done changed from 0 to 100
  • Resolution set to fixed

Made the commits reference #4661 instead of this one =/ but this should be fixed once you make the needed change to bootstrap (https://github.com/kohana/kohana/commit/cfed7919cb526ec152f29dd1e7b7e1cee88ff0a3)

Let me know if I missed anything.

#13 Updated by Dominik Chvila about 1 year ago

Seems to bee OK, thanks.

(Not sure if is necessary to set
public static $protocol = 'HTTP/1.1';
- could be NULL, couldn't?
https://github.com/kohana/core/blob/3.3/master/classes/Kohana/HTTP.php#L22

I was thinking about this issue regarding "...I would rather remove all these hacks from our classes...", from my point of view, it could cleaner to "wire" this in the classes, because of this:

https://github.com/kohana/core/blob/3.3/master/classes/Kohana/Request.php#L62

However, your fix seems tom be OK, and this could be closed as well:
http://dev.kohanaframework.org/issues/4661

Thank you.

#14 Updated by Dominik Chvila about 1 year ago

Sorry, should answer myself...

public static $protocol = 'HTTP/1.1';
- could be NULL, couldn't?

No, could not, if $_SERVER['SERVER_PROTOCOL'] is not set...

Also available in: Atom PDF