-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[Bug] Null check and return false if null. #20855
Conversation
Quality Gate passedIssues Measures |
📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #20855 +/- ##
==========================================
- Coverage 40.70% 40.70% -0.01%
==========================================
Files 1498 1498
Lines 68759 68759
Branches 11366 11367 +1
==========================================
- Hits 27986 27985 -1
Misses 38240 38240
- Partials 2533 2534 +1 ☔ View full report in Codecov by Sentry. |
@zwarm just bumping incase you missed it. Let me know if you can't review. |
Hey @aditi-bhatia sorry for pinging you. If you have time to review, that would be great. |
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 agree that we shouldn't be using the !!
operator but I see every variable in this file using it. I'm concerned that the crash will just happen later down the line with one of the other variables instead, so maybe we should be updating all of these? I know that might increase the scope of this ticket here so curious to hear your thoughts
Sorry I definitely didn't see the notification for this 😅 Thanks for the Slack reminder today! |
@aditi-bhatia It's ok. There are related issues here. I've attempted to reproduce but cannot. Removing the bang operators is easy. The hard part are the changed to all usages. And it's a good amount of them 😅 . I can't reproduce the issue, which makes the decision difficult. I'll circle back here in a few days. |
Sounds good, I tried reproducing but couldn't either. Let me know what you decide / when you'd like me to take a look again 👍 |
Decided to close this PR as another bigger crash would likely fix this here. Small enough change here where it's fine to close. Sorry for the wasted time 😅 |
Fixes Sentry Issue - Github Issue
This crash is happening because we are using the
!!
bang operator to unsafely use a possibly nullable variable. Even if the expectation is to not have a null, crashing isn't the answer. So returning false in case it is null. I took a look at howisPage
is being used, which is the source of the crash. And it's used to decide the height of the bottom sheet. I can't reproduce the issue.An alternative solution here could be to just not allow a publish bottom sheet to show. This could be good because the post loading could be a result of a race condition.
To Test:
Regression Notes
Potential unintended areas of impact
If the post is null, allowing a publish could be problematic. We don't use the post for anything else. So I can't see how it could be an issue. But is a possibility.
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual test.
What automated tests I added (or what prevented me from doing so)
None.
PR Submission Checklist:
RELEASE-NOTES.txt
if necessary.Testing Checklist (strike-out the not-applying and unnecessary ones):