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 value toInvalidArgumentException#60323
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
Conversation
3982cdf tof4a50b9Comparenicolas-grekas commentedMay 4, 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.
I challenged adding these properties in the PRs linked in the above description. https://externals.io/message/127188 andhttps://externals.io/message/127206 might give some food for thoughts on the topic. |
If you want to hear my opinion on these, I think it's reinventing the wheel. There's already established mechanism that developers use for "your input is invalid in a totally predictable way" kind of errors - exceptions. Forasmuch as exceptions are slow, it would make sense to think up the way to make them faster, not to reinvent them. Even though exceptions are somewhat slow, they are not so stupendously slow as database queries, file reads and writes, or any other kind of IO, where PHP just waits for the IO to complete and can nothing in the meantime. I believe that if Edmond doesn't lose faith, it would be solved in the next iterations of True Async. |
Also, don't you think that having the original value of uid that has failed validation in logs is useful? |
| @@ -41,7 +41,7 @@ abstract public static function fromString(string $uid): static; | |||
| public static function fromBinary(string $uid): static | |||
| { | |||
| if (16 !== \strlen($uid)) { | |||
| throw new InvalidArgumentException('Invalid binary uid provided.'); | |||
| throw new InvalidArgumentException($uid,'Invalid binary uid provided.'); | |||
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 quickly scanned our code on how we deal in other places with invalid arguments. Having a dedicated argument is something we don't have anywhere else, but we could embed the invalid value in the exception message. That's something we do quite frequently.
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.
Having a dedicated argument is something we don't have anywhere else
That's a lie.
A lot of exceptions allow passing invalid argument, and expose it.
For example, few exceptions fromDependencyInjection:
DependencyInjection\Exception\AutowiringFailedExceptionexposes service idDependencyInjection\Exception\ParameterCircularReferenceExceptionexposes array of parametersDependencyInjection\Exception\ParameterNotFoundExceptionexposes key, sourceId, sourceKey, alternatives, etcDependencyInjection\Exception\ServiceCircularReferenceExceptionexposes service idDependencyInjection\Exception\ServiceNotFoundExceptionexposes service id, and other
And there are at least 40 other distinct exceptions that follow it.
InvalidArgumentExceptionnicolas-grekas commentedMay 9, 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.
The failing value is / could be found in the error message, just not in a structured way. Let me 👎 here sorry, I already explained why and I don't see much support from others either so let's move on. |
I don't see much need for accessing the problematic uid either, thanks for proposing. |
This is absolutely useful for the case when we need to retrieve the value from logs. Don't expect people to parse exception message to get the list of all the values that failed processing. |
Uh oh!
There was an error while loading.Please reload this page.
[Uid] Add component-specific exception classes ,
[HttpFoundation] Add UriSigner::verify() that throws named exceptions
This PR adds value to Uid-specific InvalidArgumentException.
Thus, anybody who wants to catch the exception, being outside of the scope of the client code, will be able to do so, having the knowledge of what particularly has gone wrong.