Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Uid] Add component-specific exception classes#60226
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
lovely ❤️
Uh oh!
There was an error while loading.Please reload this page.
| $this->expectException(\InvalidArgumentException::class); | ||
| $this->expectException(InvalidUlidException::class); | ||
| $this->expectExceptionMessage('Invalid ULID: "this is not a ulid".'); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@nicolas-grekas , do you think it could contain value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We generally don't include a value in ourInvalidArgumentException 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.
nicolas-grekasApr 16, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What@stof wrote (and where the previous PR blocked IMHO)
So better not to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not a reference shall make the impact on garbage collection, unless it creates a circular reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Still, not something that makes sense to me.
nicolas-grekasApr 17, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It is, and it must have the value inside
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@nicolas-grekas , I want you to receive this idea
abadae2 to0c56a4bCompare0c56a4b toe86ffe3CompareThank you@rela589n. |
1d7c9ec intosymfony:7.3Uh oh!
There was an error while loading.Please reload this page.
Generalization of#60154
/cc@rela589n