Skip to content

Use isset over array_key_exists #48

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Use isset over array_key_exists #48

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0596b87
Use isset over array_key_exists
gmponos Nov 27, 2018
0e02605
Fix assertion
gmponos Nov 27, 2018
2ae49a4
Fix test with lowest dep
gmponos Nov 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/Client.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<?php

declare(strict_types=1);
namespace Http\Client\Curl;

use Http\Client\Exception;
Expand Down Expand Up @@ -330,10 +330,10 @@ private function createHeaders(RequestInterface $request, array $options)
continue;
}
if ('content-length' === $header) {
if (array_key_exists(CURLOPT_POSTFIELDS, $options)) {
if (isset($options[CURLOPT_POSTFIELDS])) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the problem i have with isset is that it hides programming errors, like when $options is mistyped or something.

also, to spin your idea further, somebody could set the option to 42 or some object, and then we still fail.

i would prefer to have a check inside the if that throws an InvalidArgumentException when !is_string.

// Small body content length can be calculated here.
$values = [strlen($options[CURLOPT_POSTFIELDS])];
} elseif (!array_key_exists(CURLOPT_READFUNCTION, $options)) {
} elseif (!isset($options[CURLOPT_READFUNCTION])) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we do not use the option here but only check for existence of the option. i'd not change this here.

we define this field as a function:

curl-client/src/Client.php

Line 293 in 2ae49a4

$options[CURLOPT_READFUNCTION] = function ($ch, $fd, $length) use ($body) {

i would be ok to add an is_callable check to throw an InvalidArgumentException if you want.

// Else if there is no body, forcing "Content-length" to 0
$values = [0];
}
Expand Down
21 changes: 21 additions & 0 deletions tests/ClientTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,27 @@ public function testExpectHeader()
static::assertContains('Expect:', $headers);
}

/**
* "Expect" header should be empty.
*
* @link https://github.com/php-http/curl-client/issues/18
*/
public function testWithNullPostFields()
{
$client = $this->getMockBuilder(Client::class)->disableOriginalConstructor()
->setMethods(['__none__'])->getMock();

$createHeaders = new \ReflectionMethod(Client::class, 'createHeaders');
$createHeaders->setAccessible(true);

$request = new Request();
$request = $request->withHeader('content-length', '0');

$headers = $createHeaders->invoke($client, $request, [CURLOPT_POSTFIELDS => null]);

static::assertContains('content-length: 0', $headers);
}

public function testRewindStream()
{
$client = $this->getMockBuilder(Client::class)->disableOriginalConstructor()
Expand Down