Replace instead of appending to the default header #4550
Conversation
There was a problem hiding this comment.
This is generally correct I think, but I'd like you to trace the history of the addValue implementation. There may be a specific reason it was written like this. Additionally, this change should be tested so that we can both verify the behavior and ensure future changes don't break it.
|
Setting headers per request is introduced from #4113 . At first I also doubted whether there were special reasons to use |
|
Sorry, tests need to be added as part of this PR, and really any code change going forward. It's the only way to ensure the integrity of the library. Also, would you mind summarizing the issue this fixes? |
|
Tests are added. "later" I meant is now. 😄 I was going out. |
Do you mean updating the changelog file? Sorry I'm afraid I can't cause I'm not good at English :( |
|
It might be: Changes
|
No, I just meant that you should fill out the PR template so that everyone can see why you're making a change and how you did so. Otherwise there might be a lot of unnecessary questions during review. |
* Replace instead of appending to the default header * Add tests for setting HTTP headers per request
* Fix serverTrust crash (#4542) * Update readme, add stale bot, add issue template, add pull request template * Update copy from Alamofire * Fix crash on nil serverTrust * Expose server trust error * add test for error throws * Fix nullability * Fix evaluateServerTrust determination (#4552) Co-authored-by: Jon Shier <jon@jonshier.com> * Revert changes making error generation public. This reverts commit 111352b. # Conflicts: # AFNetworking/AFURLSessionManager.m * Make static error function a method instead. * Make trust generatio part of AFTestCase. * Add test cases for server trust error generation. * Mark parameters explicitly nullable. * Fix nullability (#4551) * Replace instead of appending to the default header (#4550) * Replace instead of appending to the default header * Add tests for setting HTTP headers per request * Fix SPM usage with better publicHeadersPath. (#4554) Co-authored-by: Jakub Kašpar <kaspikk@gmail.com> Co-authored-by: Elf Sundae <elf.sundae@gmail.com>
|
PR description was updated. |
* commit '77ef5fed64d98107acd177a90182163a20ba4567': (31 commits) Use low priority queue instead of background for SCNetworkReachabilityGetFlags() (AFNetworking#4587) Exempt "needs investigation" from stalebot. Only mark unlabeled issues as stale. Loop over only changed keypaths in request serializer (AFNetworking#4581) Disable 'Zombie Objects' diagnostics for tvOS scheme (AFNetworking#4572) Fix docblock for setAuthenticationChallengeHandler (AFNetworking#4574) Fix type in CHANGELOG. (AFNetworking#4562) Prepare 4.0.1 Release (AFNetworking#4555) Fix ServerTrustError crash. (AFNetworking#4553) Fix SPM usage with better publicHeadersPath. (AFNetworking#4554) Replace instead of appending to the default header (AFNetworking#4550) Fix nullability (AFNetworking#4551) Update CHANGELOG.md (AFNetworking#4537) Fix UIKit+AFNetworking.h (AFNetworking#4536) Remove unused UIImage+AFNetworking.h (AFNetworking#4535) Separate bundle identifier for watchOS target (AFNetworking#4533) Fix MobileCoreServices renamed warning, close #4520 (AFNetworking#4532) Add FOUNDATION_EXPORT for AFJSONObjectByRemovingKeysWithNullValues (AFNetworking#4529) (Infra) Make infra changes to cleanup project and simplify its maintenance (AFNetworking#4531) Improve podspec (AFNetworking#4528) ... # Conflicts: # Framework/AFNetworking.h
Issue Link 🔗
https://github.com/AFNetworking/AFNetworking/issues/4549
Goals ⚽
Currently,
AFHTTPSessionManageruses-addValue:forHTTPHeaderField:to add HTTP headers to an individual request, that will append the value to the existing field specified in the request serializer.In general, we prefer to replace the default value, instead of appending things to the default value with a comma, right?
Implementation Details 🚧
This PR changes
-addValue:forHTTPHeaderField:to-setValue:forHTTPHeaderField:. In my opinion, using-setValue:is more reasonable and clearer.Testing Details 🔍
Tests were added to verify setting HTTP headers per request will replace the default value.