Fix CURLRequest keeping the last value provided by headers with the same name

[NicolaeIotu]

Fixes #5041
CURLRequest can now get and forward all headers including multiple headers with the same name

Description
Basic minimal modifications to ensure multiple headers with the same name are correctly received and forwarded.

[NicolaeIotu]

Fix Issue #5041 - Bug: CURLRequest keeps the last value provided by headers with the same name

[kenjis]

@NicolaeIotu Thank you for your PR!

We ask that contributions have code commits signed. Could you sign?
https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/signing.md

And there is no test code in your PR. Can you add test code?
https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#unit-testing
https://github.com/codeigniter4/CodeIgniter4/blob/develop/tests/README.md

[MGatner]

Please address kenjis comments. I will also add that setHeader() already appends values for existing names, with the caveat that it is an array. MessageTrait.php:

        if (isset($this->headers[$origName]) && is_array($this->headers[$origName]->getValue())) {
            if (! is_array($value)) {
                $value = [$value];
            }

            foreach ($value as $v) {
                $this->appendHeader($origName, $v);
            }
        } else {
            $this->headers[$origName]               = new Header($origName, $value);
            $this->headerMap[strtolower($origName)] = $origName;
        }

Likewise getValueLine() will handle multiple header values, concatenating them into a header string which I believe is the appropriate way to handle, rather then sending multiple header() commands with the same name.

Can you give an example of where this is not working?

[NicolaeIotu]

Hi guys,
I suspected that something else was wrong somewhere else, but since I'm not used to PHP and CodeIgniter, I just could not go into far enough.
If we take a close look only at the snippet sent by @MGatner we can deduct that the condition
if (isset($this->headers[$origName]) && is_array($this->headers[$origName]->getValue())) { ...
it is never true because $this->headers[$origName]->getValue() becomes an array only after the condition above becomes true.
In other words, the snippet would function properly if $this->headers[$origName]>getValue() becomes an array somewhere else.
That is exactly what my fix was doing in CURLRequest.php at protected function parseOptions(array $options) ...

Having deducted that, it seems that it is probably better to modify MessageTrait.php and eliminate && is_array($this->headers[$origName]->getValue()) and keep file CURLRequest.php intact (eliminate my modification).
Unless proven wrong, the modification of public function sendHeaders().. in ResponseTrait.php seems appropriate.

My kind suggestion is that somebody else takes over and fixes this issue in a brand new commit because I'm not familiar with the framework and honestly I have to spend some time figuring out how to do tests in PHP/CodeIgniter.

[MGatner]

@NicolaeIotu thank you for your analysis and contribution! Your grasp of the language and framework would not have led me to believe you were new, so kudos on that.

I understand what you're saying but you think there is a presupposition here that "some headers are multiple values and some are not". Notice that MessageTrait::setHeader() supports array values:

    /**
     * Sets a header and it's value.
     *
     * @param array|string|null $value
     *
     * @return $this
     */
    public function setHeader(string $name, $value): self

I believe the intent is that any header you want to have multiple values you pass in an array (for CURL requests this might be like $options['headers'] = ['foo' => ['bar', 'bam']];) and anything else would override an existing "flat" value instead of promoting it to an array.

I'm glad to hear a case otherwise, but that is my understanding of the current functionality.

[MGatner]

UPDATE:
Actually, the framework handles this fine. It hinges on the "multiple values" concatenation in getValueLine(). From the same doc in my comment below:

// Multiple attributes are also possible, for example:
Set-Cookie: <cookie-name>=<cookie-value>; Domain=<domain-value>; Secure; HttpOnly

So my original comment stands: you need to pass any multiple headers values as arrays.

DISREGARD BELOW (left for reference)

---

Out of curiosity, I went and looked which header you were actually trying to repeat (set-cookie) and did some investigation and found this (emphasis added):

The Set-Cookie HTTP response header is used to send a cookie from the server to the user agent, so that the user agent can send it back to the server later. To send multiple cookies, multiple Set-Cookie headers should be sent in the same response.

So indeed, you are correct about how this should function but my description of header handling above is accurate. Basically: the framework does not support multiple headers with the same name, but should.

This is a much bigger change than what we are discussing here, so I will create an issue for it and the team will have to look into the implications. If you feel like tackling it feel free to send a PR, but be mindful that we cannot implement breaking changes.

[NicolaeIotu]

@MGatner The truth is that CodeIgniter is very well documented. One just have to read the instructions.
In my opinion when consuming web services is not such a rare case to have multiple headers with the same name so it's really not a bad idea to include related changes in future releases. It's not just Set-Cookie header that can have multiple values.
Anyways I'm glad if I could help. It's the least I could do at my level.
I guess the issue will be closed by you guys at some point. If not just let me know and I will.

[kenjis]

@MGatner A header having multiple values, multiple headers with the same name. They are different.
I think the following your comment is correct.

the framework does not support multiple headers with the same name, but should.

See #5041 (comment)
At least we cannot get all the cookie values with CURLRequest.

[kenjis]

I sent PR #8194

[NicolaeIotu]

Awesome! Much appreciated