From 3fcd8ae94e9f3395f70fc45a36b32b1db7ad7e4a Mon Sep 17 00:00:00 2001
From: Larry Garfield <larry@garfieldtech.com>
Date: Thu, 12 May 2022 14:25:55 -0500
Subject: [PATCH] [BUGFIX] Fix type issues in Request
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This patch adds property and return types to the core Request class.
Parameter types cannot be added to methods from RequestInterface
as it has no typed version yet.
Doing so allows for the removal of a lot of now-unnecessary tests
as well as PHPstan baseline issues.
This patch also notes, but does not correct, the incorrect
implementation of getUri(), which this implementation permits
to return null even though that is a PSR-7 violation. Fixing
that has several knock-on effects better addressed in
future patches.
Resolves: #97620
Releases: main
Change-Id: I9b754a261b77fa8fdb6fa58706edf8a5c5248be4
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/74634
Tested-by: Daniel Haupt <mail@danielhaupt.de>
Tested-by: core-ci <typo3@b13.com>
Tested-by: Simon Schaufelberger <simonschaufi+typo3@gmail.com>
Tested-by: Stefan Bürk <stefan@buerk.tech>
Reviewed-by: Daniel Haupt <mail@danielhaupt.de>
Reviewed-by: Simon Schaufelberger <simonschaufi+typo3@gmail.com>
Reviewed-by: Stefan Bürk <stefan@buerk.tech>
---
Build/phpstan/phpstan-baseline.neon | 20 ----
typo3/sysext/core/Classes/Http/Request.php | 104 ++++++++----------
.../core/Classes/Http/ServerRequest.php | 2 +-
.../Tests/Unit/Http/RequestFactoryTest.php | 27 -----
.../core/Tests/Unit/Http/RequestTest.php | 69 +-----------
.../Unit/Http/ServerRequestFactoryTest.php | 27 -----
6 files changed, 51 insertions(+), 198 deletions(-)
diff --git a/Build/phpstan/phpstan-baseline.neon b/Build/phpstan/phpstan-baseline.neon
index 567c1e159720..f0152cd56694 100644
--- a/Build/phpstan/phpstan-baseline.neon
+++ b/Build/phpstan/phpstan-baseline.neon
@@ -675,16 +675,6 @@ parameters:
count: 1
path: ../../typo3/sysext/core/Classes/Http/RedirectResponse.php
- -
- message: "#^Result of && is always false\\.$#"
- count: 2
- path: ../../typo3/sysext/core/Classes/Http/Request.php
-
- -
- message: "#^Strict comparison using \\!\\=\\= between null and null will always evaluate to false\\.$#"
- count: 1
- path: ../../typo3/sysext/core/Classes/Http/Request.php
-
-
message: "#^Call to function is_resource\\(\\) with Psr\\\\Http\\\\Message\\\\StreamInterface will always evaluate to false\\.$#"
count: 1
@@ -1670,16 +1660,6 @@ parameters:
count: 1
path: ../../typo3/sysext/core/Tests/Unit/Html/Parser/ParserTest.php
- -
- message: "#^Call to static method PHPUnit\\\\Framework\\\\Assert\\:\\:assertNull\\(\\) with Psr\\\\Http\\\\Message\\\\UriInterface will always evaluate to false\\.$#"
- count: 1
- path: ../../typo3/sysext/core/Tests/Unit/Http/RequestTest.php
-
- -
- message: "#^Parameter \\#1 \\$uri of class TYPO3\\\\CMS\\\\Core\\\\Http\\\\Request constructor expects Psr\\\\Http\\\\Message\\\\UriInterface\\|string\\|null, array\\<int, string\\> given\\.$#"
- count: 1
- path: ../../typo3/sysext/core/Tests/Unit/Http/RequestTest.php
-
-
message: "#^Parameter \\#1 \\$body of class TYPO3\\\\CMS\\\\Core\\\\Http\\\\Response constructor expects Psr\\\\Http\\\\Message\\\\StreamInterface\\|string\\|null, array\\<int, string\\> given\\.$#"
count: 1
diff --git a/typo3/sysext/core/Classes/Http/Request.php b/typo3/sysext/core/Classes/Http/Request.php
index f4344f65071c..3e3705868ab2 100644
--- a/typo3/sysext/core/Classes/Http/Request.php
+++ b/typo3/sysext/core/Classes/Http/Request.php
@@ -33,23 +33,20 @@ class Request extends Message implements RequestInterface
{
/**
* The request-target, if it has been provided or calculated.
- * @var string|null
*/
- protected $requestTarget;
+ protected ?string $requestTarget = null;
/**
- * The HTTP method, defaults to GET
- *
- * @var string|null
+ * The HTTP method.
*/
- protected $method;
+ protected string $method = 'GET';
/**
* Supported HTTP methods
*
- * @var array
+ * @var non-empty-string[]
*/
- protected $supportedMethods = [
+ protected array $supportedMethods = [
'CONNECT',
'DELETE',
'GET',
@@ -75,44 +72,42 @@ class Request extends Message implements RequestInterface
/**
* An instance of the Uri object
- * @var UriInterface|null
+ *
+ * @todo It is a PSR-7 spec violation for this to be null. This should be corrected.
*/
- protected $uri;
+ protected ?UriInterface $uri;
/**
* Constructor, the only place to set all parameters of this Request
*
* @param string|UriInterface|null $uri URI for the request, if any.
- * @param string|null $method HTTP method for the request, if any.
+ * @param string $method HTTP method for the request, if any.
* @param string|resource|StreamInterface|null $body Message body, if any.
* @param array $headers Headers for the message, if any.
* @throws \InvalidArgumentException for any invalid value.
*/
- public function __construct($uri = null, $method = null, $body = 'php://input', array $headers = [])
+ public function __construct(UriInterface|string|null $uri = null, string $method = 'GET', $body = 'php://input', array $headers = [])
{
-
- // Build a streamable object for the body
- if ($body !== null && !is_string($body) && !is_resource($body) && !$body instanceof StreamInterface) {
- throw new \InvalidArgumentException('Body must be a string stream resource identifier, a stream resource, or a StreamInterface instance', 1436717271);
- }
-
- if ($body !== null && !$body instanceof StreamInterface) {
- $body = new Stream($body);
+ // Upcast the body to a streamable object, or error
+ // if it's an invalid type.
+ if ($body instanceof StreamInterface) {
+ $this->body = $body;
+ } else {
+ $this->body = match (get_debug_type($body)) {
+ 'string', 'resource' => new Stream($body),
+ 'null' => null,
+ default => throw new \InvalidArgumentException('Body must be a string stream resource identifier, a stream resource, or a StreamInterface instance', 1436717271),
+ };
}
if (is_string($uri)) {
$uri = new Uri($uri);
}
- if (!$uri instanceof UriInterface && $uri !== null) {
- throw new \InvalidArgumentException('Invalid URI provided; must be null, a string, or a UriInterface instance', 1436717272);
- }
-
$this->validateMethod($method);
$this->method = $method;
$this->uri = $uri;
- $this->body = $body;
[$this->lowercasedHeaderNames, $headers] = $this->filterHeaders($headers);
$this->assertHeaders($headers);
$this->headers = $headers;
@@ -143,10 +138,10 @@ class Request extends Message implements RequestInterface
* key MUST be a header name, and each value MUST be an array of strings
* for that header.
*/
- public function getHeaders()
+ public function getHeaders(): array
{
$headers = parent::getHeaders();
- if (!$this->hasHeader('host') && ($this->uri && $this->uri->getHost())) {
+ if (!$this->hasHeader('host') && ($this->uri?->getHost())) {
$headers['host'] = [$this->getHostFromUri()];
}
return $headers;
@@ -166,9 +161,9 @@ class Request extends Message implements RequestInterface
* header. If the header does not appear in the message, this method MUST
* return an empty array.
*/
- public function getHeader($header)
+ public function getHeader($header): array
{
- if (!$this->hasHeader($header) && strtolower($header) === 'host' && ($this->uri && $this->uri->getHost())) {
+ if (!$this->hasHeader($header) && strtolower($header) === 'host' && ($this->uri?->getHost())) {
return [$this->getHostFromUri()];
}
return parent::getHeader($header);
@@ -176,10 +171,8 @@ class Request extends Message implements RequestInterface
/**
* Retrieve the host from the URI instance
- *
- * @return string
*/
- protected function getHostFromUri()
+ protected function getHostFromUri(): string
{
$host = $this->uri->getHost();
$host .= $this->uri->getPort() ? ':' . $this->uri->getPort() : '';
@@ -199,17 +192,17 @@ class Request extends Message implements RequestInterface
*
* If no URI is available, and no request-target has been specifically
* provided, this method MUST return the string "/".
- *
- * @return string
*/
- public function getRequestTarget()
+ public function getRequestTarget(): string
{
if ($this->requestTarget !== null) {
return $this->requestTarget;
}
+
if (!$this->uri) {
return '/';
}
+
$target = $this->uri->getPath();
if ($this->uri->getQuery()) {
@@ -236,11 +229,8 @@ class Request extends Message implements RequestInterface
*
* @link https://tools.ietf.org/html/rfc7230#section-2.7 (for the various
* request-target forms allowed in request messages)
- *
- * @param mixed $requestTarget
- * @return static
*/
- public function withRequestTarget($requestTarget)
+ public function withRequestTarget(mixed $requestTarget): static
{
if (preg_match('#\s#', $requestTarget)) {
throw new \InvalidArgumentException('Invalid request target provided which contains whitespaces.', 1436717273);
@@ -252,12 +242,10 @@ class Request extends Message implements RequestInterface
/**
* Retrieves the HTTP method of the request, defaults to GET
- *
- * @return string Returns the request method.
*/
- public function getMethod()
+ public function getMethod(): string
{
- return !empty($this->method) ? $this->method : 'GET';
+ return $this->method;
}
/**
@@ -275,7 +263,7 @@ class Request extends Message implements RequestInterface
* @return static
* @throws \InvalidArgumentException for invalid HTTP methods.
*/
- public function withMethod($method)
+ public function withMethod($method): static
{
$clonedObject = clone $this;
$clonedObject->method = $method;
@@ -287,11 +275,14 @@ class Request extends Message implements RequestInterface
*
* This method MUST return a UriInterface instance.
*
+ * @todo This method currently may return null instead. This is
+ * a PSR-7 spec violation that should be corrected.
+ *
* @link https://tools.ietf.org/html/rfc3986#section-4.3
- * @return \Psr\Http\Message\UriInterface Returns a UriInterface instance
+ * @return \Psr\Http\Message\UriInterface|null Returns a UriInterface instance
* representing the URI of the request.
*/
- public function getUri()
+ public function getUri(): ?UriInterface
{
return $this->uri;
}
@@ -327,7 +318,7 @@ class Request extends Message implements RequestInterface
* @param bool $preserveHost Preserve the original state of the Host header.
* @return static
*/
- public function withUri(UriInterface $uri, $preserveHost = false)
+ public function withUri(UriInterface $uri, $preserveHost = false): static
{
$clonedObject = clone $this;
$clonedObject->uri = $uri;
@@ -354,20 +345,17 @@ class Request extends Message implements RequestInterface
/**
* Validate the HTTP method, helper function.
*
- * @param string|null $method
* @throws \InvalidArgumentException on invalid HTTP method.
*/
- protected function validateMethod($method)
+ protected function validateMethod(?string $method): void
{
- if ($method !== null) {
- if (!is_string($method)) {
- $methodAsString = get_debug_type($method);
- throw new \InvalidArgumentException('Unsupported HTTP method "' . $methodAsString . '".', 1436717274);
- }
- $method = strtoupper($method);
- if (!in_array($method, $this->supportedMethods, true)) {
- throw new \InvalidArgumentException('Unsupported HTTP method "' . $method . '".', 1436717275);
- }
+ if (is_null($method)) {
+ return;
+ }
+
+ $method = strtoupper($method);
+ if (!in_array($method, $this->supportedMethods, true)) {
+ throw new \InvalidArgumentException('Unsupported HTTP method "' . $method . '".', 1436717275);
}
}
}
diff --git a/typo3/sysext/core/Classes/Http/ServerRequest.php b/typo3/sysext/core/Classes/Http/ServerRequest.php
index 242e3b99f8d5..cc7f1bd6b59a 100644
--- a/typo3/sysext/core/Classes/Http/ServerRequest.php
+++ b/typo3/sysext/core/Classes/Http/ServerRequest.php
@@ -82,7 +82,7 @@ class ServerRequest extends Request implements ServerRequestInterface
$this->validateUploadedFiles($uploadedFiles);
}
- parent::__construct($uri, $method, $body, $headers);
+ parent::__construct($uri, $method ?? 'GET', $body, $headers);
$this->serverParams = $serverParams;
$this->uploadedFiles = $uploadedFiles ?? [];
diff --git a/typo3/sysext/core/Tests/Unit/Http/RequestFactoryTest.php b/typo3/sysext/core/Tests/Unit/Http/RequestFactoryTest.php
index 533216cc6000..53cdf46eedbe 100644
--- a/typo3/sysext/core/Tests/Unit/Http/RequestFactoryTest.php
+++ b/typo3/sysext/core/Tests/Unit/Http/RequestFactoryTest.php
@@ -67,33 +67,6 @@ class RequestFactoryTest extends UnitTestCase
self::assertSame('Foo', $body->__toString());
}
- /**
- * @return array
- */
- public function invalidRequestUriDataProvider(): array
- {
- return [
- 'true' => [true],
- 'false' => [false],
- 'int' => [1],
- 'float' => [1.1],
- 'array' => [['http://example.com']],
- 'stdClass' => [(object)['href' => 'http://example.com']],
- ];
- }
-
- /**
- * @dataProvider invalidRequestUriDataProvider
- * @test
- */
- public function constructorRaisesExceptionForInvalidUri($uri): void
- {
- $this->expectException(\InvalidArgumentException::class);
- $this->expectExceptionCode(1436717272);
- $factory = new RequestFactory(new GuzzleClientFactory());
- $factory->createRequest('GET', $uri);
- }
-
/**
* @test
*/
diff --git a/typo3/sysext/core/Tests/Unit/Http/RequestTest.php b/typo3/sysext/core/Tests/Unit/Http/RequestTest.php
index 5c56dfe63615..32ffe2498db8 100644
--- a/typo3/sysext/core/Tests/Unit/Http/RequestTest.php
+++ b/typo3/sysext/core/Tests/Unit/Http/RequestTest.php
@@ -63,15 +63,6 @@ class RequestTest extends UnitTestCase
self::assertNull($this->request->getUri());
}
- /**
- * @test
- */
- public function constructorRaisesExceptionForInvalidStream(): void
- {
- $this->expectException(\InvalidArgumentException::class);
- new Request(['TOTALLY INVALID']);
- }
-
/**
* @test
*/
@@ -112,58 +103,6 @@ class RequestTest extends UnitTestCase
}
}
- /**
- * @return array
- */
- public function invalidRequestUriDataProvider(): array
- {
- return [
- 'true' => [true],
- 'false' => [false],
- 'int' => [1],
- 'float' => [1.1],
- 'array' => [['http://example.com']],
- 'stdClass' => [(object)['href' => 'http://example.com']],
- ];
- }
-
- /**
- * @dataProvider invalidRequestUriDataProvider
- * @test
- */
- public function constructorRaisesExceptionForInvalidUri($uri): void
- {
- $this->expectException(\InvalidArgumentException::class);
- $this->expectExceptionCode(1436717272);
- new Request($uri);
- }
-
- /**
- * @return array
- */
- public function invalidRequestMethodDataProvider(): array
- {
- return [
- 'true' => [true],
- 'false' => [false],
- 'int' => [1],
- 'float' => [1.1],
- 'array' => [['POST']],
- 'stdClass' => [(object)['method' => 'POST']],
- ];
- }
-
- /**
- * @dataProvider invalidRequestMethodDataProvider
- * @test
- */
- public function constructorRaisesExceptionForInvalidMethodByType($method): void
- {
- $this->expectException(\InvalidArgumentException::class);
- $this->expectExceptionCode(1436717274);
- new Request(null, $method);
- }
-
/**
* @test
*/
@@ -197,7 +136,7 @@ class RequestTest extends UnitTestCase
{
$this->expectException(\InvalidArgumentException::class);
$this->expectExceptionCode(1436717271);
- new Request(null, null, $body);
+ new Request(null, 'GET', $body);
}
/**
@@ -219,7 +158,7 @@ class RequestTest extends UnitTestCase
'x-valid-string' => ['VALID'],
'x-valid-array' => ['VALID'],
];
- $request = new Request(null, null, 'php://memory', $headers);
+ $request = new Request(null, 'GET', 'php://memory', $headers);
self::assertEquals($expected, $request->getHeaders());
}
@@ -492,7 +431,7 @@ class RequestTest extends UnitTestCase
*/
public function headerCanBeRetrieved($header, $value, $expected): void
{
- $request = new Request(null, null, 'php://memory', [$header => $value]);
+ $request = new Request(null, 'GET', 'php://memory', [$header => $value]);
self::assertEquals([$expected], $request->getHeader(strtolower($header)));
self::assertEquals([$expected], $request->getHeader(strtoupper($header)));
}
@@ -525,7 +464,7 @@ class RequestTest extends UnitTestCase
public function constructorRaisesExceptionForHeadersWithCRLFVectors($name, $value): void
{
$this->expectException(\InvalidArgumentException::class);
- new Request(null, null, 'php://memory', [$name => $value]);
+ new Request(null, 'GET', 'php://memory', [$name => $value]);
}
/**
diff --git a/typo3/sysext/core/Tests/Unit/Http/ServerRequestFactoryTest.php b/typo3/sysext/core/Tests/Unit/Http/ServerRequestFactoryTest.php
index 123fd1ef59c7..139bf1e8d700 100644
--- a/typo3/sysext/core/Tests/Unit/Http/ServerRequestFactoryTest.php
+++ b/typo3/sysext/core/Tests/Unit/Http/ServerRequestFactoryTest.php
@@ -68,33 +68,6 @@ class ServerRequestFactoryTest extends UnitTestCase
self::assertSame('Foo', $body->__toString());
}
- /**
- * @return array
- */
- public function invalidRequestUriDataProvider(): array
- {
- return [
- 'true' => [true],
- 'false' => [false],
- 'int' => [1],
- 'float' => [1.1],
- 'array' => [['http://example.com']],
- 'stdClass' => [(object)['href' => 'http://example.com']],
- ];
- }
-
- /**
- * @dataProvider invalidRequestUriDataProvider
- * @test
- */
- public function constructorRaisesExceptionForInvalidUri($uri): void
- {
- $this->expectException(\InvalidArgumentException::class);
- $this->expectExceptionCode(1436717272);
- $factory = new ServerRequestFactory();
- $factory->createServerRequest('GET', $uri);
- }
-
/**
* @test
*/
--
GitLab