Upgrade PHPUnit to 10.x. Fix Stream conformance

[gsteel]

Q	A
QA	yes

[gsteel]

This uses 1.x-dev of php-http/psr7-integration-tests in order to install PHPUnit 10.

There's no release there yet: php-http/psr7-integration-tests#76

Some failing tests need attention…

[froschdesign]

The release is here: https://github.com/php-http/psr7-integration-tests/releases/tag/1.4.0

[gsteel]

So we can't upgrade to PHPUnit 10 without first resolving BC issues around numeric header names, and we won't be able to support PHP 8.4 without upgrading to PHPUnit 10.

This patch is a BC break (probably) because it casts header names to string prior to validation on the assumption that numeric keys have already been cast to int by PHP.

Paging @laminas/technical-steering-committee for input…

Do we need to release a major here?

[internalsystemerror]

My thoughts are, BC break = Major version bump. What would be the burden/implications from this?

[Xerkus]

Can you elaborate? Numeric headers can not be handled by the library interface and it can not be resolved.
See #159

[weierophinney]

Agreed with @Xerkus - header names cannot be purely numeric (and this not integers). If anything, this feels like resolving a potential error for users, and not introducing a break.

[gsteel]

It appears to me that tests no longer pass with the previous behaviour of throwing an exception for integer header names when creating the request from globals.

To get the integration tests to pass, I've cast numerics to string in the appropriate place, but this is the BC break - currently, users expect integer header names to be exceptional and this patch makes them valid as per RFC whatever.

It doesn't affect the existing issue that withHeader('123') will yield int 123 from getHeaders

[gsteel]

Agreed with @Xerkus - header names cannot be purely numeric (and this not integers). If anything, this feels like resolving a potential error for users, and not introducing a break.

@weierophinney OK - maybe I'm a bit confused here - I thought that numeric header names were valid according to RFC 7230 ??

[Xerkus]

Please see the issue I linked and issues it refers to. We can not allow numeric headers.

[Xerkus]

Please remove type casting for headers from the PR. It hides the issue for the current test implementation.

[gsteel]

@Xerkus - I'm going to go back to the drawing board on this patch to identify properly where existing test cases are failing.

[Xerkus]

@gsteel for context, we filtered out numeric headers from globals when request is created but did not forbid numeric headers in the headers security check because @weierophinney wanted to bring it to PHP-FIG first.

There are numeric headers in the wild from a random http client as evidenced by the original report but we couldn't identify any valid use case for them. Strictly speaking, unregistered headers not prefixed with x- are not RFC conforming. Hence why we should not try to support them just because they fit ABNF.

[gsteel]

Thanks @Xerkus and @weierophinney

So it turns out that I added some failing tests around this during the upgrade to PHPUnit 10. Whilst I was waiting for the integration tests to get released, I forgot I'd added them and assumed they were new failures.

Generally speaking, I'm just a bit of an idiot!!

Sorry for wasting everyone's time - this should be a straight-forward review now.

[akrabat]

Strictly speaking, unregistered headers not prefixed with x- are not RFC conforming.

This is no longer true with RFC9110, where Section 16.3.2.1 states:

Field names ought not be prefixed with "X-"

Numeric only field names are discouraged, but not illegal:

While the field-name syntax is defined to allow any token character, in practice some implementations place limits on the characters they accept in field-names. To be interoperable, new field names SHOULD constrain themselves to alphanumeric characters, "-", and ".", and SHOULD begin with a letter. For example, the underscore ("_") character can be problematic when passed through non-HTTP gateway interfaces (see Section 17.10).¶

[Xerkus]

This is no longer true with RFC9110, where Section 16.3.2.1 states:

I was not aware we got newer RFC on this. Thank you.

[Xerkus]

I wonder if this CDATA thrashing in the baseline is due to different xml libraries available when baseline is updated.

[gsteel]

Maybe - I have libxml 2.9.13 locally for PHP 8.1

[Xerkus]

@gsteel Thank you