Skip to content

PHPStan level 9 #536

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

PHPStan level 9 #536

wants to merge 14 commits into from

Conversation

swissspidy
Copy link
Member

@swissspidy swissspidy commented May 18, 2025

@swissspidy swissspidy force-pushed the add/phpstan branch 2 times, most recently from 8f14b59 to 7a36e2a Compare May 18, 2025 12:16
*/
protected function _url( $args, $callback ) {
foreach ( $args as $obj_id ) {
$object = $this->fetcher->get_check( $obj_id );
Copy link
Member Author

Choose a reason for hiding this comment

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

The fetcher property didn't even exist on this abstract class, and in the extending classes it was private. So this code never worked. Plus it was never used, so it's safe to remove.

@@ -182,10 +182,6 @@ public function create( $args, $assoc_args ) {
$assoc_args['post_category'] = $this->get_category_ids( $assoc_args['post_category'] );
}

if ( isset( $assoc_args['meta_input'] ) && Utils\wp_version_compare( '4.4', '<' ) ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Figured with our move to WP 4.9+ we can remove this compat code.

Copy link

codecov bot commented May 18, 2025

Codecov Report

Attention: Patch coverage is 91.05691% with 11 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/User_Command.php 75.86% 7 Missing ⚠️
src/Menu_Command.php 0.00% 1 Missing ⚠️
src/Menu_Item_Command.php 87.50% 1 Missing ⚠️
src/Site_Command.php 90.00% 1 Missing ⚠️
src/Term_Command.php 95.83% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@swissspidy swissspidy marked this pull request as ready for review May 18, 2025 13:16
@swissspidy swissspidy requested a review from a team as a code owner May 18, 2025 13:16
@swissspidy swissspidy marked this pull request as draft July 3, 2025 13:53
@swissspidy swissspidy changed the title PHPStan level 6 PHPStan level 9 Jul 3, 2025
@swissspidy swissspidy marked this pull request as ready for review July 3, 2025 15:46
if ( false === $items || is_wp_error( $items ) ) {
if ( false === $items ) {
Copy link
Member

Choose a reason for hiding this comment

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

Is it impossible here to end up with a WP_Error? Relying on the docblocks is not enough, and I'm seeing that multiple filters are involved... 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no code path in there that returns WP_Error, I already checked.

Filters can always be abused, but we can‘t check for every possible return value.

git blame was unfortunately not helpful here (mangled history)

Comment on lines -414 to +416
if ( ! $menu || is_wp_error( $menu ) ) {
if ( false === $menu ) {
Copy link
Member

Choose a reason for hiding this comment

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

Same here re. WP_Error. I'm assuming there was a reason why that particular check was in there?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so.

It was originally introduced in 246a8bc, but without any test coverage or so.

Like in the other cases, this function never returns a WP_Error

*/
public function remove( $args, $assoc_args ) {

list( $menu, $location ) = $args;

$menu = wp_get_nav_menu_object( $menu );
if ( ! $menu || is_wp_error( $menu ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Same here re. WP_Error.

@swissspidy swissspidy requested a review from schlessera July 8, 2025 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants