Allow all valid HTTP methods

[amreladawy]

As allowed HTTP methods are defined in the protected $validMethods in the Request class, HTTP methods like LINK and UNLINK are not supported.

The proposed solution is to extend the Request class to add these methods. Wouldn't it be easier to have a setter function in the Request class or a function to append other HTTP methods to the $validMethods ?

#2005

[amreladawy]

Here is a pull request to add LINK/UNLINK

#2161

[akrabat]

See comment on PR. A closed list of methods doesn't make sense, so we need to fix it properly.

[johnhunt]

Rob suggested rewriting validateMethod() to use a regex along these lines: https://regex101.com/r/f80dzN/2

[Zegnat]

That regular expression (/^[[:alnum:]!#$%&\'*\/+\-.^_`|~]+$/i) seems wrong to me. It allows for / within a method, but this doesn’t seem to be allowed as per RFC 7230.

The method in the Request Line is defined in section 3.1.1.:

     method         = token

Token is defined in section 3.2.6.:

     token          = 1*tchar

     tchar          = "!" / "#" / "$" / "%" / "&" / "'" / "*"
                    / "+" / "-" / "." / "^" / "_" / "`" / "|" / "~"
                    / DIGIT / ALPHA
                    ; any VCHAR, except delimiters

(DIGIT and ALPHA are defined by RFC 5234, the ABNF syntax.)

The correct regex is: /^[[:alnum:]!#$%&'*+\-.^_`|~]+$/i.

[akrabat]

I'm not going to disagree :)

[akrabat]

Looking it up, Diactoros uses

/^[!#$%&\'*+.^_`\|~0-9a-z-]+$/i

(https://github.com/zendframework/zend-diactoros/blob/master/src/RequestTrait.php#L303)

[Zegnat]

@akrabat you will want to use double-backtick to encapsulate inline code that includes a backtick itself.

Other then the slight reorder and exchanging the POSIX [:alnum:], that one looks the same as mine. I was just dropping in to make sure the previously mentioned one wouldn’t get used and then needed to be changed.

[akrabat]

@Zegnat I agree. Would love a PR :)

[Zegnat]

@akrabat against 3.x or 4.x?

[akrabat]

It's a bug fix, so 3.x & we'll forward port to 4.x

[Zegnat]

I’ll see if I can get you a regular expression based PR by today then!

[akrabat]

👍 Don't forget the tests!

[Zegnat]

I am running into some scheduling conflicts and haven’t gotten around to working on this much. For the current 3.x branch, the following patch should be enough, but someone would still need to fix the tests:

--- Slim/Http/Request.php
+++ Slim/Http/Request.php
@@ -111,23 +111,6 @@ class Request extends Message implements ServerRequestInterface
     protected $uploadedFiles;

     /**
-     * Valid request methods
-     *
-     * @var string[]
-     */
-    protected $validMethods = [
-        'CONNECT' => 1,
-        'DELETE' => 1,
-        'GET' => 1,
-        'HEAD' => 1,
-        'OPTIONS' => 1,
-        'PATCH' => 1,
-        'POST' => 1,
-        'PUT' => 1,
-        'TRACE' => 1,
-    ];
-
-    /**
      * Create new HTTP request with data extracted from the application
      * Environment object
      *
@@ -349,7 +332,7 @@ class Request extends Message implements ServerRequestInterface
         }

         $method = strtoupper($method);
-        if (!isset($this->validMethods[$method])) {
+        if (preg_match("/^[!#$%&'*+.^_`|~0-9a-z-]+$/i", $method) !== 1) {
             throw new InvalidMethodException($this, $method);
         }

If someone wants to pick this up before I have time again: please.

[mathmarques]

Just wondering if we should PR this to 3.x. Isn't a BC break?

[geggleto]

@mathmarques it's kinda a bug fix though

[mathmarques]

@geggleto
If anyone is extending Request and changing $validMethods to only allow some methods they could have a problem. Slim will throw NotFoundException(if I'm not wrong) instead of InvalidMethodException.

[akrabat]

Please provide two PRs: one for 3.x and one for 4.x

The 3.x one keeps $validMethods but marks it deprecated in the docblock.