amphp / http

HTTP primitives which can be shared by servers and clients.
https://amphp.org/http
MIT License
88 stars 10 forks source link

Add header with valid types #20

Closed PNixx closed 1 year ago

PNixx commented 1 year ago

I catch error when add header with timestamp:

$request->setHeaders(array_filter([
    'apns-topic'      => $topic,
    'apns-expiration' => time() + $ttl,
]));

It's very stupid to make sure that all data types match the string.

Bilge commented 1 year ago

It's not stupid if string is the correct type (it is).

bwoebi commented 1 year ago

The problem is not the signature of setHeader(), but the fact that the file uses strict_types, which causes setHeaders() to forward it using strict typing rules...

PNixx commented 1 year ago

You're already casting to a string https://github.com/amphp/http/blob/98735395846c26b60a880815f65103621f85b534/src/HttpMessage.php#L148, so both strings and numbers must be available as input.

trowski commented 1 year ago

HTTP headers are meant to be strings. It is not stupid to expect strings to be passed.

If you'd like to make the argument that integers and floats should also be allowed, I'm willing to entertain that idea. Note that they must be converted to strings before being added to the array of headers.

This PR is incomplete without modifying the signatures of other methods and value handling within the method. The one you did modify, addHeader, doesn't address the issue you provided.

trowski commented 1 year ago

Please update the docblock types too. I'm surprised Psalm isn't failing due to the mis-match.

PNixx commented 1 year ago

i understand what is a docblock

trowski commented 1 year ago

Docblock refers to the comments which start with /**. These are used to provide type annotations that we cannot expression in PHP code directly, for example array<string>, typing the array should only contain strings.

PNixx commented 1 year ago

I think, need slit validation with array and string https://github.com/amphp/http/blob/2.x/src/HttpMessage.php#L171. This optimizes validation without iterating through the array https://github.com/amphp/http/blob/2.x/src/HttpMessage.php#L224-L228

trowski commented 1 year ago

Close enough, I'll clean it up in another commit. Thanks!

PNixx commented 1 year ago

@trowski why you revert support int|float??? $headers['Content-Length'] = 123 - always set in int number. All libs! My code is die with v2.0.0-beta.1.

kelunik commented 1 year ago

Because the signature should really be array|string, because that's the correct type for headers. There's still some support built-in for casting ints and floats to strings.

PNixx commented 1 year ago

I opened issue https://github.com/amphp/http/issues/22 for this question