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.

Sign up for GitHub

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/#5110 fix php81 warnings #5968

Merged
merged 11 commits into from
Dec 16, 2022
Merged

Bug/#5110 fix php81 warnings #5968

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1ec76fd
Refactor usage of `FILTER_SANITIZE_STRING`.
kuasha420 Oct 3, 2022
e8f99a9
Fix failing phpunit test.
kuasha420 Oct 3, 2022
36f7040
Change default value of `filter_input`'s `options` param to 0.
kuasha420 Oct 5, 2022
fc51d2a
Fix `htmlspecialchars` null warnings.
kuasha420 Oct 5, 2022
0252423
Do not call htmlspecialchars with null setup_slug.
kuasha420 Oct 13, 2022
0533f2b
Check $error to be not empty before running `htmlspecialchars`.
kuasha420 Oct 14, 2022
6930f15
Use Elvis operator.
kuasha420 Nov 1, 2022
575cc70
Add `ReturnTypeWillChange` attribute to supress warnings.
kuasha420 Nov 1, 2022
0493f60
Add spaces in phpcs multiple ignores.
kuasha420 Nov 1, 2022
d6d889b
Replace phpcs disables with inline ignores.
kuasha420 Nov 30, 2022
9197ff7
Address CR feedback.
kuasha420 Dec 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion includes/Core/Admin/Screens.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,7 @@ private function get_screens() {
'render_callback' => function( Context $context ) {
$is_view_only = ! $this->authentication->is_authenticated();

$setup_slug = $context->input()->filter( INPUT_GET, 'slug', FILTER_SANITIZE_STRING );
$setup_slug = htmlspecialchars( $context->input()->filter( INPUT_GET, 'slug' ) ?: '' );
$reauth = $context->input()->filter( INPUT_GET, 'reAuth', FILTER_VALIDATE_BOOLEAN );
if ( $context->input()->filter( INPUT_GET, 'permaLink' ) ) {
?>
Expand Down
2 changes: 1 addition & 1 deletion includes/Core/Admin/Standalone.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ function() {
public function is_standalone() {
global $pagenow;

$page = $this->context->input()->filter( INPUT_GET, 'page', FILTER_SANITIZE_STRING );
$page = htmlspecialchars( $this->context->input()->filter( INPUT_GET, 'page' ) ?: '' );
$standalone = $this->context->input()->filter( INPUT_GET, 'googlesitekit-standalone', FILTER_VALIDATE_BOOLEAN );

return ( 'admin.php' === $pagenow && false !== strpos( $page, 'googlesitekit' ) && $standalone );
Expand Down
8 changes: 2 additions & 6 deletions includes/Core/Assets/Assets.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -911,18 +911,14 @@ private function get_inline_tracking_data() {
* @return array The inline data to be output.
*/
private function get_inline_data() {
$current_user = wp_get_current_user();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

$site_url = $this->context->get_reference_site_url();
$input = $this->context->input();
$page = $input->filter( INPUT_GET, 'page', FILTER_SANITIZE_STRING );
$site_url = $this->context->get_reference_site_url();
$input = $this->context->input();

$admin_data = array(
'siteURL' => esc_url_raw( $site_url ),
'resetSession' => $input->filter( INPUT_GET, 'googlesitekit_reset_session', FILTER_VALIDATE_BOOLEAN ),
);

$current_entity = $this->context->get_reference_entity();

return array(

/**
Expand Down
2 changes: 1 addition & 1 deletion includes/Core/Authentication/Authentication.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ public function register() {
'admin_init',
function() {
if (
'googlesitekit-dashboard' === $this->context->input()->filter( INPUT_GET, 'page', FILTER_SANITIZE_STRING )
'googlesitekit-dashboard' === htmlspecialchars( $this->context->input()->filter( INPUT_GET, 'page' ) ?: '' )
&& User_Input_State::VALUE_REQUIRED === $this->user_input_state->get()
) {
wp_safe_redirect( $this->context->admin_url( 'user-input' ) );
Expand Down
6 changes: 3 additions & 3 deletions includes/Core/Authentication/Clients/OAuth_Client.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,8 +399,8 @@ function ( $scope ) {
* @since 1.49.0 Uses the new `Google_Proxy::setup_url_v2` method when the `serviceSetupV2` feature flag is enabled.
*/
public function authorize_user() {
$code = $this->context->input()->filter( INPUT_GET, 'code', FILTER_SANITIZE_STRING );
$error_code = $this->context->input()->filter( INPUT_GET, 'error', FILTER_SANITIZE_STRING );
$code = htmlspecialchars( $this->context->input()->filter( INPUT_GET, 'code' ) );
$error_code = htmlspecialchars( $this->context->input()->filter( INPUT_GET, 'error' ) );
// If the OAuth redirects with an error code, handle it.
if ( ! empty( $error_code ) ) {
$this->user_options->set( self::OPTION_ERROR_CODE, $error_code );
Expand Down Expand Up @@ -450,7 +450,7 @@ public function authorize_user() {
if ( isset( $token_response['scope'] ) ) {
$scopes = explode( ' ', sanitize_text_field( $token_response['scope'] ) );
} elseif ( $this->context->input()->filter( INPUT_GET, 'scope' ) ) {
$scope = $this->context->input()->filter( INPUT_GET, 'scope', FILTER_SANITIZE_STRING );
$scope = htmlspecialchars( $this->context->input()->filter( INPUT_GET, 'scope' ) );
$scopes = explode( ' ', $scope );
} else {
$scopes = $this->get_required_scopes();
Expand Down
22 changes: 11 additions & 11 deletions includes/Core/Authentication/Setup.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ private function get_oauth_proxy_failed_help_link() {
* @since 1.48.0
*/
public function handle_action_setup_start() {
$nonce = $this->context->input()->filter( INPUT_GET, 'nonce', FILTER_SANITIZE_STRING );
$nonce = htmlspecialchars( $this->context->input()->filter( INPUT_GET, 'nonce' ) );
$redirect_url = $this->context->input()->filter( INPUT_GET, 'redirect', FILTER_SANITIZE_URL );

$this->verify_nonce( $nonce, Google_Proxy::ACTION_SETUP_START );
Expand Down Expand Up @@ -207,12 +207,12 @@ public function handle_action_setup_start() {
*/
public function handle_action_verify() {
$input = $this->context->input();
$step = $input->filter( INPUT_GET, 'step', FILTER_SANITIZE_STRING );
$nonce = $input->filter( INPUT_GET, 'nonce', FILTER_SANITIZE_STRING );
$code = $input->filter( INPUT_GET, 'googlesitekit_code', FILTER_SANITIZE_STRING );
$site_code = $input->filter( INPUT_GET, 'googlesitekit_site_code', FILTER_SANITIZE_STRING );
$verification_token = $input->filter( INPUT_GET, 'googlesitekit_verification_token', FILTER_SANITIZE_STRING );
$verification_method = $input->filter( INPUT_GET, 'googlesitekit_verification_token_type', FILTER_SANITIZE_STRING );
$step = htmlspecialchars( $input->filter( INPUT_GET, 'step' ) );
$nonce = htmlspecialchars( $input->filter( INPUT_GET, 'nonce' ) );
$code = htmlspecialchars( $input->filter( INPUT_GET, 'googlesitekit_code' ) );
$site_code = htmlspecialchars( $input->filter( INPUT_GET, 'googlesitekit_site_code' ) );
$verification_token = htmlspecialchars( $input->filter( INPUT_GET, 'googlesitekit_verification_token' ) );
$verification_method = htmlspecialchars( $input->filter( INPUT_GET, 'googlesitekit_verification_token_type' ) );

$this->verify_nonce( $nonce );

Expand Down Expand Up @@ -266,10 +266,10 @@ public function handle_action_verify() {
*/
public function handle_action_exchange_site_code() {
$input = $this->context->input();
$step = $input->filter( INPUT_GET, 'step', FILTER_SANITIZE_STRING );
$nonce = $input->filter( INPUT_GET, 'nonce', FILTER_SANITIZE_STRING );
$code = $input->filter( INPUT_GET, 'googlesitekit_code', FILTER_SANITIZE_STRING );
$site_code = $input->filter( INPUT_GET, 'googlesitekit_site_code', FILTER_SANITIZE_STRING );
$step = htmlspecialchars( $input->filter( INPUT_GET, 'step' ) );
$nonce = htmlspecialchars( $input->filter( INPUT_GET, 'nonce' ) );
$code = htmlspecialchars( $input->filter( INPUT_GET, 'googlesitekit_code' ) );
$site_code = htmlspecialchars( $input->filter( INPUT_GET, 'googlesitekit_site_code' ) );

$this->verify_nonce( $nonce );

Expand Down
16 changes: 12 additions & 4 deletions includes/Core/REST_API/Data_Request.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,9 @@ public function __isset( $name ) {
*
* @return bool
*/
public function offsetExists( $key ) {
// phpcs:ignore Squiz.Commenting.InlineComment.WrongStyle,Squiz.PHP.CommentedOutCode.Found
#[\ReturnTypeWillChange]
public function offsetExists( $key ) { // phpcs:ignore Squiz.Commenting.FunctionComment.WrongStyle
Copy link
Collaborator

Choose a reason for hiding this comment

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

When I run phpcs with PHP 8, these lines still raise errors:

image

Loosening up the rule definition allows linting to pass in PHP 8 as well as older versions.

Suggested change
public function offsetExists( $key ) { // phpcs:ignore Squiz.Commenting.FunctionComment.WrongStyle
public function offsetExists( $key ) { // phpcs:ignore Squiz.Commenting.FunctionComment

return array_key_exists( $key, $this->data );
}

Expand All @@ -132,7 +134,9 @@ public function offsetExists( $key ) {
*
* @return mixed
*/
public function offsetGet( $key ) {
// phpcs:ignore Squiz.Commenting.InlineComment.WrongStyle,Squiz.PHP.CommentedOutCode.Found
#[\ReturnTypeWillChange]
public function offsetGet( $key ) { // phpcs:ignore Squiz.Commenting.FunctionComment.WrongStyle
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above

Suggested change
public function offsetGet( $key ) { // phpcs:ignore Squiz.Commenting.FunctionComment.WrongStyle
public function offsetGet( $key ) { // phpcs:ignore Squiz.Commenting.FunctionComment

if ( $this->offsetExists( $key ) ) {
return $this->data[ $key ];
}
Expand All @@ -146,7 +150,9 @@ public function offsetGet( $key ) {
* @param string|int $key Key to set the value for.
* @param mixed $value New value for the given key.
*/
public function offsetSet( $key, $value ) {
// phpcs:ignore Squiz.Commenting.InlineComment.WrongStyle,Squiz.PHP.CommentedOutCode.Found
#[\ReturnTypeWillChange]
public function offsetSet( $key, $value ) { // phpcs:ignore Squiz.Commenting.FunctionComment.WrongStyle
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above

Suggested change
public function offsetSet( $key, $value ) { // phpcs:ignore Squiz.Commenting.FunctionComment.WrongStyle
public function offsetSet( $key, $value ) { // phpcs:ignore Squiz.Commenting.FunctionComment

// Data is immutable.
}

Expand All @@ -155,7 +161,9 @@ public function offsetSet( $key, $value ) {
*
* @param string|int $key Key to unset.
*/
public function offsetUnset( $key ) {
// phpcs:ignore Squiz.Commenting.InlineComment.WrongStyle,Squiz.PHP.CommentedOutCode.Found
#[\ReturnTypeWillChange]
public function offsetUnset( $key ) { // phpcs:ignore Squiz.Commenting.FunctionComment.WrongStyle
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above

Suggested change
public function offsetUnset( $key ) { // phpcs:ignore Squiz.Commenting.FunctionComment.WrongStyle
public function offsetUnset( $key ) { // phpcs:ignore Squiz.Commenting.FunctionComment

// Data is immutable.
}
}
3 changes: 2 additions & 1 deletion includes/Core/Util/Input.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ public function __construct() {
* Gets a specific external variable by name and optionally filters it.
*
* @since 1.1.2
* @since n.e.x.t Changed default value of $options parameter to 0.
*
* @link https://php.net/manual/en/function.filter-input.php
*
Expand All @@ -60,7 +61,7 @@ public function __construct() {
* If the flag FILTER_NULL_ON_FAILURE is used, it returns FALSE if the variable is not set
* and NULL if the filter fails.
*/
public function filter( $type, $variable_name, $filter = FILTER_DEFAULT, $options = null ) {
public function filter( $type, $variable_name, $filter = FILTER_DEFAULT, $options = 0 ) {
$value = filter_input( $type, $variable_name, $filter, $options );

// Fallback for environments where filter_input may not work with specific types.
Expand Down
12 changes: 6 additions & 6 deletions includes/Modules/Analytics.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ protected function handle_provisioning_callback() {
}

// The handler should check the received Account Ticket id parameter against the id stored in the provisioning step.
$account_ticket_id = $input->filter( INPUT_GET, 'accountTicketId', FILTER_SANITIZE_STRING );
$account_ticket_id = htmlspecialchars( $input->filter( INPUT_GET, 'accountTicketId' ) );
$stored_account_ticket_id = get_transient( self::PROVISION_ACCOUNT_TICKET_ID . '::' . get_current_user_id() );
delete_transient( self::PROVISION_ACCOUNT_TICKET_ID . '::' . get_current_user_id() );

Expand All @@ -277,17 +277,17 @@ protected function handle_provisioning_callback() {
}

// Check for a returned error.
$error = $input->filter( INPUT_GET, 'error', FILTER_SANITIZE_STRING );
$error = $input->filter( INPUT_GET, 'error' );
if ( ! empty( $error ) ) {
wp_safe_redirect(
$this->context->admin_url( 'module-analytics', array( 'error_code' => $error ) )
$this->context->admin_url( 'module-analytics', array( 'error_code' => htmlspecialchars( $error ) ) )
);
exit;
}

$account_id = $input->filter( INPUT_GET, 'accountId', FILTER_SANITIZE_STRING );
$web_property_id = $input->filter( INPUT_GET, 'webPropertyId', FILTER_SANITIZE_STRING );
$profile_id = $input->filter( INPUT_GET, 'profileId', FILTER_SANITIZE_STRING );
$account_id = htmlspecialchars( $input->filter( INPUT_GET, 'accountId' ) );
$web_property_id = htmlspecialchars( $input->filter( INPUT_GET, 'webPropertyId' ) );
$profile_id = htmlspecialchars( $input->filter( INPUT_GET, 'profileId' ) );

if ( empty( $account_id ) || empty( $web_property_id ) || empty( $profile_id ) ) {
wp_safe_redirect(
Expand Down
2 changes: 1 addition & 1 deletion tests/phpunit/integration/ContextTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public function test_filter_input() {
$this->assertTrue( $context->input()->filter( INPUT_GET, 'foo', FILTER_VALIDATE_BOOLEAN ) );

$_GET['dirty'] = '<script>dirt</script>';
$this->assertEquals( 'dirt', $context->input()->filter( INPUT_GET, 'dirty', FILTER_SANITIZE_STRING ) );
$this->assertEquals( '&lt;script&gt;dirt&lt;/script&gt;', htmlspecialchars( $context->input()->filter( INPUT_GET, 'dirty' ) ) );
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test doesn't seem so useful now as it's effectively just testing the standard htmlspecialchars function. I'd suggest it should be removed.

Suggested change
$_GET['dirty'] = '<script>dirt</script>';
$this->assertEquals( 'dirt', $context->input()->filter( INPUT_GET, 'dirty', FILTER_SANITIZE_STRING ) );
$this->assertEquals( '&lt;script&gt;dirt&lt;/script&gt;', htmlspecialchars( $context->input()->filter( INPUT_GET, 'dirty' ) ) );

}

public function test_admin_url() {
Expand Down