Show warning when deprecated hook is registered

[sirzooro]

At this moment WP shows warning when someone tries to use deprecated function or file. It will be good to do the same for deprecated hooks. My suggestion is to do this check in add_action()/add_filter() functions. They should compare hook name against list of deprecated ones and show warning if necessary.

[sirzooro]

I have performed search for deprecated hooks, and found these ones and their replacements:

'tiny_mce_version' => 'mce_external_plugins',
'private_to_published' => 'private_to_publish',
'rewrite_rules' => 'mod_rewrite_rules',
'retreive_password' => 'retrieve_password',

[scribu]

+1 on the idea.

[nacin]

I've been looking into this for some time and started on a patch. I never noticed this ticket, thanks scribu for finding it.

Adding Andy to CC as we spoke about this in IRC yesterday.

[solarissmoke]

@nacin, are you still working on a patch? :P

I was thinking that rather than check for deprecated actions every time do_action is run, we might introduce functions like do_deprecated_action() / apply_deprecated_filter() which issue a warning and then run the action. Thoughts?

[scribu]

@solarissmoke: Yes, that seems like a much more efficient way to do it.

[solarissmoke]

Proposed new functions for handling deprecated hooks

[solarissmoke]

Here's an attachment with what I had in mind..

[westi]

Cross referencing with #12913 which has been closed as a dupe of this ticket

[nacin]

Posted a patch to #17559.

[nacin]

Some more patches on #17559. We'll do this in 3.3.

[SergeyBiryukov]

FWIW, I like ticket:17559:do_deprecated_action.diff​, so I refreshed it.

Replying to nacin:

Still need someone to track down version numbers for a number of hooks.

Done in 10441.do_deprecated_action.patch​, also fixed order of arguments in several instances.

[scribu]

What are the [sic]s for?

[SergeyBiryukov]

They're from the original patch. My first thought was that double quotes should be used there, but apparently this is not the case. So I guess it's to prevent confusion. Maybe a more descriptive comment would be helpful though.

[SergeyBiryukov]

Updated @since and expanded [sic] comments

[SergeyBiryukov]

#24259 was marked as a duplicate.

[nacin]

#24429 was marked as a duplicate.

[ryanve]

I like the idea here, but I think it'd be better implemented as described in #24429

[DrewAPicture]

Refresh + docs

[DrewAPicture]

#35818 was marked as a duplicate.

[boonebgorges]

#34195 depends on this ticket.

[boonebgorges]

In addition to #34195, there are a number of hooks in WP that are documented as deprecated where this could be used. So let's put it in.

10441.2.diff​ is a refresh makes the following changes:

• Add unit tests.
• Use do_action_ref_array() and apply_filters_ref_array() in the wrappers, to simplify the syntax.
• I changed the names of the functions from do_deprecated_action() to do_action_deprecated(). IMO this makes it clearer that the function is related to do_action(). And in the case of apply_filters(), it feels more semantically correct - filters (ie callbacks) are being applied to a value; it's not the callbacks that we're deprecating, it's the use of the hook. Anyway, I don't care very much about this, so if anyone feels strongly the other way, it can be changed back.
• Added the deprecated_hook_run action for getting a stack trace. The previous patch mentioned an action deprecated_hook_used for this purpose but didn't define it. I understand why one think used is more precise, but since all the other _deprecated functions have _run hooks, we should do it here too.

Feedback welcome.

[DrewAPicture]

@boonebgorges: I like this.

Do we have any idea of the scope this would be applied in the current core landscape? I know there's a few "deprecated" hooks scattered about. And how would you propose documenting these new function calls in-place? Simply adding @deprecated tags in the hook docs?

[boonebgorges]

Thanks for chiming in, @DrewAPicture.

Do we have any idea of the scope this would be applied in the current core landscape? I

deprecated-hooks.txt​ is a rough list of existing hooks marked @deprecated. ($ pcregrep -nrM "\@deprecated[^;]+(apply_filters|do_action)" src/ > ~/deprecated-hooks.txt, with a bit of formatting cleanup). I don't think it's necessarily the case that we need to start throwing notices in all these cases - as in the case of _deprecated_function() and friends, we should be judicious - but it's a starting place.

And how would you propose documenting these new function calls in-place? Simply adding @deprecated tags in the hook docs?

Good question. My suggestion is to keep the inline hook docs when deprecating, so that

	/**
	 * Filter the fields to select in the terms query.
	 *
	 * Field lists modified using this filter will only modify the term fields returned
	 * by the function when the `$fields` parameter set to 'count' or 'all'. In all other
	 * cases, the term fields in the results array will be determined by the `$fields`
	 * parameter alone.
	 *
	 * Use of this filter can result in unpredictable behavior, and is not recommended.
	 *
	 * @since 2.8.0
	 *
	 * @param array $selects    An array of fields to select for the terms query.
	 * @param array $args       An array of term query arguments.
	 * @param array $taxonomies An array of taxonomies.
	 */
	$fields = implode( ', ', apply_filters( 'get_terms_fields', $selects, $args, $taxonomies ) );

becomes

	/**
	 * Filter the fields to select in the terms query.
	 *
	 * Field lists modified using this filter will only modify the term fields returned
	 * by the function when the `$fields` parameter set to 'count' or 'all'. In all other
	 * cases, the term fields in the results array will be determined by the `$fields`
	 * parameter alone.
	 *
	 * Use of this filter can result in unpredictable behavior, and is not recommended.
	 *
	 * @since 2.8.0
	 *
	 * @param array $selects    An array of fields to select for the terms query.
	 * @param array $args       An array of term query arguments.
	 * @param array $taxonomies An array of taxonomies.
	 */
	$fields = implode( ', ', apply_filters_deprecated( 'get_terms_fields', array( $selects, $args, $taxonomies ), '4.6.0' ) );

I presume that this will mean an update to phpdoc-parser, so that it knows (a) to look for the new function names, (b) that they count as "hooks". Presumably the parser should also (c) recognize that the get_terms_fields hook in the context of apply_filters_deprecated() is *the very same hook* as in the context of apply_filters(), so that any metadata (like user-submitted notes) are not lost when the filter is deprecated.

I've started looking at phpdoc-parser, and it looks like (a) and (b) should be relatively easy to implement. It looks like (c) should automatically be the case - "hooks" are a single post type, identified by post_name = $hook_name, and the hook name will not be changing on deprecation. The parser should also recognize that a hook is being deprecated, and mark it as such, but at a glance it looks like this may just work too.

Does this all seem correct to you? Your comment reminds me that this is perhaps more of a change from the documentation side than from the core side, so I want to make sure that we get it right from your point of view.

[DrewAPicture]

I think we'd actually do well to also add an @deprecated tag to the hook docs as well, as there's already logic in place to display a callout box on the Code Reference for elements marked as such. I'd have to check, but I think that might be dumb to the post type.

The thinking is that while we could simply map the apply_filters|do_action_deprecated() calls to assign the "deprecated" meta/term for an item, it lowers the maintenance burden to simply deprecate hooks (from a documentation perspective) exactly the same way that we do other elements like functions and methods.

In the case of replacements, we'd need to include that information in the @deprecated and/or additional @see tag so that the deprecation callout knows where to point – again, logic is already in place to handle that, we'd just need to slightly tweak the inline docs standard, e.g.

<?php
 * @deprecated 4.6.0 Use 'replacement_hook' instead.
 * @see 'replacement_hook'

Should output something like this in the Code Reference page:

As for the other changes to the parser, that shouldn't be a problem. I don't any issue with adjusting it to route more functions to the hook post type.

I'll get started on a corresponding meta ticket for any changes that need to happen in the wporg-developer theme and a PR for the parser. The nice part is that we don't re-parse until after release so we'd have some time to get the meta/parser changes in place.

[DrewAPicture]

At this point, it would be a good idea to loop @coffee2code in. He might have some insight as he built most of the wporg-developer logic and is pretty familiar with the parser internals.

[DrewAPicture]

Created ​#meta1766 to cover the wporg-developer (DevHub theme) end.

[DrewAPicture]

PR created for the parser changes: ​#178.

[boonebgorges]

@DrewAPicture Should we wait for anything on the dotorg/docs side before pulling the trigger in trunk?

[DrewAPicture]

@boonebgorges Nope, merge away! This would be considered the first step, the others will follow along behind.

[boonebgorges]

In 37861:

Allow action and filter hooks to be deprecated.

When a filter or action hook is deprecated, the corresponding apply_filters()
or do_action() calls should be switched out with apply_filters_deprecated()
or do_action_deprecated(). The latter functions will throw a deprecation
before invoking the original hook.

Props solarissmoke, SergeyBiryukov, DrewAPicture.
Fixes #10441.

[TimothyBlynJacobs]

I think there is an issue with the current version with apply_filters_deprecated.

If no hook is registered for that tag, the function bails early and returns null instead of the original value. Shouldn't it return the first $args?

[flixos90]

Replying to TimothyBlynJacobs:

I think there is an issue with the current version with apply_filters_deprecated.

If no hook is registered for that tag, the function bails early and returns null instead of the original value. Shouldn't it return the first $args?

I agree, that is definitely a bug to me. Fixed it in 10441.3.diff​.

[flixos90]

In 10441.4.diff​ I added an additional unit test for the above bug. I also moved the unit tests for apply_filters_deprecated into the filters.php tests file as this seems to be more appropriate I think.

[ocean90]

In 37909:

Tests: After [37861] move tests for deprecated filters into filters.php.

See #10441.

[ocean90]

In 37911:

Plugins: Return the original value in apply_filters_deprecated() if no filter is registered for the tag.

Props flixos90.
Fixes #10441.

[jrf]

All: I've got a patch ready which changes over existing deprecated hooks to use the new functions and includes updated documentation with @see tags. Should I upload it here or should I open a new ticket for this ?

/cc @DrewAPicture

[DrewAPicture]

@jrf Create new tickets for deprecating individual hooks, please :-)

[jrf]

@DrewAPicture Oh. That's unexpected. A new ticket for each hook ? That's something like fifty tickets. Sorry. In that case, I pass. Got better things to do with my time.

All these hooks are already marked as deprecated, all the patch is about is switching them over to the new functions.

Anyways, thanks for the reply.

[DrewAPicture]

Replying to jrf:

@DrewAPicture Oh. That's unexpected. A new ticket for each hook ? That's something like fifty tickets. Sorry. In that case, I pass. Got better things to do with my time.

All these hooks are already marked as deprecated, all the patch is about is switching them over to the new functions.

Anyways, thanks for the reply.

I would say we could do a ticket per file or component, but yes, marking something deprecated in an inline comment and actually deprecating it require different levels of discussion and approval.

Doing huge tickets covering a lot of independent elements can easily get confusing both in present and past tense when somebody comes along later wondering why something was changed.

[jrf]

marking something deprecated in an inline comment and actually deprecating it require different levels of discussion and approval.

That does make sense.

Anyway - as discussed on Slack, I'm uploading the complete patch here to be available for splitting up. It touches 15 files in total.