WordPress.org

Make WordPress Core

Opened 6 months ago

Last modified 8 weeks ago

#41121 reviewing task (blessed)

Consistency of the _deprecated_function() calls and related documentation.

Reported by: jrf Owned by: DrewAPicture
Milestone: 5.0 Priority: normal
Severity: normal Version: 4.9
Component: General Keywords: has-patch
Focuses: docs Cc:

Description

While working on updating the `WP.DeprecatedFunctions` sniff which is part of the WordPress Coding Standards project, we've come across a number of inconsistencies in the application of the function and/or the related documentation.

@jdgrimes previously already reported this concerning three functions in #41094 (has its own patch in that ticket), however since then a lot more inconsistencies have been identified.

In a series of patches which I will add to this ticket I will address these:

Patch 1: Fix deprecation info in various docblocks

  • Fix incorrect deprecation comment naming the deprecated function as its own alternative
  • Fix typo in docblock @deprecated tag which meant that deprecation is not indicated on the dev ref website
  • Add missing @deprecated tags for two functions
  • Add missing alternative in @deprecated comment for wp_get_sites()

Patch 2: Add missing replacement functions to various _deprecated_function() calls

  • Add missing parentheses to a few deprecated function replacement function strings to be consistent with how this is done everywhere else.
  • Add missing alternative for the WP_Customize Widget deprecated methods.
  • Add missing alternative for three deprecated ms functions
  • Add missing alternative for deprecated wp_cache_reset() function

Patch 3: Add missing _deprecated_function() function calls to various functions

  • Add missing _deprecated_function() call for funky_javascript_callback()
  • Add missing _deprecated_function() call for _save_post_hook().
  • Add missing _deprecated_function() call for default_topic_count_text().
  • Add missing _deprecated_function() call for screen_meta().
  • Add missing _deprecated_function() call for two screen icon related functions
  • Add missing _deprecated_function() call for nine wp_dashboard related functions
  • Add missing _deprecated_function() call for wp_ajax_wp_fullscreen_save_post().
  • Add missing _deprecated_function() call for ms_deprecated_blogs_file()

Patch 4: Add missing _deprecated_function() calls to various deprecated class methods

  • Add _deprecated_function() calls to all methods within the deprecated WP_User_Search class.
  • Add missing _deprecated_function() call for four methods in WP_Customize_Image_Control

Patch 5: Add missing _deprecated_function() calls for four deprecated pluggable functions.

Not all functions complied with what the docblock at the top of the files states:

Deprecated warnings are also thrown if one of these functions is being defined by a plugin.

Attachments (5)

0001-docblock-fixes.patch (3.1 KB) - added by jrf 6 months ago.
Patch 1
0002-calls-missing-replacement-functions.patch (5.8 KB) - added by jrf 6 months ago.
Patch 2
0003-missing-_deprecated_function-calls.patch (6.0 KB) - added by jrf 6 months ago.
Patch 3
0004-missing-_deprecated_function-calls-methods.patch (4.4 KB) - added by jrf 6 months ago.
Patch 4
0005-missing-_deprecated_function-calls-pluggable.patch (1.8 KB) - added by jrf 6 months ago.
Patch 5

Download all attachments as: .zip

Change History (38)

@jrf
6 months ago

Patch 1

#1 @jrf
6 months ago

  • Keywords has-patch added

#2 @DrewAPicture
6 months ago

  • Owner set to DrewAPicture
  • Status changed from new to reviewing

This ticket was mentioned in Slack in #core-coding-standards by netweb. View the logs.


6 months ago

#4 @netweb
6 months ago

  • Milestone changed from Awaiting Review to 4.9
  • Type changed from defect (bug) to task (blessed)

Related: #41057 Update PHP codebase per WordPress PHP Coding Standards

Moving to 4.9 and 'switching to blessing task, this ticket is required as part of #41057 and is requires committing before #41057 can be committed.

Last edited 6 months ago by netweb (previous) (diff)

#5 @jdgrimes
6 months ago

Note that some of these functions may intentionally not contain the _deprecated_function() call, for various reasons. I think I remember when screen_icon() was deprecated, in particular, that the decision was made not to add the notice. Might be worth investigating some of these to see if there was a reason that the _deprecated_function() call was omitted, and if it still holds.

#6 @jrf
6 months ago

this ticket is required as part of #41057 and is requires committing before #41057 can be committed.

Sorry, I need to correct you there @netweb.

This ticket is unrelated to ticket #41057. The sniff involved does not contain any auto-fixers, so will not impact the efforts for #41057.

Apart from that, we can update the sniff independently of whether or not this has been fixed in core yet, so these two tickets should really be seen independently of each other.

I have opened a related issue in WPCS to start detecting these type of inconsistencies automatically:
https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/993
This is currently not (yet) done. Whether the new sniff which would be written for this, would contain any auto-fixers remains to be seen.

Independently of all that, the WP.Deprecated... sniffs are currently not part of the WPCS WordPress-Core ruleset. They are only included in the WordPress-Extra ruleset.

Last edited 6 months ago by jrf (previous) (diff)

#7 @jrf
6 months ago

Related: #41125

Last edited 8 weeks ago by SergeyBiryukov (previous) (diff)

#8 @DrewAPicture
6 months ago

In 40922:

Docs: Standardize and add missing deprecation notations in DocBlocks for the following functions:

  • post_form_autocomplete_off()
  • _rotate_image_resource()
  • _flip_image_resource()
  • wp_get_sites()
  • deactivate_sitewide_plugin()

Props jrf.
See #41121.

#9 @DrewAPicture
6 months ago

@jrf I'm gonna leave the additions in class-wp-customizer-widgets.php for 0002-calls-missing-replacement-functions.patch out right now as I've asked for a second opinion on whether the quotes should be encoded or not.

They'll go in either way, just want to get a consensus – hard-escaping quotes is mostly a no-no in any kind of string that core passes around, in my experience. :-)

#10 follow-up: @jrf
6 months ago

@DrewAPicture I've based my usage of escaped quotes on the precedent for this as set in numerous other function calls to _deprecated_function(), though there is no real consensus as current usage is inconsistent.

Escaped quotes:

Non-escaped quotes:

#11 @DrewAPicture
6 months ago

I talked to a few people about it, and @SergeyBiryukov pointed out that if we just reference the hook name as the replacement, it'll output to the log as "Use 'customize_dynamic_setting_args' instead", which I agree is clear enough. I'll fix it on commit.

#12 @jrf
6 months ago

@DrewAPicture I changed it to explicitely mention the fact it is a filter to distinguish it from the expected/most common alternative being an alternative function.

The _deprecated_function() documentation even explicitly mentions that a function is expected as the replacement: https://developer.wordpress.org/reference/functions/_deprecated_function/

There are a few more exceptions where something else is used (not a function) and those are generally made more explicit as well.

See:

#13 in reply to: ↑ 10 @SergeyBiryukov
6 months ago

Replying to jrf:

I've based my usage of escaped quotes on the precedent for this as set in numerous other function calls to _deprecated_function(), though there is no real consensus as current usage is inconsistent.

All of those cases are pieces of code, not strings. In this case, we're trying to slot some hardcoded English text into a translatable string, which seems less than ideal and doesn't have a precedent AFAIK.

With _deprecated_function( __METHOD__, '4.2.0', 'customize_dynamic_setting_args' ), the resulting message would include Use 'customize_dynamic_setting_args' instead, which seems clear enough. It's a developer notice, and developers should be able to figure out that it's a filter, not a function.

#14 @SergeyBiryukov
6 months ago

In this case, we're trying to slot some hardcoded English text into a translatable string, which seems less than ideal and doesn't have a precedent AFAIK.

We could probably do something like this, but it still seems unnecessary complicated to me:

_deprecated_function( __METHOD__, '4.2.0', sprintf(
	/* translators: %s: 'customize_dynamic_setting_args' */
	__( 'the %s filter' ),
	'customize_dynamic_setting_args'
) );

#15 @DrewAPicture
6 months ago

In 40929:

Improve the usefulness of several _deprecated_function() calls by passing known replacement functions, methods, or hooks.

Props jrf.
See #41121.

#16 @DrewAPicture
6 months ago

On 0003-missing-_deprecated_function-calls.patch:

  • Opened #41153 to address formally deprecating screen_icon() and get_screen_icon().
  • Followed up on hard-deprecation of wp_ajax_wp_fullscreen_save_post() in 16:ticket:30949.
  • Followed up on hard-deprecation of funky_javascript_callback() in 5:ticket:12520.
  • Followed up on hard-deprecation of _save_post_hook() in 27:ticket:11399
  • Followed up on hard-deprecation default_topic_count_text() in 3:ticket:27262.
  • Followed up on hard-deprecation of all of the dashboard widget callbacks in 42:ticket:25824
  • Followed up on hard-deprecation of ms_deprecated_blogs_file() in 67:ticket:19235
Last edited 6 months ago by DrewAPicture (previous) (diff)

#17 follow-up: @DrewAPicture
6 months ago

@westonruter Looking for feedback on "hard-deprecating" the prepare_controls(), add_tab(), remove_tab(), and print_tab_image() methods in `WP_Customize_Image_Control. See 0004-missing-_deprecated_function-calls-methods.patch. They were deprecated in 4.1.

#18 follow-up: @DrewAPicture
6 months ago

@boonebgorges Seeking feedback on hard-deprecation of individual methods within WP_User_Query, which was itself deprecated in 3.1.

Separately, #41125 has been opened, which would allow for drawing a distinction between the class itself being deprecated from its methods (though, realistically, if the class is deprecated, do we really need to also mark the individual methods as deprecated?). Interested in your perspective here. See 0004-missing-_deprecated_function-calls-methods.patch.

#19 @DrewAPicture
6 months ago

@nacin Beside the individual functions I asked for follow up on in 0003, I'd be interested in your overall take on 0005-missing-_deprecated_function-calls-pluggable.patch. I think ultimately the goal here is to introduce a sense of consistency in how we actually deprecate stuff.

#20 @jrf
6 months ago

@DrewAPicture

(though, realistically, if the class is deprecated, do we really need to also mark the individual methods as deprecated?).

IMHO, we should.

As the (deprecated) class is not marked as final, it could be extended and the parent methods could be used without calling the parent _construct().
A dev will in that case not be informed of the class deprecation as things stand at this moment.

<?php
class My_User_Search extends WP_User_Search {
        public function __construct() {
                // Set things up differently from the parent.
        }

        public function prepare_query() {
                parent::prepare_query();
                $this->query_where = 'something else';
        }
}

#21 in reply to: ↑ 17 @westonruter
6 months ago

Replying to DrewAPicture:

@westonruter Looking for feedback on "hard-deprecating" the prepare_controls(), add_tab(), remove_tab(), and print_tab_image() methods in `WP_Customize_Image_Control. See 0004-missing-_deprecated_function-calls-methods.patch. They were deprecated in 4.1.

It makes sense to me. Note that the prepare_controls method is being called in \WP_Customize_Header_Image_Control::enqueue(), so that would need to be removed. It probably wouldn't cause issues for any plugins that extend WP_Customize_Header_Image_Control, which I hope there are none.

#22 @DrewAPicture
6 months ago

In 40938:

Scripts: Correctly reference WP_Scripts::print_extra_script() as the replacement for the deprecated WP_Scripts::print_scripts_l10n() method, rather than a nonexistent global print_extra_script() function.

Props fergbrain.
See #41121.

#23 @DrewAPicture
6 months ago

In 40949:

Docs: Notate format_for_editor() as the replacement function in the DocBlock for the deprecated wp_richedit_pre().

Props rabmalin.
See #41121. Fixes #40709.

#24 in reply to: ↑ 18 @boonebgorges
6 months ago

Replying to DrewAPicture:

@boonebgorges Seeking feedback on hard-deprecation of individual methods within WP_User_Query, which was itself deprecated in 3.1.

Separately, #41125 has been opened, which would allow for drawing a distinction between the class itself being deprecated from its methods (though, realistically, if the class is deprecated, do we really need to also mark the individual methods as deprecated?). Interested in your perspective here. See 0004-missing-_deprecated_function-calls-methods.patch.

"Hard" (maybe better to say "explicit" or "complete"?) deprecation seems fine to me, and has the advantage of being consistently explicit. Can you say a bit more about why this feels redundant to you? Is it because of the additional lines of code
(the _deprecated_function() calls themselves) and the maintenance burden they may represent? Or is it because of some redundancy it'd introduce in the autogenerated documentation?

#25 follow-up: @DrewAPicture
4 months ago

@boonebgorges I've changed my opinion on whether to also explicitly deprecate the methods in WP_User_Search. In terms of the _deprecated_function() calls, do you think we should try to match up method:method where possible or is it enough to just point to WP_User_Query as in 0004-missing-_deprecated_function-calls-methods.patch?

#26 @DrewAPicture
4 months ago

In 41273:

Docs: Explicitly deprecate the add_tab(), remove_tab(), and print_tab_image() methods for WP_Customize_Image_Control, originally soft-deprecated in 4.1.

Props jrf.
See #41121.

#27 @DrewAPicture
4 months ago

@jrf Looking at 0005-missing-_deprecated_function-calls-pluggable.patch, I think I'm actually going to push back on this one due to the nature of certain functions being considered "pluggable".

It's quite possible that we could introduce unintended consequences by globally deprecating these pluggable functions – as the core versions are only in play if not already defined.

I don't think it's unreasonable to believe that extensions have created versions of these functions in the wild that may possibly have nothing to do with the purposes they were originally intended to serve.

#28 @jrf
4 months ago

@DrewAPicture In that case, I believe the documentation at the top of the file should be adjusted to reflect that.

It currently states:

Deprecated warnings are also thrown if one of these functions is being defined by a plugin.

https://core.trac.wordpress.org/browser/trunk/src/wp-includes/pluggable-deprecated.php#L7

I would also like to raise a vote for consistency. Either have all of the deprecated pluggable functions throw a deprecation warning - even when redefined -. Or have none of them throw the deprecation warning when redefined. Not half/half like now.

#29 in reply to: ↑ 25 @boonebgorges
4 months ago

Replying to DrewAPicture:

@boonebgorges I've changed my opinion on whether to also explicitly deprecate the methods in WP_User_Search. In terms of the _deprecated_function() calls, do you think we should try to match up method:method where possible or is it enough to just point to WP_User_Query as in 0004-missing-_deprecated_function-calls-methods.patch?

This looks perfect to me.

This ticket was mentioned in Slack in #core by melchoyce. View the logs.


2 months ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


8 weeks ago

#32 @westonruter
8 weeks ago

  • Milestone changed from 4.9 to 5.0

This isn't getting traction during 4.9 beta, so punting for now.

#33 @DrewAPicture
8 weeks ago

Thanks @westonruter. Hopefully we can finish it up for 5.0.

Note: See TracTickets for help on using tickets.