[Uid] Add component-specific exception classes #60226
Conversation
| $this->expectException(\InvalidArgumentException::class); | ||
| $this->expectException(InvalidUlidException::class); | ||
| $this->expectExceptionMessage('Invalid ULID: "this is not a ulid".'); | ||
|
|
There was a problem hiding this comment.
@nicolas-grekas , really thank you, appreciate it a lot!
| { | ||
| public function __construct( | ||
| public readonly int $type, | ||
| string $value, |
There was a problem hiding this comment.
do you think it'd be possible to make value accessible as well?
actually there'd been discussion regarding this in a previous MR, @fabpot prefers private property and getter
There was a problem hiding this comment.
private property and getter
I saw that but I think this is still better :)
| class InvalidArgumentException extends \InvalidArgumentException | ||
| { | ||
| } |
There was a problem hiding this comment.
@nicolas-grekas , do you think it could contain value?
There was a problem hiding this comment.
We generally don't include a value in our InvalidArgumentException exceptions. When such error happens for an error related to an object, this would mean that the exception retains a reference to that object (even if you never call the getter), which would have an impact on GC.
There was a problem hiding this comment.
What @stof wrote (and where the previous PR blocked IMHO)
So better not to me.
There was a problem hiding this comment.
Not a reference shall make the impact on garbage collection, unless it creates a circular reference.
There was a problem hiding this comment.
Still, not something that makes sense to me.
There was a problem hiding this comment.
Sure, but it's not the responsibility of the exception to carry this as a standalone value. (the native InvalidArgumentException doesn't carry it if you want an example)
There was a problem hiding this comment.
It is, and it must have the value inside
There was a problem hiding this comment.
I'm sorry you feel frustrated about the situation. We made a step in the direction you're aiming for but we don't do the last step for reasons I already shared. You'll have to parse the message for your use case...
There was a problem hiding this comment.
I don't feel any frustration about anything. I have a big lump of faith down inside of me believing that it is the way to go, and that it should be implemented.
There was a problem hiding this comment.
@nicolas-grekas , I want you to receive this idea
abadae2 to
0c56a4b
Compare
0c56a4b to
e86ffe3
Compare
|
Thank you @rela589n. |
Generalization of #60154
/cc @rela589n