-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fixes a crash when the value returned by Blog.version is not a string #16877
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
Conversation
|
You can trigger an installable build for these changes by visiting CircleCI here. |
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
|
@chipsnyder @Gio2018 @leandroalonso 👋 folks! @emilylaguna was a legend in jumping onto this bugfix and ping me about it. I don't feel like I have too much context around this code to give a proper review, though, so I'm pinging you as per GitHub's suggestion. Feel free to bring more knowledgeable people in (blame GitHub if this ping is inappropriate 😝) Thanks! For context the crash this PR fixes is something that is a new one in 17.7 and it's slowly but steadily rising in occurrences. |
|
I had time to start the hotfix process and changed the base for this PR to |
|
Reviewing it 👀 |
leandroalonso
left a comment
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 wasn't able to trigger the crash by adding a self-hosted site.
But following the test steps and playing with the value I was able to check correctness.
@emilylaguna this is a good opportunity to add unit tests for this change, ensuring that it works as expected and preventing any regression in the future. :)
|
@leandroalonso good point! I'm adding tests now. |
|
@leandroalonso tests added: ea8b3e3 |
|
@leandroalonso updated tests to include a test covering the "else" case: 741d128 |
|
🥳 Will merge once build tests finish. |
| set(blogOption: "jetpack_user_email", value: email) | ||
| set(blogOption: "jetpack_version", value: version as Any) | ||
| set(blogOption: "jetpack_user_login", value: username as Any) | ||
| set(blogOption: "jetpack_user_email", value: email as Any) |
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.
@emilylaguna I noticed these typecast while looking over #16883.
I'm curious to know what made them necessary?
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.

Sentry crash: https://sentry.io/organizations/a8c/issues/2511522132/events/?project=1438083
To test:
Verify the crash
Force the crash
return @5.5Verify the fix
This force returns an NSNumber for the software version
5. Relaunch the app
6. It should not crash
Regression Notes
[[blog version] floatValue];since NSString implements this method this will not cause an issue.hasRequiredWordPressVersionin Blog.m expects a string anywaysWhat I did to test those areas of impact (or what existing automated tests I relied on)
I did manual testing of that section.
What automated tests I added (or what prevented me from doing so)
None.
PR submission checklist:
RELEASE-NOTES.txtif necessary.