Make WordPress Core

Opened 15 years ago

Closed 8 years ago

Last modified 7 years ago

#10441 closed feature request (fixed)

Show warning when deprecated hook is registered

Reported by: sirzooro's profile sirzooro Owned by: boonebgorges's profile boonebgorges
Milestone: 4.6 Priority: normal
Severity: normal Version:
Component: Plugins Keywords: dev-feedback has-patch
Focuses: Cc:

Description

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.

Attachments (9)

10441-new-functions.php (2.5 KB) - added by solarissmoke 13 years ago.
Proposed new functions for handling deprecated hooks
10441.do_deprecated_action.patch (11.4 KB) - added by SergeyBiryukov 13 years ago.
10441.do_deprecated_action.2.patch (11.5 KB) - added by SergeyBiryukov 13 years ago.
Updated @since and expanded [sic] comments
10441.diff (3.9 KB) - added by DrewAPicture 9 years ago.
Refresh + docs
10441.2.diff (8.5 KB) - added by boonebgorges 8 years ago.
deprecated-hooks.txt (8.7 KB) - added by boonebgorges 8 years ago.
10441.3.diff (453 bytes) - added by flixos90 8 years ago.
10441.4.diff (4.5 KB) - added by flixos90 8 years ago.
trac-10441-change-over-existing-deprecated-hooks.6.patch (23.6 KB) - added by jrf 8 years ago.

Download all attachments as: .zip

Change History (61)

#1 @sirzooro
15 years ago

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',

#2 @ryan
14 years ago

  • Milestone changed from 2.9 to Future Release

#3 @scribu
14 years ago

  • Keywords needs-patch added

+1 on the idea.

#4 @nacin
14 years ago

  • Cc skeltoac added

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.

#5 @solarissmoke
13 years ago

  • Keywords dev-feedback added

@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?

#6 @scribu
13 years ago

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

@solarissmoke
13 years ago

Proposed new functions for handling deprecated hooks

#7 @solarissmoke
13 years ago

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

#8 @westi
13 years ago

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

#9 @nacin
13 years ago

Posted a patch to #17559.

#10 @nacin
13 years ago

  • Keywords 3.3-early added
  • Owner set to nacin
  • Status changed from new to accepted

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

#11 @SergeyBiryukov
13 years ago

  • Keywords has-patch added; needs-patch removed

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.

#12 @scribu
13 years ago

What are the [sic]s for?

#13 @SergeyBiryukov
13 years ago

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
13 years ago

Updated @since and expanded [sic] comments

#14 @emhr
12 years ago

  • Cc gene@… added
  • Summary changed from Show warning when deprecated hook is registered to emhr

#15 @emhr
12 years ago

  • Summary changed from emhr to Show warning when deprecated hook is registered

#16 @SergeyBiryukov
11 years ago

#24259 was marked as a duplicate.

#17 @nacin
11 years ago

#24429 was marked as a duplicate.

#18 @ryanve
11 years ago

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

#19 @toscho
11 years ago

  • Cc info@… added

#20 @chriscct7
9 years ago

  • Keywords 3.3-early removed

#21 @DrewAPicture
9 years ago

  • Owner changed from nacin to DrewAPicture

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


9 years ago

@DrewAPicture
9 years ago

Refresh + docs

#23 @DrewAPicture
8 years ago

#35818 was marked as a duplicate.

#24 @boonebgorges
8 years ago

#34195 depends on this ticket.

@boonebgorges
8 years ago

#25 @boonebgorges
8 years ago

  • Milestone changed from Future Release to 4.6
  • Owner changed from DrewAPicture to 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.

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


8 years ago

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


8 years ago

#28 @DrewAPicture
8 years ago

@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?

#29 @boonebgorges
8 years ago

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.

#30 @DrewAPicture
8 years ago

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
/**
 * Some now-deprecated hook.
 *
 * @since 2.6.0
 * @deprecated 4.6.0 Use 'replacement_hook' instead.
 * @see 'replacement_hook'

Should output something like this in the Code Reference page:

http://cl.ly/1S1p3f2M3s0E/Screen%20Shot%202016-06-11%20at%2010.07.21%20AM.png

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.

Last edited 8 years ago by DrewAPicture (previous) (diff)

#31 @DrewAPicture
8 years ago

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.

#32 @DrewAPicture
8 years ago

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

#33 @DrewAPicture
8 years ago

PR created for the parser changes: #178.

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


8 years ago

This ticket was mentioned in Slack in #accessibility by cheffheid. View the logs.


8 years ago

#36 @boonebgorges
8 years ago

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

#37 @DrewAPicture
8 years ago

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

#38 @boonebgorges
8 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

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


8 years ago

#40 follow-up: @TimothyBlynJacobs
8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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
8 years ago

#41 in reply to: ↑ 40 @flixos90
8 years ago

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.

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


8 years ago

@flixos90
8 years ago

#43 @flixos90
8 years ago

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.

#44 @ocean90
8 years ago

In 37909:

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

See #10441.

#45 @ocean90
8 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 37911:

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

Props flixos90.
Fixes #10441.

#46 @jrf
8 years ago

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

#47 @DrewAPicture
8 years ago

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

#48 follow-up: @jrf
8 years ago

@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.

#49 in reply to: ↑ 48 @DrewAPicture
8 years ago

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.

#50 @jrf
8 years ago

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.

This ticket was mentioned in Slack in #docs by drew. View the logs.


8 years ago

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


7 years ago

Note: See TracTickets for help on using tickets.