Do not add the same tag multiple times #423
Conversation
There was a problem hiding this comment.
i think when we wrote this, we assumed that the application would not add tags multiple times, but now that i think about it, thats not a good assumption as it could make taging a lot more complicated in flexible and modular systems.
thanks for the pull request.
| $headerFormatter = new CommaSeparatedTagHeaderFormatter('FOS-Tags'); | ||
|
|
||
| $tagHandler = new ResponseTagger(['header_formatter' => $headerFormatter, 'strict' => true]); | ||
| $tagHandler->addTags(['post-1', 'post-2', 'post-1']); |
There was a problem hiding this comment.
for the sake of completeness, i'd like to have another line here that does $tagHandler->addTags(['post-2', 'post-3']);. with the current test, array_merge($this->tags, array_unique($filtered)) would pass but its not correct.
| @@ -123,7 +123,7 @@ public function addTags(array $tags) | |||
| throw new InvalidTagException('Empty tags are not allowed'); | |||
| } | |||
|
|
|||
| $this->tags = array_merge($this->tags, $filtered); | |||
| $this->tags = array_unique(array_merge($this->tags, $filtered)); | |||
There was a problem hiding this comment.
i am pondering whether we should do the unique only at the end when building the tags header. not sure whats better performance - doing unique only at the end in getTagsHeaderValue would add less overhead for people who's system is not adding duplicate tags. on the other hand, a system with massive duplicates would profit from a rolling de-duplication. also depends on whether addTags is called a lot or only a few times... wdyt?
There was a problem hiding this comment.
I asked myself the same question but in the end I guess it's such a minor thing, nobody will ever notice 😄 I put it into the addTags() because there's already some filtering going on there. I just figured it would be better to add it there so it's all in one place. But again, at the end of the day it doesn't really matter I think 😊
|
can you please add the changelog entry? otherwise this looks good to me |
|
please rebase on master before adding the changelog to avoid a merge conflict ;-) |
|
Done. |
|
i tagged the changes as 2.4.0 thanks a lot for the contributions! |
|
Yeyy, thank you!!! |
No description provided.