Slim: HTTP methods should not be made uppercase.

[Zegnat]

While looking at the code for #2156, I spotted strtoupper() being applied to methods. This is wrong. According to RFC 7230 section 3.1.1. (emphasis mine):

The method token indicates the request method to be performed on the target resource. The request method is case-sensitive.

I considered changing it in the PR I am working on, but I feel like it might be out of scope. I can imagine this change being a BC breaking change in case people were expecting methods to be normalised. Filing this as a separate issue so we can discuss when and where this should be fixed.

[tuupola]

FWIW this is also one part which fails on the PSR-7 integration tests.

https://gist.github.com/tuupola/5992c962f07068c4f4e3333c29d986fa

[sharifzadesina]

According to PSR-7 this is also right.

[...]

interface RequestInterface extends MessageInterface
{
    [...]

    /**
     * Return an instance with the provided HTTP method.
     *
     * While HTTP method names are typically all uppercase characters, HTTP
     * method names are case-sensitive and thus implementations SHOULD NOT
     * modify the given string.
     *
     * This method MUST be implemented in such a way as to retain the
     * immutability of the message, and MUST return an instance that has the
     * changed request method.
     *
     * @param string $method Case-sensitive method.
     * @return static
     * @throws \InvalidArgumentException for invalid HTTP methods.
     */
    public function withMethod($method);

    [...]
}

Tnx for reporting.

[Zegnat]

Thanks for referencing PSR-7, @sharifzadesina. I am more knowledgable of the HTTP spec than PSR-7. Can we treat this as a bug then, and fix it against 3.x, even if it might cause BC breaks?

[sharifzadesina]

@Zegnat All bug fixes are BC breaks, but slim have huge community so I don't think we can have this in 3.x, slim 4 will come soon, we can have this bug fix in version 4.

[Zegnat]

I am trying to establish what is and isn’t worth working on for #2156, for 3.x, and what could better be postponed for 4.x.

[akrabat]

The strtoupper() should certainly be removed from 4.x

[akrabat]

The problem with fixing this is 3.x, is that if anyone is currently testing against getMethod(), they are doing a simple == against an uppercase string. This stands a good chance of breaking.

[Zegnat]

[…] if anyone is currently testing against getMethod(), they are doing a simple == against an uppercase string. This stands a good chance of breaking.

To be honest, if they are receiving a post request and checking for the expected POST, this should break. Those two types of requests cannot be assumed to be the same.

As long as they are targeting browsers as UA, chances are they will never notice the change. Those that would break would be custom HTTP client implementations that for some reason have been sending lower-case methods when they meant to be sending standardised methods.

[akrabat]

Except that we break people's working apps.

[geggleto]

We are not changing this as I do not have time to fix all of my projects that use it. 🗡