Multiple exclusion strategy #44
Conversation
|
I've just two remarks:
|
|
Ah, and one more, please add a unit test instead of the functional tests. This is far easier here. |
| { | ||
| if (is_string($strategy)) { | ||
| unset($this->chain[$strategy]); | ||
| } elseif (false !== ($index = array_search($strategy, $this->chain, true))) { |
There was a problem hiding this comment.
instead of using array_search, you could simply check if $this->chain[get_class($strategy)] === $strategy, as you are using the class name as key currently
|
About the name, Disjunct is probably too academic. At least I couldn't immediately grasp it. What about "Multiple"? |
|
@Narretz the issue is that there is 2 ways to implement the composed strategy: |
| */ | ||
| class ChainExclusionStrategy implements ExclusionStrategyInterface | ||
| { | ||
| private $chain = array(); |
There was a problem hiding this comment.
@schmittjoh should this use an array or do you prefer using a PhpCollection\SequenceInterface ?
|
Maybe we can borrow a term from Security component here. What about "MultipleAffirmative"? Other than that, Disjunct is fine, if the docs make it clear what it means. |
ChainExclusionStrategy, as first implemented by @adrienbrault in schmittjoh/JMSSerializerBundle#140 and recently brought up again in #39. Strategy must be set explicitly, otherwise the behavior stays the same as currently.