New _deprecated_argument() function

[nacin]

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.

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

[Denis-de-Bernardy]

agreeing with strider.

[filosofo]

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.

[strider72]

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

[filosofo]

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

[strider72]

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.... :-)

[nacin]

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.

[filosofo]

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.

[westi]

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?

[nacin]

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

[nacin]

Sample uses of _deprecated_argument()

[nacin]

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.

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

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

[westi]

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

[westi]

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

[nacin]

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.

[westi]

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

[nacin]

_deprecated_argument() for the rest of core.

[nacin]

What should be the final patch is now attached.

[nacin]

Use this instead. Small change in trackback_url()

[nacin]

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.

[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 ) ) {

[westi]

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.

[sirzooro]

Replying to westi:

But WP_DEBUG is always defined.

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

[filosofo]

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 ...."?

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

[westi]

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.

[nacin]

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.

[nacin]

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?

[nacin]

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.

[westi]

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.

[nacin]

Simplified _deprecated_argument()

[nacin]

Okay, new patch.

[nacin]

Two typos in that patch. Another coming up.

[nacin]

Simplified _deprecated_argument() - sans typos.

[hakre]

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

[nacin]

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.

[nacin]

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

[westi]

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

[westi]

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

[nacin]

All remaining missing version numbers

[nacin]

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.

[nacin]

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.

[nacin]

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

[strider72]

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.

[strider72]

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

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

[nacin]

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.

[nacin]

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

[nacin]

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

[westi]

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

[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

?

[westi]

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.

[strider72]

Moves $version argument to end

[strider72]

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.

[strider72]

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

[nacin]

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.