-
Notifications
You must be signed in to change notification settings - Fork 841
Phan: Address PhanPossiblyUndeclaredVariable violations #45911
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
base: trunk
Are you sure you want to change the base?
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Vaultpress plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Boost plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Wpcomsh plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 14 files. Only the first 5 are listed here.
|
projects/packages/videopress/tests/php/VideoPress_Uploader_Test.php
Outdated
Show resolved
Hide resolved
| if ( $site_id > -1 ) { | ||
| $author['site_ID'] = (int) $site_id; | ||
| $author['site_visible'] = $site_visible; | ||
| $author['site_visible'] = $site_visible ?? false; |
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.
Any particular reason not to do this to preserve the current behavior?
| $author['site_visible'] = $site_visible ?? false; | |
| $author['site_visible'] = $site_visible ?? null; |
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.
This value seems to be a bool in normal cases, so I figured I'd match that in edge cases.
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.
Looks like before it was always null unless IS_WPCOM && ! $is_jetpack && $site_id > -1. 🤷
| // Find which endpoint to serve. | ||
| $found = false; | ||
| $found = false; | ||
| $path_pieces = array(); |
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.
You could probably leave this out. The suppression you're adding on line 591 for $endpoint should cover $path_pieces too.
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's true the suppression would cover it, but there are scenarios where it isn't defined (e.g. if the outer or inner for loop doesn't run through).
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.
But in that case $endpoint won't be defined either. So why set one but not the other?
Co-authored-by: Brad Jorsch <anomiex@users.noreply.github.com>
| } | ||
|
|
||
| if ( ! $display_filters && empty( $instance['search_box_enabled'] ) && empty( $instance['user_sort_enabled'] ) ) { | ||
| if ( empty( $filters ) && empty( $instance['search_box_enabled'] ) && empty( $instance['user_sort_enabled'] ) ) { |
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.
$filters will always exist, so you could just do like
| if ( empty( $filters ) && empty( $instance['search_box_enabled'] ) && empty( $instance['user_sort_enabled'] ) ) { | |
| if ( ! $filters && empty( $instance['search_box_enabled'] ) && empty( $instance['user_sort_enabled'] ) ) { |
| endif; | ||
|
|
||
| if ( $display_filters ) { | ||
| if ( ! empty( $filters ) ) { |
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.
Same
| if ( ! empty( $filters ) ) { | |
| if ( $filters ) { |
| Template_Tags::render_widget_search_form( array(), '', '' ); | ||
|
|
||
| if ( $display_filters ) { | ||
| if ( ! empty( $filters ) ) { |
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.
Same
| if ( ! empty( $filters ) ) { | |
| if ( $filters ) { |
| if ( $site_id > -1 ) { | ||
| $author['site_ID'] = (int) $site_id; | ||
| $author['site_visible'] = $site_visible; | ||
| $author['site_visible'] = $site_visible ?? false; |
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.
Looks like before it was always null unless IS_WPCOM && ! $is_jetpack && $site_id > -1. 🤷
| // Find which endpoint to serve. | ||
| $found = false; | ||
| $found = false; | ||
| $path_pieces = array(); |
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.
But in that case $endpoint won't be defined either. So why set one but not the other?
Closes MONOREP-221
This addresses
PhanPossiblyUndeclaredVariableviolations, skipping the following:packages/block-delimiterplugins/crmplugins/super-cacheicalendar-readerandmarkdownlibsFixes include:
projects/packages/backup/src/class-jetpack-backup.phpandprojects/plugins/jetpack/modules/subscriptions/views.php)elseordefaultcaseEdge cases aside, there was one actual bug in
projects/plugins/vaultpress/vaultpress.phpthat may have prevented the Jetpack logo from showing.Proposed changes:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
The goal was to keep things as close to the original functionality as possible. CI should be happy.