search

amphp / http

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

Message::setHeaders() issue #15

Closed enumag closed 1 year ago

enumag commented 4 years ago

See https://github.com/amphp/http-client/commit/fa7925363e6d5a0d0d337e2e6eb1affb93cf226e for for the problem.

We could rename the method to something more clear like changeHeaders. Words like modify or alter may also be possible.

We can't simply remove it because the method has the added value of being atomic.

By the way we could also make the new method accept iterable instead of array.

Thoughts? @kelunik @trowski @remorhaz

remorhaz commented 4 years ago

The semantic of both methods is like "add/set (each of) headers". The "add" behavior is perfectly obvious, the "set" behavior is indeed a bit misleading, because it's not obvious - if it just sets the internal property "headers" to the argument (like most setter do, and this interpretation led to the defect) or sets each header to given list of values (and this is the way the code works).

I agree that there is a problem. Maybe we can just add new resetHeaders([...]) method, but I'm not sure if this will really help.

remorhaz commented 4 years ago

Another idea is renaming current setHeaders() into replaceHeaders() and making setHeaders() work as simple setter.

kelunik commented 4 years ago

The current method will be deprecated, but not changed in behavior. We really need a good name as replacement.

remorhaz commented 4 years ago

At least we can implement clearHeaders() to ensure removing headers that are already set.