chore: Tokens::offsetSet - explicit validation of input
#9004
Conversation
Tokens::offsetSet - explicit validation of input
| Utils::triggerDeprecation(new \InvalidArgumentException(\sprintf( | ||
| 'Tokens should be a list of Token instances - newval must be a Token. This will be enforced in version %d.0.', | ||
| Application::getMajorVersion() + 1 | ||
| ))); |
There was a problem hiding this comment.
I guess it already fails when you try to use other values because of the registerFoundToken call below. registerFoundToken has a native Token type hint.
There was a problem hiding this comment.
yeah, but crash is super ugly when coming from nested function and not part of visible contract for this method (and can change any moment when internals of method change)
There was a problem hiding this comment.
Yes, but not sure if a deprecation is necessary when it already fails anyway. IMO we could just add explicit failure here.
There was a problem hiding this comment.
tbh was wondering about same. not sure. especially as registerFoundToken is called conditionally
There was a problem hiding this comment.
especially as registerFoundToken is called conditionally
Hmm yes. At first I thought the equals condition would always fail for values other than Tokens. But actually it can succeed.
$tokens[$i] = new Token(';');
$tokens[$i] = ';'; // <-- no registerFoundToken call, so probably no failureOk so the deprecation might be better to be safe.
(but I guess you can't do anything useful with values other than Token instances)
There was a problem hiding this comment.
let me conclude as-is for now then. we can always revisit and promote to exception on v3 , if new reason comes in
inspired by #9001