👍 (even if it is clearly a micro-optimization)

ping @nicolas-grekas

👍

Wait: is there a possibility that someone has has specialized the add() method?
Because then this patch is going to change the behavior. As it's for 2.3, we should be cautious.

It's not part of our BC promise.

Even if it's not on our BC promise, this is clearly a behavior change and as such it cannot be committed in 2.3, but only master.

I'm fine if it only goes into master.

Thinking about it more, I think this should not be changed. If someone wants to alter the behavior of add(), he would now also have to override the replace() method and that does not sounds right. Given that we are not talking about a big improvement, I'm 👎 to make this change.

The internal implementation should never be relied on (if not explicitly defined for being extendable, like protected methods). So in this case if a user wants to change the behavior, he has to overwrite all public methods that are relevant, i.e. add and replace.

I bet we have dozens of classes where replace()/set() does not call add().
Or even in this class, get could also call has, but it doesn't. So if someone overwrites has it won't have an effect on get.

Closing as it can break some code for no real benefit.

Btw, the constructor doesn't call set or add as well. So the argument that it allows people to overwrite the behavior with one method is not correct.

I suggest to close this one, the potential benefit looks micro, and we also have a potential bc break, let's stay on the safe side

Cmon if we go this route then we would need to rewrite almost all classes in symfony to use the available getters instead of accessing private properties. We never do that.
The BC break is so unlikely and even if somebody is affected it's clearly his faulty implementation relying on implementation details. Let's at least do this change in 3.0.