Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix GH-16814: MessageFormatter::format throws an error on invalid arr… #16816

Closed
wants to merge 4 commits into from

Conversation

devnexen
Copy link
Member

…ay element.

string_arg:
/* This implicitly converts objects
* Note that our vectors will leak if object conversion fails
* and PHP ends up with a fatal error and calls longjmp
* as a result of that.
*/
str = zval_get_tmp_string(elem, &tmp_str);
if (!try_convert_to_string(elem)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this will implicitly modify the array. Can we skip the entire iteration when the parameter is unsed?

@nielsdos
Copy link
Member

nielsdos commented Nov 15, 2024

Honestly, I think throwing away the exception is pretty bad. What if the entry is actually used and the __toString() legitimately throws? I also think this isn't a bug in the first place:
If the key is used then we should throw, but if it isn't used then why is it passed in the first place?
If we were a statically typed language we'd have a typehint array<string> and the coercion would fail upfront.

@devnexen
Copy link
Member Author

devnexen commented Nov 15, 2024

I kind of agree with you (one of the reasons I keep it as draft), I was just trying to see what s possible but ultimately looking at the code so many times, it is mostly good as is..

@iluuu1994
Copy link
Member

I agree that exceptions should not be discarded. What I was suggesting is trying to skip the coercion to string in the first place. But as Niels has mentioned, IMO this is definitely not a bug, and might not even be a worthwhile feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants