http icon indicating copy to clipboard operation
http copied to clipboard

Message::setHeaders() issue

Open enumag opened this issue 5 years ago • 4 comments

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

enumag avatar Jun 17 '20 17:06 enumag

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 avatar Jun 18 '20 09:06 remorhaz

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

remorhaz avatar Jun 18 '20 14:06 remorhaz

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

kelunik avatar Jun 18 '20 18:06 kelunik

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

remorhaz avatar Jun 19 '20 12:06 remorhaz