WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#11386 closed enhancement (fixed)

New _deprecated_argument() function

Reported by: nacin Owned by: westi
Milestone: 3.0 Priority: normal
Severity: normal Version:
Component: Inline Docs Keywords: has-patch commit
Focuses: Cc:

Description

Started in #11028.

Patch with the ones I've found.

Most of these have no documentation on their original use, so it can't hurt to remove those. But even for those that do have a hint on what the argument used to do, I'm not sure keeping it is worth it.

The changes to deprecated.php functions would conflict with a patch in #7493, so I'm leaving that file out of this patch and will re-patch #7493.

Attachments (11)

11386-deprecated-args.diff (10.1 KB) - added by nacin 4 years ago.
function-deprecated_argument.diff (2.5 KB) - added by nacin 4 years ago.
New function, _deprecated_argument( $function, $argument, $version, $replacement = null )
function-deprecated_argument-sample-uses.diff (4.3 KB) - added by nacin 4 years ago.
Sample uses of _deprecated_argument()
_deprecated_argument_in-the-wild.diff (10.3 KB) - added by nacin 4 years ago.
_deprecated_argument() for the rest of core.
_deprecated_argument_in-the-wild.2.diff (10.3 KB) - added by nacin 4 years ago.
Use this instead. Small change in trackback_url()
11386.6.diff (15.1 KB) - added by nacin 4 years ago.
Simplified _deprecated_argument()
11386.7.diff (15.1 KB) - added by nacin 4 years ago.
Simplified _deprecated_argument() - sans typos.
11386.8.diff (9.3 KB) - added by nacin 4 years ago.
All remaining missing version numbers
11386.9.diff (15.3 KB) - added by nacin 4 years ago.
All remaining missing version numbers + removing deprecated argument from xfn_check() calls
11386.10.diff (20.0 KB) - added by nacin 4 years ago.
Patch 9, plus $version added to the three deprecated_* actions.
11386_version_arg_move.diff (1.4 KB) - added by strider72 4 years ago.
Moves $version argument to end

Download all attachments as: .zip

Change History (56)

nacin4 years ago

comment:1 follow-ups: strider724 years ago

A potential problem with this is if a new argument is later added to the function. Then you could have old plugins passing data to the missing parameter, but the new version of the function accepting it as the new parameter. Might mess things up.

comment:2 Denis-de-Bernardy4 years ago

  • Keywords close added

agreeing with strider.

comment:3 in reply to: ↑ 1 ; follow-ups: filosofo4 years ago

Replying to strider72:

A potential problem with this is if a new argument is later added to the function. Then you could have old plugins passing data to the missing parameter, but the new version of the function accepting it as the new parameter. Might mess things up.

Actually, in many cases this is something we want to happen.

Take one of the functions in the patch, get_the_author(). Its argument, "$idnode," was deprecated in WordPress 2.0.5 ($idnode let you specify what you wanted as a return value, such as "nickname" or the like).

2.0.5 was released over 3 years ago. The soonest get_the_author() could get a new argument would be in 3.0, which will probably happen summer 2010. Is anyone seriously going to argue that we should coddle plugins and themes calling get_the_author() with an argument that's been deprecated for 3 and a half years?

But get_the_author() (and like functions) do need an argument, and that--logically and consistent with other WP functions--is the author's ID.

comment:4 in reply to: ↑ 3 strider724 years ago

Replying to filosofo:

Actually, in many cases this is something we want to happen.
[...]
Is anyone seriously going to argue that we should coddle plugins and themes calling... an argument that's been deprecated for 3 and a half years?

Those are two different arguments: not coddling vs. desired behavior. Things breaking is never *desired* behavior, to my mind.

Not arguing that the "don't coddle" argument is wrong, per se, just that breaking things in the name of "tough love" is not the same as "we *want* it to break".

comment:5 in reply to: ↑ 3 filosofo4 years ago

Sorry for not being clear. The antecedent for "something we want to happen" is "a new argument is later added to the function," not "mess things up."

I think the number of things potentially messed up is vanishingly small, while the benefit of such new arguments is large. So I'm advocating something positive, not wanton destruction or even a significant amount of "tough love."

comment:6 strider724 years ago

Here's an idea -- How about adding a "deprecated" flag when deprecated parameters are used?

function( $deprecated=null ) {

if ($deprecated) { _deprecated_parameter() }

}

I forget the "flag deprecated" function off the top of my head, but I think you get the point. Perhaps we need a new one to go with "deprecated function" and "deprecated file" functions...?

Then as will functions and files, we can eventually clear these away with a clear conscience.... :-)

comment:7 in reply to: ↑ 1 nacin4 years ago

Replying to strider72:

A potential problem with this is if a new argument is later added to the function. Then you could have old plugins passing data to the missing parameter, but the new version of the function accepting it as the new parameter. Might mess things up.

I hadn't considered that, and I wanted to close as wontfix once I saw it.

At the very least, I am all for a _deprecated_argument() function. This is important, as while a plugin developer may notice changes to functions over the course of a new version, they might not realize an argument they were using no longer works. For the most part, they'd have to follow development quite closely to have caught the change.

That said, deprecated files still include the proper file and nearly all deprecated functions have replacements and still perform the proper function. On the other hand, deprecated arguments are entirely useless and no longer serve any purpose in the code. Indeed, there is no longer any documentation for what they were or when they were removed.

So, if we were to re-tailor this to a _deprecated_argument() function, would that have traction?

Also, before I forget and this ticket gets closed, there is actually a functional change to the_author() in this patch, which may have been inadvertent but nonetheless would restore it to proper functionality. If you want the_author() to echo a value, it'll do so, but also return it. It should only do one or the other. Probably could go into another ticket.

comment:8 filosofo4 years ago

I don't object to the deprecated warnings, but it might just be better to address each of these individually.

For example, with get_the_author() we could cast the new author ID argument to integer, and that would effectively ignore any old-style arguments.

comment:9 westi4 years ago

  • Keywords needs-patch added; has-patch close removed
  • Owner set to westi
  • Status changed from new to accepted
  • Type changed from defect (bug) to enhancement

I really like the _deprecated_argument() idea - I thought it up completely independently of this ticket and wrote about it here: #10805

I would also use it in side the back-compat bit of the fix for: #10468

Does someone want to create a patch for that function and a separate patch for how and where we would use it?

nacin4 years ago

New function, _deprecated_argument( $function, $argument, $version, $replacement = null )

nacin4 years ago

Sample uses of _deprecated_argument()

comment:10 nacin4 years ago

  • Keywords needs-review added; needs-patch removed
  • Summary changed from Remove $deprecated = '' arguments that don't affect argument order to New _deprecated_argument() function

Patches added. Some things I came across -

  1. I wished we could just pass the value of the argument to the function and check !empty() there, but there are enough arguments that expect true (many of them in the sample patch), so it would either unnecessarily complicate _deprecated_argument() or result in a hack when calling it: _deprecated_argument(__FUNCTION__, 'deprecated_echo', $deprecated_echo === true ? null : true, '0.0', 'the other function'). I went with manually checking in each function.
  1. I wanted to do ordinal numbers for deprecated arguments (with the option to specify a string argument name as well), as nearly all have been renamed to 'deprecated' (and should stay that way, IMO), such that it says "The third argument of get_option() is deprecated since version 0.0 with no alternative available."

This is good feedback especially since keeping these is partly to maintain argument order if we ever add more arguments. But conditional ordinals complicates translations if not making them impossible in some languages, so I'd like to hear some feedback first. (We could always do "Argument 1 ...") These approaches also might complicate a function running on a hook, as they'll expect an argument name.

  1. Version numbers will require blame. Once reviewed and ready to patch across core, I'll go ahead and look them up.

comment:11 follow-up: westi4 years ago

  • Keywords commit added; needs-review removed

Looks really good.

The only change I think I would make is to change the behaviour of $replacement to be a short message where appropriate rather than a function name or "array".

In most cases there is no message.

In cases like wp_clear_scheduled_hook() we can probably put a short string which explains the change.

e.g.

"This argument has changed to an array() so as to match with the behaviour of all the other cron functions."

comment:12 westi4 years ago

(In [12536]) Add new function _deprecated_argument() to be used for marking arguments as deprecated so that with WP_DEBUG enabled developers can see they need to review and update their code. See #11386 props nacin.

comment:13 in reply to: ↑ 11 nacin4 years ago

Replying to westi:

In cases like wp_clear_scheduled_hook() we can probably put a short string which explains the change.

I love the idea and had only avoided that because it would add more strings for translators, and neither of the other _deprecated_* functions rely on translated strings.

comment:14 westi4 years ago

(In [12537]) Fix typo in _deprecated_argument() and start using _deprecated_argument() in wp-includes files. See #11386 props nacin.

nacin4 years ago

_deprecated_argument() for the rest of core.

comment:15 nacin4 years ago

What should be the final patch is now attached.

nacin4 years ago

Use this instead. Small change in trackback_url()

comment:16 nacin4 years ago

Final final patch. Small change in trackback_url() so there is no change in functionality, but still check for identicality in checking if the deprecated argument was disturbed.

comment:17 follow-up: sirzooro4 years ago

  • Cc sirzooro added

_deprecated_argument() function needs update - you should check if WP_DEBUG is defined first. Without this you will get PHP notice that WP_DEBUG is not defined.

if( defined( 'WP_DEBUG' ) && WP_DEBUG && apply_filters( 'deprecated_argument_trigger_error', true ) ) {

comment:18 in reply to: ↑ 17 ; follow-up: westi4 years ago

Replying to sirzooro:

_deprecated_argument() function needs update - you should check if WP_DEBUG is defined first. Without this you will get PHP notice that WP_DEBUG is not defined.

if( defined( 'WP_DEBUG' ) && WP_DEBUG && apply_filters( 'deprecated_argument_trigger_error', true ) ) {

But WP_DEBUG is always defined.

comment:19 in reply to: ↑ 18 sirzooro4 years ago

Replying to westi:

But WP_DEBUG is always defined.

As I checked, this changed in 2.9. So this is not an issue.

comment:20 filosofo4 years ago

What's the point of the "argument name" argument of the _deprecated_argument() function? In almost all your examples, you pass the string "deprecated" or "deprecated_1" except for one I think where you pass "args."

Is that really of much use? Wouldn't it be good enough to say something like "Warning, function x is being called with a deprecated argument. Instead, use ...."?

comment:21 follow-up: filosofo4 years ago

To elaborate, any sane developer trying to address the use of a deprecated argument is going to look at both the source of the function itself and its use, so even knowing which argument is deprecated isn't going to do much.

It's good enough to throw a warning identifying the function itself.

comment:22 in reply to: ↑ 21 ; follow-up: westi4 years ago

Replying to filosofo:

To elaborate, any sane developer trying to address the use of a deprecated argument is going to look at both the source of the function itself and its use, so even knowing which argument is deprecated isn't going to do much.

It's good enough to throw a warning identifying the function itself.

This sounds like a good improvement and will remove the need for multiple calls when functions have many deprecated arguments.

comment:23 in reply to: ↑ 22 nacin4 years ago

Replying to westi:

This sounds like a good improvement and will remove the need for multiple calls when functions have many deprecated arguments.

Yea, I would tend to agree. Let me grep back through with a new patch.

comment:24 nacin4 years ago

Well, we call it multiple times very rarely -- only twice. Once is easy, but in the_author()'s case, one of them passes a special message. I suppose we could conditionally pass the message as well, such as:

if ( !empty( $deprecated ) || $deprecated_echo !== true )
    _deprecated_argument( __FUNCTION__, '1.5', $_deprecated_echo !== true ? __('Use get_the_author() instead if you do not want the value echoed.') : null );

Perhaps we should consider renaming deprecated arguments to $deprecated_{old_name} in lieu of this, and provide better feedback?

comment:25 nacin4 years ago

Also, different arguments in the same function in the future might be deprecated at different times, also requiring a check for the $version argument to pass what I assume should be the lower number.

comment:26 westi4 years ago

I think we can pick and choose.

Where appropriate call it once and name all the old arguments the same.

Where different messages are needed call it more than once.

nacin4 years ago

Simplified _deprecated_argument()

comment:27 nacin4 years ago

Okay, new patch.

comment:28 nacin4 years ago

  • Keywords has-patch added; commit removed

Two typos in that patch. Another coming up.

nacin4 years ago

Simplified _deprecated_argument() - sans typos.

comment:29 hakre4 years ago

If this gets committed, the following ticket needs some love: #10805.

comment:30 nacin4 years ago

There's a sprintf argument order issue in trunk. More or less, it says "The (function) argument of (argument)" instead of "The (argument) argument of (function)".

Latest patch from a few days ago (patch 7) fixes this anyway by removing the name of the argument from _deprecated_argument()'s parameters.

comment:31 nacin4 years ago

#10805 - Apply _deprecated_argument() to user levels
#11652 - Apply _deprecated_argument() to deprecated get_bloginfo() options

comment:32 westi4 years ago

(In [12584]) Updates and improvements to _depreceated_argument. See #11386 props nacin.

comment:33 westi4 years ago

(In [12584]) Updates and improvements to _depreceated_argument. See #11386 props nacin.

nacin4 years ago

All remaining missing version numbers

comment:34 nacin4 years ago

What should be the final patch is attached.

Some of these are still 0.0 because they're from b2 but were never implemented. A few others were never implemented from when the function was introduced. We could probably strip a few of these but to future proof argument order, we might as well leave them.

comment:35 nacin4 years ago

Oh, also --

Most xfn_check() calls use a deprecated, not used, never implemented argument. Apparently, the XFN meta box has been unusable with WP_DEBUG since _deprecated_argument() was committed, as the notices break up the HTML.

Patch 9 will handle this. I'll roll patch 8 into it.

nacin4 years ago

All remaining missing version numbers + removing deprecated argument from xfn_check() calls

comment:36 strider724 years ago

Nice addition. :-) I'm not sure if this should be a separate ticket or not, but it would be nice if the 'deprecated_argument_run' action also passed the $version. I know you're being consistent with _deprecated_function and _deprecated_file, but frankly it would be nice if those passed it as well.

{{do_action('deprecated_argument_run', $function, $version, $message);}}

As the author of the "Log Deprecated Calls" plugin, I think it would be nice if I could show people the version information along with the rest. Just a thought.

comment:37 follow-up: strider724 years ago

Okay, to be more consistent with the others, that should probably be:

do_action('deprecated_argument_run', $function, $message, $version);

comment:38 in reply to: ↑ 37 nacin4 years ago

Replying to strider72:

Okay, to be more consistent with the others, that should probably be:

do_action('deprecated_argument_run', $function, $message, $version);

It would have to be added to the end for back compat reasons.

I patched this at one point but never got around to creating a ticket for it. I think I figured at the time that I would wait until all version numbers were in. At this point, I've patched all function and argument versions, I just need patches committed here and in #11388.

While I'm here, I should cross-reference #11815 - we need to remove a _deprecated_argument() call because it doesn't work well with a default filter that calls the function.

nacin4 years ago

Patch 9, plus $version added to the three deprecated_* actions.

comment:39 nacin4 years ago

  • Keywords commit added

Okay, new patch that should be a commit candidate.

This patch now passes $version to the three deprecated_* actions. (Inline docs updated as well.)

I've also removed one version number update from here, as that whole _deprecated_argument() call should be removed (see #11815).

comment:40 westi4 years ago

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

(In [12680]) Add missing version numbers to _deprecated_argument() calls.
Remove deprecated argument from xfn_check() calls.
Pass version number to deprecated_file_included, deprecated_function_run and deprecated_argument_run actions.
Fixes #11386 props nacin.

comment:41 follow-up: strider724 years ago

I thought the $version argument was being added to the end of the arguments for backwards compatibility?

Because in trunk WP, $version is the middle of the three, e.g. in _deprecated_function:

$function, $version, $replacement=null

shouldn't that be:

$function, $replacement=null, $version=null

?

comment:42 in reply to: ↑ 41 westi4 years ago

Replying to strider72:

I thought the $version argument was being added to the end of the arguments for backwards compatibility?

Because in trunk WP, $version is the middle of the three, e.g. in _deprecated_function:

$function, $version, $replacement=null

shouldn't that be:

$function, $replacement=null, $version=null

?

$version has always been in that place in the function call arguments as the $replacement is optional.

$version has been added to the actions as an extra argument at the end.

strider724 years ago

Moves $version argument to end

comment:43 strider724 years ago

Adding $version in the middle and moving $replacement to the end changes the backtrace order, which in turn breaks backwards compatibility with the most likely pre-existing uses of these hooks. I can specifically say it screws up my plugin.

Please consider the patch I just attached.

comment:44 strider724 years ago

Apologies -- the problem is something else. The existing argument order on the _deprecated_* functions is the same as it was before.

comment:45 nacin4 years ago

Somehow I managed to I omit $version from the deprecated_argument_run hook. westi has added it in [12686]. Thanks, westi.

Should be all set now.

Note: See TracTickets for help on using tickets.