WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 3 months ago

#13874 accepted enhancement

Add package argument to "_deprecated_function" function

Reported by: johnjamesjacoby Owned by: westi
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Bootstrap/Load Keywords: has-patch
Focuses: Cc:

Description

Got to thinking it would be nice if plugins could use the _deprecated_ API by passing a "$package" argument to separate themselves from WP core, and so this patch was born.

Attachments (3)

_deprecated_package.patch (73.6 KB) - added by johnjamesjacoby 4 years ago.
_deprecated_ function enhancements
13874.diff (3.3 KB) - added by ericmann 2 years ago.
Refreshed patch with $package at end of function calls.
13874.2.diff (3.3 KB) - added by wonderboymusic 15 months ago.

Download all attachments as: .zip

Change History (27)

johnjamesjacoby4 years ago

_deprecated_ function enhancements

comment:1 johnjamesjacoby4 years ago

This was patched against 3.0-RC3-15241

comment:2 demetris4 years ago

Why not put the new argument at the end and give it a default value of wp-core?

comment:3 follow-up: nacin4 years ago

We would be breaking compatibility with plugins that currently use this by rearranging arguments. $package = 'core' at the end seems sensible to me.

comment:4 johnjamesjacoby4 years ago

What plugins currently use this? If they're deprecating their own functions this way, they're breaking deprecated functions for core, since WP only knows to deprecate its own functions.

I also figured tacking it to the beginning made sense from a code readability perspective; file->package->version->function

..but, I'd settle for whatever is easiest.

comment:5 demetris4 years ago

My concern is not so much backwards compatibility as readability.

The way you added it is more logical and makes it easier and more readable for the few external scripts that will use the function; core code, on the other hand, is more compact and more readable when it uses the default value of an argument that comes after all arguments that it has to define.

comment:6 westi4 years ago

  • Milestone set to 3.1
  • Owner set to westi
  • Status changed from new to accepted

I want to do this in 3.1

comment:7 nacin3 years ago

  • Milestone changed from Awaiting Triage to Future Release

comment:8 in reply to: ↑ 3 johnjamesjacoby2 years ago

  • Keywords needs-refresh added

Replying to nacin:

We would be breaking compatibility with plugins that currently use this by rearranging arguments. $package = 'core' at the end seems sensible to me.

After further review, I agree. Recent Twitter conversations have brought this back up; it'd be great to get a refreshed patch on this.

comment:9 ericmann2 years ago

+1 To this idea, though I agree that new arguments should be placed at the end. Whether or not people should have been using this functionality in plugins before doesn't really matter. Fact is, people have used it out of the interest of being helpful to other devs. We should make an effort to not break things when modifying functionality.

comment:10 GaryJ2 years ago

  • Cc gary@… added

ericmann2 years ago

Refreshed patch with $package at end of function calls.

comment:11 ericmann2 years ago

  • Keywords needs-refresh removed

As I started walking through the code to refresh the patch, I realized that placing $package at the end of the function calls, along with setting a default value of "core," made this a relatively small patch to apply. There's no real need to walk back through every instance of the _deprecated_function()/_deprecated_file()/_deprecated_argument() calls since they'll all inherit the right value for $package anyway.

But this should open things up for plugin and theme devs to specify their own deprecated functions in the future.

comment:12 strider722 years ago

Just a note to suggest that while this is a convenience, it isn't technically necessary, as a backtrace can readily identify the file in which the deprecated function was called.

My "Log Deprecated Calls" plugin does this: http://striderweb.com/nerdaphernalia/features/wp-log-deprecated-calls/

I have nothing against convenience, of course. ;-)

wonderboymusic15 months ago

comment:13 wonderboymusic15 months ago

  • Milestone changed from Future Release to 3.6

westi: "I want to do this in 3.1" - my patch is identical to the one before it, just applies cleaner to trunk

comment:14 alex-ye11 months ago

  • Cc nashwan.doaqan@… added

comment:15 ryan11 months ago

  • Milestone changed from 3.6 to Future Release

comment:16 wonderboymusic9 months ago

  • Milestone changed from Future Release to 3.7

westi: "I want to do this in 3.1"

comment:17 follow-up: ericlewis8 months ago

Modifying _deprecated_function and _deprecated_file to allow for more integration with third-party code is great. However, these functions remain prefixed with underscores to denote them as private, as well as in the Docblock above. Deprecating these deprecated functions in lieu of un-prefixed ones may be overkill and quite the meta commit, but we should at least change the Dockblock.

comment:18 hotchkissconsulting8 months ago

Why not add wp_deprecated_function, wp_deprecated_argument and wp_deprecated_file, which are just passthroughs to the private functions, then eventually deprecate _deprecated_function, etc?

comment:19 hotchkissconsulting8 months ago

  • Cc hotchkissconsulting added

comment:20 ericlewis8 months ago

Related: #9927 and #24164

I'm coming across more and more situations where a function's name is misleading, and there's a suggestion to rename it; a top-down decision on what to do in these situations would be good.

comment:21 desrosj8 months ago

  • Cc desrosj@… added

comment:22 in reply to: ↑ 17 nacin7 months ago

Replying to ericlewis:

Modifying _deprecated_function and _deprecated_file to allow for more integration with third-party code is great. However, these functions remain prefixed with underscores to denote them as private, as well as in the Docblock above. Deprecating these deprecated functions in lieu of un-prefixed ones may be overkill and quite the meta commit, but we should at least change the Dockblock.

I don't feel strongly either way, but if we're going to add package support to this, new functions (with the package argument being first) might not be a bad idea.

I'm coming across more and more situations where a function's name is misleading, and there's a suggestion to rename it; a top-down decision on what to do in these situations would be good.

Usually we err on not renaming functions for the sake of renaming them. We would rather do things like overhaul their internals or arguments in the process, otherwise we're just using up the existing namespace (and annoying developers by deprecating potentially commonly used functions just for a rename).

comment:23 nacin7 months ago

  • Milestone changed from 3.7 to Future Release

comment:24 nacin3 months ago

  • Component changed from Warnings/Notices to Bootstrap/Load
Note: See TracTickets for help on using tickets.