Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request HTTP method defaults to empty string #26

Closed
weierophinney opened this issue Dec 31, 2019 · 8 comments
Closed

Request HTTP method defaults to empty string #26

weierophinney opened this issue Dec 31, 2019 · 8 comments
Labels
Bug Something isn't working

Comments

@weierophinney
Copy link
Member

Discovered while digging in php-http/curl-client#14

Apparently, diactoros defaults the HTTP method when building a new Request('http://example.com') to '' (empty string). As far as I know, an empty string is not a valid HTTP method (not sure if that assumption is reflected in the HTTP spec), and therefore the initial state of a diactoros HTTP request is invalid, and should lead to an exception.


Originally posted by @Ocramius at zendframework/zend-diactoros#150

@weierophinney weierophinney added the Bug Something isn't working label Dec 31, 2019
@weierophinney
Copy link
Member Author

What would you consider a valid default? GET? HEAD? OPTIONS?

Additionally, IIRC, somebody presented a use case for allowing a nullable
method, so we'll need to see if those needs are still valid, and how to
handle that with the concept of a default.
On Mar 4, 2016 8:30 PM, "Marco Pivetta" [email protected] wrote:

Discovered while digging in php-http/curl-client#14
php-http/curl-client#14

Apparently, diactoros defaults the HTTP method when building a new
Request('http://example.com') to '' (empty string). As far as I know, an
empty string is not a valid HTTP method (not sure if that assumption is
reflected in the HTTP spec), and therefore the initial state of a diactoros
HTTP request is invalid, and should lead to an exception.


Reply to this email directly or view it on GitHub
zendframework/zend-diactoros#150.


Originally posted by @weierophinney at zendframework/zend-diactoros#150 (comment)

@weierophinney
Copy link
Member Author

What would you consider a valid default? GET? HEAD? OPTIONS?

That is a good question, but I'm fairly sure that 90% of the web traffic is just GET, so going with that is a quite decent choice.
That would just be the default value, but the idea is to simply reject anything that isn't a valid HTTP method. For example, HTTP methods with invalid characters should also be rejected (spaces are one simple case that can be handled).

Overall, this logic can be encapsulated in a tiny HttpMethod value object, which doesn't need to be exposed to userland.


Originally posted by @Ocramius at zendframework/zend-diactoros#150 (comment)

@weierophinney
Copy link
Member Author

I've just remembered that i implemented psr-7 starting from phly/http and added a default method in the constructor ('GET') and a simple http-method filtering method (mwop would have nameed it marhallMethod). Juts to get an idea form ths code fragment

//...
    protected static $validMethods = [
        'OPTIONS'  => true,
        'GET'      => true,
        'HEAD'     => true,
        'POST'     => true,
        'PUT'      => true,
        'DELETE'   => true,
        'TRACE'    => true,
        'CONNECT'  => true,
        'PATCH'    => true,
        'PROPFIND' => true,
    ];

    /**
     * Array of possible CSRF Header names
     * @var array
     */
    protected static $csrfHeaderNames = [
        'X-CSRF-Token',
        'X-CSRFToken',
        'X-XSRF-TOKEN',
    ];

    /**
     * Constructor
     * @param UriInterface $uri
     * @param string $method
     * @param array $headers
     * @param Stream|resource|string $body
     * @param string $protocolVersion
     * @throws InvalidArgumentExceptions
     */
    public function __construct(
        $uri = null,
        $method = 'GET',
        $headers = [],
        $body = 'php://temp',
        $protocolVersion = '1.1'
    ) {
        parent::__construct($protocolVersion, $headers, $body);

        $this->method = $this->filterMethod($method);

        // Initialize uri from constructor argument or build uri from request
        // environment
        if (null === $uri) {
            $this->uri = new Uri('');
        } else if (is_string($uri)) {
            $this->uri = new Uri($uri);
        } elseif($uri instanceof UriInterface) {
            $this->uri = $uri;
        } else {
            throw new InvalidArgumentException(
                'The constructor $uri must be a string, an instance of UriInterface or null'
            );
        }
    }
//...
    /**
     * Validate the HTTP method
     *
     * @param null|string $method
     * @throws InvalidArgumentException on invalid HTTP method.
     */
    protected function filterMethod($method)
    {
        if (null === $method) {
            return 'GET';
        }

        if (! is_string($method)) {
            throw new InvalidArgumentException(
                'The HTTP method must be a string'
            );
        }

        $method = strtoupper($method);

        if (! isset(static::$validMethods[$method])) {
            throw new InvalidArgumentException(sprintf(
                'Unsupported HTTP method "%s"',
                $method
            ));
        }

        return $method;
    }

Originally posted by @pine3ree at zendframework/zend-diactoros#150 (comment)

@Xerkus
Copy link
Member

Xerkus commented May 1, 2023

@Ocramius is this still relevant?

@Ocramius
Copy link
Member

Ocramius commented May 1, 2023

Haven't used it in a while, but I don't remember us validating this input anywhere.

@Xerkus
Copy link
Member

Xerkus commented May 1, 2023

@Ocramius behavior is unchanged since the issue was opened. I was rather asking if it is a behavior that you still think needs to be changed.

@Ocramius
Copy link
Member

Ocramius commented May 1, 2023

Yeah, I'd say that an empty HTTP method is not viable, so we'd need some default or some exception

@Xerkus Xerkus modified the milestone: 3.0.0 May 1, 2023
@Xerkus
Copy link
Member

Xerkus commented May 1, 2023

Nevermind. I tested it wrong and missed default method value in the trait.

private $method = 'GET';

This is already handled.

@Xerkus Xerkus closed this as completed May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants