Configuration: mutable dependencies shouldn't be cloned #579
Conversation
| $new->jwtConfiguration = clone $jwtConfiguration; | ||
| $new->jwtConfiguration = $jwtConfiguration; |
There was a problem hiding this comment.
This was on purpose: the cloning is specifically done to NOT have the JwtConfig change.
There was a problem hiding this comment.
Yeah, and that's wrong: mutability is part of an API, if an object is designed to be mutable we cannot violate its design.
We can only enforce immutability on our own library, not the others.
There was a problem hiding this comment.
Strong disagree: this is exactly why cloning is done here, to "detach" from the other system.
It was explicitly added here, on purpose.
There was a problem hiding this comment.
I'm not arguing the why: it has always been clear to me, and I agree with that.
I'm saying that even though we are capable of cloning external services, we shouldn't consider ourself allowed to.
A mutable dependency can contain all sort of interconnections with other services, and bad OOP practices.
We are type-hinting some interfaces, and it would be legit for me to feed a dependency that impements that interface and extends the \Doctrine\ORM\UnitOfWork: image the distaster if we clone such Kraken.
There was a problem hiding this comment.
Right, but we are cloning a config object here, not a UnitOfWork: dereferencing and common sense, please 😛
Let's revert this.
No description provided.