Skip to content
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.

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

[7.x] Add ArrayAccess support for Http client get requests #32401

Merged
merged 1 commit into from
Apr 16, 2020
Merged

[7.x] Add ArrayAccess support for Http client get requests #32401

merged 1 commit into from
Apr 16, 2020

Conversation

dmason30
Copy link
Contributor

@dmason30 dmason30 commented Apr 16, 2020

Fixes #32398.

TL;DR
Fixes an Undefined index: Content-Type exception and allows access to get request params via array access $request['foo'] and in turn allows you to make assertions such as $request['foo'] === 'bar'.

Description:

I was writing a test similar to the below and when running it was greeted with an ErrorException of Undefined index: Content-Type.

public function testGet()
{
    \Http::fake(['*' => \Http::response(['response' =>'Test return data'], 200)]);

    \Http::acceptJson()
        ->get('https://google.com', ['apikey' => 'aaa-bbb-ccc'])
        ->json()

    \Http::assertSent(function (Request $request) {
        $this->assertStringStartsWith('https://google.com', $request->url());
        $this->assertSame('aaa-bbb-ccc', $request['apikey']); // Undefined index: Content-Type
        return true;
    });
}

Cause

Looking at the line causing the error it seems it just needs to check if the key it is searching for exists in the headers array before trying to access it:

Illuminate/Http/Client/Request.php:

public function isForm()
{
    return $this->hasHeader('Content-Type', 'application/x-www-form-urlencoded');
}

public function hasHeader($key, $value = null)
{
    ...
    // Added an Arr::has($headers, $key) here. This ⬇️ was causing the error. 
    return empty(array_diff($value, $this->headers()[$key]));
}

Initial fix & New error found

So I initially set out to fix hasHeader so that it checks the $key exists in the array returned by $this->headers() and I ran the tests and got the same error Undefined index: Content-Type but from a different location:

Illuminate/Http/Client/Request.php:

public function isJson()
{
    return Str::contains($this->header('Content-Type')[0], 'json');
}

public function header($key)
{
    // Caused by this line ⬇️ 
    return $this->headers()[$key];
}

Adding ArrayAccess Support

I then added Arr::get check in the header($key) method and then discovered that there is not any existing support for accessing get query parameters via $request and was met with another Undefined index: foo in the below test:

public function testGetWithArrayQueryParam()
{
    $this->factory->fake();

    $this->factory->get('http://foo.com/get', ['foo' => 'bar']);

    $this->factory->assertSent(function (Request $request) {
        return $request->url() === 'http://foo.com/get?foo=bar'
            && $request['foo'] === 'bar'; // Undefined index: foo
    });
}

So this change gets the query parameters either from $options['query'] or by extracting them from the $url. It handles the below with the tests adjusted to prove it.

Http::get('http://foo.com');
Http::get('http://foo.com?foo=bar');
Http::get('http://foo.com', ['foo' => 'bar']);
Http::get('http://foo.com', 'foo=bar');
Http::get('http://foo.com/get?foo=bar&page=1', ['hello' => 'world']); // Hello world overwrites params
Http::get('http://foo.com', ['foo;bar; space test' => 'laravel']);

Then you can do assertions on the $request data:

Http::assertSent(function (Request $request) {
    return $request->url() === 'http://foo.com/get?foo=bar'
        && $request['foo'] === 'bar';
});

@driesvints
Copy link
Member

@dmason30 probably best to explain why this is needed, not just link to an issue.

@dmason30
Copy link
Contributor Author

@driesvints Thanks done as requested.

@taylorotwell taylorotwell merged commit e737582 into laravel:7.x Apr 16, 2020
@dmason30 dmason30 deleted the http-client-get-array-access branch April 17, 2020 02:28
kieraninnes pushed a commit to tootootltd/azure-text-analytics that referenced this pull request Jun 24, 2020
…se we need ArrayAccess support for Http client in our tests - @see: laravel/framework#32401 for more info
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Http::assertSent on get request causes Undefined Index exception
3 participants