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.

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 function arguments types reported by PHPStan #553

Closed
wants to merge 90 commits into from
Closed

Fix function arguments types reported by PHPStan #553

wants to merge 90 commits into from

Conversation

xknown
Copy link
Member

@xknown xknown commented Sep 24, 2020

This PR attempts to fix the arguments type passed to a WP core and PHP's built-in functions, which would hopefully help reduce the number of PHP fatals for PHP 8.0.

The full list of reported issues can be seen at

https://gist.github.com/xknown/77f8cbe233da75080d1e9258a8c94f95

https://core.trac.wordpress.org/ticket/51423

While this will not be a type error in PHP, we maybe need to report an
error if the user pass an invalid post/page. Otherwise, one can only see
an empty page.
`remove_query_arg()` only accepts `array|string`. This doesn't produce a
PHP fatal currently, but it will if we ever add type hints in the
future.
This adds explicit casts to the arguments passed to both `esc_url`, and
`wp_basename`
It removes some redundant esc_attr calls.
We sometimes pass the result (`bool|array`) of `request_filesystem_credentials()` to the first parameter (`array|false`) of `WP_Filesystem()`. This change attempts to make the PHPDoc type hints consistent
Updates `wp_update_link` to handle invalid link ids, also adds a couple
of unit tests.
This makes sure we are checking for error conditions to avoid potential
PHP type errors.
…e/is_wp_version_compatible` require a string
fatal caused by "774    Parameter #1 $arr1 of function array_merge
expects array, array|bool given."
`extract_from_markers`, as well as `insert_with_markers`.

Add also unit tests
appropriate types to avoid potential PHP type errors/warnings.
couple of PHP warnings (array_pop needs a variable)
@@ -246,7 +252,7 @@

<?php wp_nonce_field( $nonce_action ); ?>
<input type="hidden" name="action" value="<?php echo esc_attr( $formaction ); ?>" />
<input type="hidden" name="c" value="<?php echo esc_attr( $comment->comment_ID ); ?>" />
<input type="hidden" name="c" value="<?php echo intval( $comment->comment_ID ); ?>" />
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<input type="hidden" name="c" value="<?php echo intval( $comment->comment_ID ); ?>" />
<input type="hidden" name="c" value="<?php echo (int) $comment->comment_ID; ?>" />

@@ -279,7 +288,7 @@
comment_footer_die( __( 'Sorry, you are not allowed to edit comments on this post.' ) );
}

if ( wp_get_referer() && ! $noredir && false === strpos( wp_get_referer(), 'comment.php' ) ) {
if ( wp_get_referer() && ! $noredir && false === strpos( strval( wp_get_referer() ), 'comment.php' ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if ( wp_get_referer() && ! $noredir && false === strpos( strval( wp_get_referer() ), 'comment.php' ) ) {
if ( wp_get_referer() && ! $noredir && false === strpos( (string) wp_get_referer(), 'comment.php' ) ) {

@@ -261,7 +261,7 @@
$class .= ' active';
}
?>
<button type="button" class="<?php echo esc_attr( $class ); ?>" aria-pressed="<?php echo esc_attr( $active ); ?>" data-device="<?php echo esc_attr( $device ); ?>">
<button type="button" class="<?php echo esc_attr( $class ); ?>" aria-pressed="<?php echo esc_attr( $active ? 'true' : 'false' ); ?>" data-device="<?php echo esc_attr( $device ); ?>">
Copy link
Member

Choose a reason for hiding this comment

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

If the values are hard-coded strings, 'true'/'false', the call to esc_attr() can be removed.

$admin_body_class .= ' branch-' . str_replace( array( '.', ',' ), '-', (float) get_bloginfo( 'version' ) );
$admin_body_class .= ' version-' . str_replace( '.', '-', preg_replace( '/^([.0-9]+).*/', '$1', get_bloginfo( 'version' ) ) );
$admin_body_class .= ' branch-' . str_replace( array( '.', ',' ), '-', (string) ( (float) get_bloginfo( 'version' ) ) );
$admin_body_class .= ' version-' . str_replace( '.', '-', (string) preg_replace( '/^([.0-9]+).*/', '$1', get_bloginfo( 'version' ) ) );
Copy link
Member

@jrfnl jrfnl Oct 17, 2020

Choose a reason for hiding this comment

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

preg_replace() will always return a string when given a string, except when the regex errored out and this regex does not look like one which will error.

@@ -242,7 +242,7 @@

$form_action = 'editpost';
$nonce_action = 'update-post_' . $post_ID;
$form_extra .= "<input type='hidden' id='post_ID' name='post_ID' value='" . esc_attr( $post_ID ) . "' />";
$form_extra .= "<input type='hidden' id='post_ID' name='post_ID' value='" . intval( $post_ID ) . "' />";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$form_extra .= "<input type='hidden' id='post_ID' name='post_ID' value='" . intval( $post_ID ) . "' />";
$form_extra .= "<input type='hidden' id='post_ID' name='post_ID' value='" . (int) $post_ID . "' />";

@@ -1561,7 +1573,7 @@ function _wp_post_thumbnail_html( $thumbnail_id = null, $post = null ) {
}
}

$content .= '<input type="hidden" id="_thumbnail_id" name="_thumbnail_id" value="' . esc_attr( $thumbnail_id ? $thumbnail_id : '-1' ) . '" />';
$content .= '<input type="hidden" id="_thumbnail_id" name="_thumbnail_id" value="' . intval( $thumbnail_id ? $thumbnail_id : '-1' ) . '" />';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$content .= '<input type="hidden" id="_thumbnail_id" name="_thumbnail_id" value="' . intval( $thumbnail_id ? $thumbnail_id : '-1' ) . '" />';
$content .= '<input type="hidden" id="_thumbnail_id" name="_thumbnail_id" value="' . ( $thumbnail_id ? (int) $thumbnail_id : '-1' ) . '" />';

@jrfnl
Copy link
Member

jrfnl commented Oct 17, 2020

Looked through a few of the changes (by far not all) and would like to observe the following: a lot of these changes bypass underlying errors by doing type casting, instead of handling the errors properly. While this will work, I'm not sure this is always the right choice.

@xknown
Copy link
Member Author

xknown commented Nov 3, 2020

Looked through a few of the changes (by far not all) and would like to observe the following: a lot of these changes bypass underlying errors by doing type casting, instead of handling the errors properly. While this will work, I'm not sure this is always the right choice.

Thanks for the feedback, and sorry for the late response. I have been busy with some work-related things, and some afk time. I agree that most of the changes are only adding explicit casts, which is probably fine in most cases.

I've been manually reviewing these changes, and while I add some proper error checking in some places, some commits only add explicit casts mostly because the surrounding code don't really handle very well error conditions (one example that comes to my mind is errors happening when running wp_install()). If you point me out a few cases where do you think we can do something different, then please let me know.

@xknown
Copy link
Member Author

xknown commented Nov 10, 2020

Closing in favor of individual PRs. See https://core.trac.wordpress.org/ticket/51423 for more details.

@xknown xknown closed this Nov 10, 2020
@xknown xknown deleted the fix/function-argument-types-php8.0 branch November 10, 2020 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants