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: correctly emplace optional values in the value converter #20985
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.
i am deeply sorry for the pain this probably caused you to debug
What. The. Hell. |
return true; | ||
} | ||
T converted; | ||
if (Converter<T>::FromV8(isolate, val, &converted)) { | ||
if (!Converter<T>::FromV8(isolate, val, &converted)) { | ||
*out = base::nullopt; |
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 seems like this branch should return false
with a failed conversion rather than silently convert to nullopt, but given that this is fixing a much bigger 🤦♂ issue, i'm OK with merging as is.
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 isn't a failed conversion because this is an optional param. If this param fails to convert the optional will be empty and the next arg will use this param.
This emulates expected behavior of optional arguments in the middle of argument lists.
Release Notes Persisted
|
I have automatically backported this PR to "7-1-x", please check out #21008 |
I don't wanna talk about it....
Fixes #20941
Will follow up with a test for master, we do not need to fix anything in master as the gin converter migration fixed this by accident
Notes: Fixed issue where
app.setAppLogsPath
did not work when you provided a valid path