fix: CURLRequest - clear response headers between requests

[michalsn]

Description
Fixes #7394

• create a new Response object for a request

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[iRedds]

I believe that for each request, a new response instance should be created, and not passed as a dependency.

[kenjis]

I can still understand sharing request options, but I don't believe there is a use case for sharing and overriding response objects.

[michalsn]

I believe that for each request, a new response instance should be created, and not passed as a dependency.

The idea was to allow the use of Response classes other than the default one.

I can still understand sharing request options, but I don't believe there is a use case for sharing and overriding response objects.

Existing headers are now always removed before collecting the new response headers.

[kenjis]

Tests were failed. Please rebase.

[kenjis]

I believe that for each request, a new response instance should be created, and not passed as a dependency.

I agree.

The idea was to allow the use of Response classes other than the default one.

I don't understand why do we need to pass a Response object.

I think it is not correct to share the same Response object between requests.
Because they are the different responses.

This is not a bit clean, but I think better:

--- a/system/HTTP/CURLRequest.php
+++ b/system/HTTP/CURLRequest.php
@@ -137,7 +137,12 @@ class CURLRequest extends OutgoingRequest
             $this->resetOptions();
         }
 
-        return $this->response;
+        $response = $this->response;
+
+        // Set new Response for the next request.
+        $this->response = new Response(config('App'));
+
+        return $response;
     }
 
     /**

[kenjis]

If possible, I think this (and deprecate $response in the constructor) is better:

--- a/system/HTTP/CURLRequest.php
+++ b/system/HTTP/CURLRequest.php
@@ -125,6 +125,8 @@ class CURLRequest extends OutgoingRequest
      */
     public function request($method, string $url, array $options = []): ResponseInterface
     {
+        $this->response = new Response(config('App'));
+
         $this->parseOptions($options);
 
         $url = $this->prepareURL($url);

[michalsn]

Returning a new response class for every CURL request would mean we tightly couple the response with the CodeIgniter Response class.

For me, that would be a regression in extensibility. Of course, "resetting" headers would be a compromise, but there is something we get in return.

The question is, do we want to offer fewer options and introduce a BC break?

[iRedds]

@michalsn I don't know the real situation, but it seems to me that 90% of CI developers will use the classes that are available.
The remaining 10% will not experience problems with class extensions.

But for example, I want to give the code. And I have not seen angry comments about the impossibility of replacing the class.

CodeIgniter4/system/HTTP/IncomingRequest.php

Lines 904 to 911 in 354dc67

	public function getFiles(): array
	{
	if ($this->files === null) {
	$this->files = new FileCollection();
	}
	return $this->files->all(); // return all files
	}

CodeIgniter4/system/HTTP/Files/FileCollection.php

Lines 164 to 187 in 354dc67

	protected function createFileObject(array $array)
	{
	if (! isset($array['name'])) {
	$output = [];
	foreach ($array as $key => $values) {
	if (! is_array($values)) {
	continue;
	}
	$output[$key] = $this->createFileObject($values);
	}
	return $output;
	}
	return new UploadedFile(
	$array['tmp_name'] ?? null,
	$array['name'] ?? null,
	$array['type'] ?? null,
	$array['size'] ?? null,
	$array['error'] ?? null
	);
	}

[kenjis]

We should always avoid BC breaks especially in patch versions.

Then how about something like this:

--- a/system/HTTP/CURLRequest.php
+++ b/system/HTTP/CURLRequest.php
@@ -125,6 +125,8 @@ class CURLRequest extends OutgoingRequest
      */
     public function request($method, string $url, array $options = []): ResponseInterface
     {
+        $this->response = clone $this->responseOrig;
+
         $this->parseOptions($options);
 
         $url = $this->prepareURL($url);

[kenjis]

I don't see a use case for changing the Response class.
Also, if you want to change the return value (ResponseInterface), you need to extend CURLRequest, and I think you can change the return type there.

[michalsn]

@iRedds I don't know if anybody is using other response classes than the default one, but ~5 years ago, I would take advantage of this approach in one project.

The argument that framework already has places where things are tightly coupled doesn't speak to me. Just because we don't give everywhere a choice about what class we will use doesn't mean we should "level down" and take that choice away from the places where we have it now.

@kenjis I see the added value in replacing the default class in some cases. A long time ago I had a project where I would extend the Response class to simplify handling the response headers. I was consuming a really specific API.

I like your suggestion with clone, and this is a good option.

---

The bottom line is that I allow the thought that keeping the support for the custom Response class is just my fixation.

Since dropping the support would involve a BC break, I would love to hear from other members. @codeigniter4/core-team any thoughts on this one?

[iRedds]

@michalsn I can offer two alternatives.

1. Instead of a Response object, pass a factory through an interface.

public function __construct(protected CURLFactoryInterface $factory, protected array $options = []){}

new CURLRequest(new CURLFactory(), $options)

// Now we can get a new Response object.
$this->factory->response();

2. You can also simply add a method directly in the CURLRequest class, which will return the object we need.

protected function buildResponse(): ResponseInterface
{
    return new Response(config('App'));
}

[kenjis]

@michalsn Okay, you had the real use cases. So removing injection of ResponseInterface is degrade and a breaking change. We should use clone now.

Anyway, it is unacceptable to share only one Response object; if you send a request twice, the first response changes. It is a terrible bug.

[michalsn]

@iRedds These are good options. I especially like the second one, but both would involve BC changes. That's why I send a change with clone version.

@kenjis Yes, I agree. The new response class for every request will be much better.

Thank you both for the input. Much appreciate.

[kenjis]

$this->responseOrig may be null.

<?php

namespace App\Controllers;

use CodeIgniter\HTTP\CURLRequest;
use CodeIgniter\HTTP\URI;
use Config\App;

class Home extends BaseController
{
    public function index()
    {
        $client = new CURLRequest(new App(), new URI());
        $client->get('https://codeigniter.com/');
    }
}

Error
__clone method called on non-object

SYSTEMPATH/HTTP/CURLRequest.php at line 135

[michalsn]

Good point. Fixed.