Add @phpstan-return never

[IanDelMar]

This PR adds @phpstan-return never to functions/methods that will never return a value, and always terminate with a call to die/exit, wp_die, wp_send_json, wp_send_json_success or wp_send_json_error.

There are some functions that are not covered yet. Cases that are not covered are:

• the terminated function call is wrapped in a if-else (e.g. wp_ajax_send_password_reset()),
• a call to wp_die() that uses a variable as 3rd argument (e.g. wp_nonce_ays()),
• a call to wp_die() that explicitly sets the default value for 'exit' (e.g. wp_die('', '', ['exit' => true])).

Related to #36

List of functions/methods affected

• comment_footer_die
• Custom_Background::ajax_background_add
• Custom_Background::wp_set_background_image
• Custom_Image_Header::ajax_header_add
• Custom_Image_Header::ajax_header_crop
• Custom_Image_Header::ajax_header_remove
• dead_db
• do_favicon
• graceful_fail
• install_theme_information
• media_send_to_editor
• ms_not_installed
• redirect_post
• wp_ajax_add_menu_item
• wp_ajax_ajax_tag_search
• wp_ajax_autocomplete_user
• wp_ajax_closed_postboxes
• wp_ajax_crop_image
• wp_ajax_dashboard_widgets
• wp_ajax_date_format
• wp_ajax_delete_comment
• wp_ajax_delete_inactive_widgets
• wp_ajax_delete_meta
• wp_ajax_delete_plugin
• wp_ajax_delete_theme
• wp_ajax_destroy_sessions
• wp_ajax_dim_comment
• wp_ajax_dismiss_wp_pointer
• wp_ajax_fetch_list
• wp_ajax_find_posts
• wp_ajax_generate_password
• wp_ajax_get_attachment
• wp_ajax_get_permalink
• wp_ajax_get_post_thumbnail_html
• wp_ajax_get_revision_diffs
• wp_ajax_get_tagcloud
• wp_ajax_health_check_background_updates
• wp_ajax_health_check_dotorg_communication
• wp_ajax_health_check_get_sizes
• wp_ajax_health_check_loopback_requests
• wp_ajax_health_check_site_status_result
• wp_ajax_heartbeat
• wp_ajax_hidden_columns
• wp_ajax_image_editor
• wp_ajax_imgedit_preview
• wp_ajax_inline_save
• wp_ajax_inline_save_tax
• wp_ajax_install_plugin
• wp_ajax_install_theme
• wp_ajax_logged_in
• wp_ajax_media_create_image_subsizes
• wp_ajax_menu_get_metabox
• wp_ajax_menu_locations_save
• wp_ajax_menu_quick_search
• wp_ajax_meta_box_order
• wp_ajax_nopriv_generate_password
• wp_ajax_nopriv_heartbeat
• wp_ajax_oembed_cache
• wp_ajax_parse_embed
• wp_ajax_parse_media_shortcode
• wp_ajax_query_attachments
• wp_ajax_query_themes
• wp_ajax_rest_nonce
• wp_ajax_sample_permalink
• wp_ajax_save_attachment
• wp_ajax_save_attachment_compat
• wp_ajax_save_attachment_order
• wp_ajax_save_user_color_scheme
• wp_ajax_save_widget
• wp_ajax_save_wporg_username
• wp_ajax_search_install_plugins
• wp_ajax_search_plugins
• wp_ajax_send_attachment_to_editor
• wp_ajax_send_link_to_editor
• wp_ajax_set_attachment_thumbnail
• wp_ajax_set_post_thumbnail
• wp_ajax_time_format
• wp_ajax_toggle_auto_updates
• wp_ajax_trash_post
• wp_ajax_update_plugin
• wp_ajax_update_theme
• wp_ajax_update_welcome_panel
• wp_ajax_upload_attachment
• wp_ajax_widgets_order
• wp_ajax_wp_compression_test
• wp_ajax_wp_fullscreen_save_post
• wp_ajax_wp_link_ajax
• wp_ajax_wp_privacy_erase_personal_data
• wp_ajax_wp_privacy_export_personal_data
• wp_ajax_wp_remove_post_lock
• WP_Customize_Manager::handle_dismiss_autosave_or_lock_request
• WP_Customize_Manager::handle_load_themes_request
• WP_Customize_Manager::handle_override_changeset_lock_request
• WP_Customize_Manager::refresh_nonces
• WP_Customize_Manager::wp_die
• WP_Customize_Nav_Menus::ajax_load_available_items
• WP_Customize_Widgets::wp_ajax_update_widget
• WP_List_Table::ajax_response
• WP_List_Table::ajax_user_can
• WP_List_Table::get_columns
• WP_List_Table::prepare_items
• WP_Recovery_Mode::redirect_protected
• wp_send_json_error
• wp_send_json_success
• WP_Sitemaps_Stylesheet::render_stylesheet
• WP_Widget::widget
• wpmu_admin_do_redirect

[szepeviktor]

Thank you.

Do we still need szepeviktor/phpstan-wordpress#201?

[IanDelMar]

Do we still need szepeviktor/phpstan-wordpress#201?

Yes. The extension handles wp_die. This PR handles functions that call eg wp_die.

[swissspidy]

Is it possible to add some exceptions to this rule?

For example WP_Widget::widget() calls die( 'function WP_Widget::widget() must be overridden in a subclass.' );

And now PHPStan complains that my widget's widget() method doesn't exit. It may be technically correct, but in practice it shouldn't really complain here.

[szepeviktor]

Does adding a return type void to your widget method help?

[swissspidy]

It does not. Adding @phpstan-return never to my method also doesn't help.

[szepeviktor]

We could have an exception list for everything. And we just go crazy 🤯 "thanks" to (governance-less) WordPress.

My contribution to the WordPress ecosystem should stop and my related packages be abandoned.

[szepeviktor]

It does not. Adding @phpstan-return never to my method also doesn't help.

Please add this error to your ignore list.
Having PHPStan running on WordPress is already a miracle!

[swissspidy]

Please add this error to your ignore list.

Yeah that's what I did now

[herndlm]

Viktor we know how you think about WordPress, but please don't forget that the users here are most likely the ones that want to make things better or are somewhat aware of the bad sides of WordPress :)

[szepeviktor]

I know that, Martin.
Having those static analysis packages with 6M downloads helps others but makes me known as a "WordPress expert". Nowadays when I fork a plugin repo I rename "WordPress"/"wp" to "that".
e.g. https://github.com/szepeviktor/ElasticThat
No one finds me to run/maintain his WordPress website/webshop/app.
I was talking to a handful of WordPress agencies, they are all looking for builders but I am a thinker! (and these agencies do not have engineer qualities, they just take the keyboard and start typing)

[szepeviktor]

BTW We cannot make WordPress better, John and Ian is sending PR-s here, not to core!

[IanDelMar]

John and Ian is sending PR-s here, not to core!

I just refuse to learn how to deal with this Core Trac thing. Getting old and inflexible.

Regarding WP_Widget::widget(): I'm not sure what the point is in putting die( 'Function WP_Widget::widget() must be overridden in a subclass. ); in a method that is supposed to be overridden. However, I suspect it's a fairly common thing to extend WP_Widget and we should consider an exception if more people have "trouble" with the @phpstan-return never. Stubs are supposed to make things more robust, better and easier, not more annoying, right? So if there is a legitimate number of people who want to see WP_Widget removed, I think we should do it.

[szepeviktor]

I'm not sure what the point is in putting die( 'Function WP_Widget::widget() must be overridden in a subclass. ); in a method that is supposed to be overridden.

This is simple.
In PHP frameworks these must-implement methods thrown an exception. So if you don't implement them you get an exception.
Another approach is to use interfaces.
But WordPress is script-like, not built on top of a framework.

[IanDelMar]

I've just seen the abandon warning 😧

[szepeviktor]

I've just seen

Here is a special viktor-smiley for you

"

# ~~`"`~~

[TobiasBg]

Hi @szepeviktor,

thanks for your hard work on this, it is highly appreciated!

This seems to be caused issues for me as well. In particular I now get
Method XYZ::prepare_items() should always throw an exception or terminate script execution but doesn't do that.
errors which can't be ignored by PHPStan (neither via @phpstan-ignore-line nor the ignoreErrors directive in the phpstan.neon.dist. I now have to exclude the affected files from checking, as PHPStan now tells me
Error message "Method XYZ::prepare_items() should always throw an exception or terminate script execution but doesn't do that." cannot be ignored, use excludePaths instead., which obviously is not really a good thing to do.
Do you happen to know other workarounds?

[szepeviktor]

Hello Tobias! 👋🏻
A special workaround is ongoing. I will abandon all my WordPress-related packages/repositories.

We could have an exception list for this new feature.
#110 (comment)
Or simply hardcode WP_Widget in isNeverReturn.
Would you send a PR?

[IanDelMar]

If "function Class_Name::methode_name() must be overridden in a subclass." is the standard way of dying for methods that must be overridden, we can exclude all those dying methods.

[IanDelMar]

These are affected:

• WP_List_Table::ajax_user_can()
• WP_List_Table::prepare_items()
• WP_List_Table::get_columns()
• WP_Widget::widget()

[TobiasBg]

@szepeviktor, I totally understand that you are frustrated, and I'm really sorry about that.
I have only come to learn about your packages recently, when I started using PHPStan (and thanks again for helping me already with PRs!). And I have seen your search for sponsors in the phpstan-wordpress repo and will look into getting that set up tomorrow!

[szepeviktor]

I have seen your search for sponsors

Thank you for your helpful attitude.
Actually I am looking for the joy of work. If you get me money I will buy work with it.

[Screenfeed]

We seem to have the same issue some are having here.

Error: Return type (void) of method PLL_Widget_Languages::widget() should be compatible with return type (never) of method WP_Widget::widget()
Error: Return type (array<string>) of method PLL_Table_Languages::get_columns() should be compatible with return type (never) of method WP_List_Table::get_columns()
Error: Return type (void) of method PLL_Table_Languages::prepare_items() should be compatible with return type (never) of method WP_List_Table::prepare_items()

So the current solution is to add these to the ignored errors? (which is weird since #133 seems to fix this issue 🤔)

Note: I try to spread the word about potential sponsors, I've seen your notice yesterday.

[IanDelMar]

which is weird since #133 seems to fix this issue 🤔

#133 is in the master, but not yet released. In your composer.json, replace the version with "dev-master":

"php-stubs/wordpress-stubs": "dev-master"

[Screenfeed]

🤦
Thanks.