Add missing checks in patch request [Fixes #96] #98
Conversation
DrudgeRajen
changed the title
src/Tus/Client.php
Outdated
| @@ -452,6 +452,14 @@ protected function sendPatchRequest(int $bytes, int $offset) : int | |||
| throw new ConnectionException('Connection aborted by user.'); | |||
| } | |||
|
|
|||
| if (HttpResponse::HTTP_UNSUPPORTED_MEDIA_TYPE == $statusCode) { | |||
There was a problem hiding this comment.
use strict type check please ===
src/Tus/Client.php
Outdated
| @@ -452,6 +452,14 @@ protected function sendPatchRequest(int $bytes, int $offset) : int | |||
| throw new ConnectionException('Connection aborted by user.'); | |||
| } | |||
|
|
|||
| if (HttpResponse::HTTP_UNSUPPORTED_MEDIA_TYPE == $statusCode) { | |||
| throw new ConnectionException('Unsupported media Types.'); | |||
There was a problem hiding this comment.
it is not a connection exception. Can we use TusPhp\Exception please.
src/Tus/Client.php
Outdated
| } | ||
|
|
||
| if (HttpResponse::HTTP_BAD_REQUEST === $statusCode) { | ||
| throw new ConnectionException('Bad request.'); |
There was a problem hiding this comment.
same as above, not a connection exception
src/Tus/Server.php
Outdated
| @@ -32,6 +32,9 @@ class Server extends AbstractTus | |||
| /** @const string Tus Concatenation Extension */ | |||
| const TUS_EXTENSION_CONCATENATION = 'concatenation'; | |||
|
|
|||
| /** @const string Tus Content Type Extension */ | |||
| const HEADER_CONTENT_TYPE = 'application/offset+octet-stream'; | |||
There was a problem hiding this comment.
can we move this to AbstractTus class and use it in Client#L428 and Server#L464
src/Tus/Server.php
Outdated
| $contentType = $this->request->header('Content-Type'); | ||
| $contentLength = $this->request->header('Content-Length'); | ||
|
|
||
| if ( ! $contentType || $contentType !== self::HEADER_CONTENT_TYPE) { |
There was a problem hiding this comment.
first check ! $contentType is not necessary, second check will also be false in case $contentType is null or empty?
tests/Tus/ServerTest.php
Outdated
| ->getRequest() | ||
| ->getRequest() | ||
| ->headers | ||
| ->set('Content-Length', 200); |
There was a problem hiding this comment.
instead of 2 set statements, we can use single add method to set headers at once
$this->tusServerMock
->getRequest()
->getRequest()
->headers
->add([
'Content-Type' => 'application/offset+octet-stream',
'Content-Length' => 200,
]);
tests/Tus/ServerTest.php
Outdated
| ->getRequest() | ||
| ->getRequest() | ||
| ->headers | ||
| ->set('Content-Length', 200); |
tests/Tus/ServerTest.php
Outdated
| ->getRequest() | ||
| ->getRequest() | ||
| ->headers | ||
| ->set('Content-Length', 200); |
|
@DrudgeRajen second point seems invalid now: https://github.com/tus/tus-resumable-upload-protocol/pull/137/files |
| @@ -454,6 +454,10 @@ protected function sendPatchRequest(int $bytes, int $offset) : int | |||
| throw new ConnectionException('Connection aborted by user.'); | |||
| } | |||
|
|
|||
| if (HttpResponse::HTTP_UNSUPPORTED_MEDIA_TYPE === $statusCode) { | |||
| throw new Exception('Unsupported media Types.'); | |||
There was a problem hiding this comment.
I thought it was going to be changed to TusException?
There was a problem hiding this comment.
It is already from Tus\Exception namespace, we just need to rename it to TusException to avoid confusion
ankitpokhrel
changed the title
All PATCH requests MUST use Content-Type: application/offset+octet-stream, otherwise the server SHOULD return a 415 Unsupported Media Type status.
If a PATCH request does not include a Content-Length header containing an integer value larger than 0, the server SHOULD return a 400 Bad Request status.Test Coverage