WordPress.org

Make WordPress Core

Opened 18 months ago

Last modified 4 months ago

#22400 new enhancement

Remove all, or at least most, uses of extract() within WordPress

Reported by: Viper007Bond Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.4.2
Component: General Keywords: westi-likes needs-testing has-patch
Focuses: Cc:

Description

extract() is a terrible function that makes code harder to debug and harder to understand. We should discourage it's use and remove all of our uses of it.

Joseph Scott has a good write-up of why it's bad.

Attachments (10)

22400.author-template.php.diff (2.9 KB) - added by MikeHansenMe 17 months ago.
author-template.php
22400.meta-boxes.php.diff (8.4 KB) - added by MikeHansenMe 17 months ago.
meda-boxes.php
22400.media.php.diff (5.0 KB) - added by MikeHansenMe 17 months ago.
media.php
22400.wp-activate.php.diff (935 bytes) - added by cfinke 17 months ago.
wp-activate.php
22400.taxonomy.php.diff (2.3 KB) - added by MikeHansenMe 17 months ago.
Updated to remove debug.
22400.dashboard.php.diff (3.7 KB) - added by MikeHansenMe 17 months ago.
Dashboard.php with some minor code clean up as well
22400.general-templates.php.diff (14.1 KB) - added by MikeHansenMe 17 months ago.
general-template.php with some minor code cleanup
22400.wp-activate.php.2.diff (1.3 KB) - added by jeremyfelt 9 months ago.
Refreshed patch for extract() use in wp-activate.php
22400.general-template-w-unit-tests.diff (11.8 KB) - added by MikeHansenMe 3 months ago.
wp_get_archives refresh with unit tests
22400.taxonomy.php.2.diff (4.3 KB) - added by MikeHansenMe 11 days ago.
Refresh + minor cleanup + unit tests

Download all attachments as: .zip

Change History (41)

comment:1 scribu18 months ago

  • Keywords needs-patch removed
  • Milestone changed from Future Release to Awaiting Review

The overwhelming scenario where extract() is used in WP is this:

function some_function( $args = array() ) {
  $defaults = array(
    'foo' => 'bar',
    ...
  );

  $args = wp_parse_args( $args, $defaults );

  extract( $args, EXTR_SKIP );

  ...
}

As long as the $defaults array contains all the variables that will be used further down, I don't think it's a problem.

comment:2 follow-ups: Viper007Bond18 months ago

Having $defaults contain all valid parameters certainly helps a lot but I still think extract() is bad.

If I see $foo somewhere deep in the function, it's not immediately clear if this is a function-set value or a user-supplied value. Yes, I probably have to search either way to see where the variable comes from but by instead using $args['foo'], it's more clear that this is a configuration argument rather than just a data storage variable.

I think another question is what's the advantage to using extract()? I can't think of any other than saving keystrokes which shouldn't be a valid reason.

Version 1, edited 18 months ago by Viper007Bond (previous) (next) (diff)

comment:3 MikeHansenMe18 months ago

  • Cc mdhansen@… added

Here is a list of pages that use extract() some are 3rd party.

/wp-activate.php
/wp-admin/custom-header.php
/wp-admin/includes/bookmark.php
/wp-admin/includes/class-pclzip.php
/wp-admin/includes/class-wp-comments-list-table.php
/wp-admin/includes/class-wp-list-table.php
/wp-admin/includes/class-wp-plugin-install-list-table.php
/wp-admin/includes/class-wp-terms-list-table.php
/wp-admin/includes/class-wp-upgrader.php
/wp-admin/includes/dashboard.php
/wp/wp-admin/includes/file.php
/wp-admin/includes/media.php
/wp-admin/includes/meta-boxes.php
/wp-admin/includes/taxonomy.php
/wp-admin/includes/template.php
/wp-admin/install.php
/wp-includes/SimplePie/Enclosure.php
/wp-includes/author-template.php
/wp-includes/bookmark-template.php
/wp-includes/bookmark.php
/wp-includes/category-template.php
/wp-includes/class-wp-ajax-response.php
/wp-includes/class-wp-xmlrpc-server.php
/wp-includes/comment-template.php
/wp-includes/comment.php
/wp-includes/default-widgets.php
/wp-includes/functions.php
/wp-includes/general-template.php
/wp-includes/media.php
/wp-includes/pluggable.php
/wp-includes/pomo/mo.php
/wp-includes/post-template.php
/wp-includes/post.php
/wp-includes/shortcodes.php
/wp-includes/taxonomy.php
/wp-includes/template.php
/wp-includes/theme-compat/comments-popup.php
/wp-includes/user.php
/wp-signup.php

comment:4 in reply to: ↑ 2 ; follow-up: ericmann18 months ago

Replying to Viper007Bond:

If I see $foo somewhere deep in the function, it's not immediately clear if this is a function-set value or a user-supplied value. Yes, I probably have to search either way to see where the variable comes from but by instead using $args['foo'], it's immediately clear that this is a configuration argument rather than just a data storage variable.

I agree. Additionally, we're finally making huge strides in keeping core documented. It's easy to mark up a function signature with phpDoc and identify the $args array that gets passed in. Seeing a variable later that's not in this signature definition is confusing at best and misleading at worst.

If I see $foo in a function, I have to research where it's set. Is it a global? Did I create it? Is it the return of another function?

If I see $args['foo'], and I also know that $args was passed in (because it's in the documentation), there is zero ambiguity.

comment:5 tollmanz18 months ago

  • Cc tollmanz@… added

comment:6 in reply to: ↑ 4 rmccue18 months ago

+1, it's a pain to trace this variables back at times, and it messes with autocompletion in editors with that ability.

Replying to ericmann:

I agree. Additionally, we're finally making huge strides in keeping core documented. It's easy to mark up a function signature with phpDoc and identify the $args array that gets passed in. Seeing a variable later that's not in this signature definition is confusing at best and misleading at worst.

Side-note: the PHP-FIG has been discussing how to document these sorts of parameters. It's a huge discussion, and nothing's finalised yet, but once that is finalised we should look at beginning to use it (along with an updated documentation tool).

Replying to MikeHansenMe:

/wp-includes/SimplePie/Enclosure.php

I'm happy to say that's in a method that will be deprecated. :)

comment:7 in reply to: ↑ 2 DrewAPicture18 months ago

  • Cc xoodrew@… added

Replying to Viper007Bond:

Having $defaults contain all valid parameters certainly helps a lot but I still think extract() is bad.

In many contexts, I'd argue that unless you have intimate knowledge of the codebase, the $defaults array is the best in-line documentation available for incoming args, regardless of whether extract() is used or not.

I've read several places that using extract() often creates confusion, though I think as you pointed out, a lot of that stems from the lack of distinction with the resulting variables. With what seems to be widespread use in core (as evidenced in comment:3), I'm surprised EXTR_PREFIX_ALL never got used to set unique prefixes.

Last edited 18 months ago by DrewAPicture (previous) (diff)

comment:8 johnbillion18 months ago

  • Cc johnbillion added

comment:9 in reply to: ↑ 2 MikeSchinkel18 months ago

  • Cc mike@… added

Replying to Viper007Bond:

Having $defaults contain all valid parameters certainly helps a lot but I still think extract() is bad.

If I see $foo somewhere deep in the function, it's not immediately clear if this is a function-set value or a user-supplied value. Yes, I probably have to search either way to see where the variable comes from but by instead using $args['foo'], it's immediately clear that this is a configuration argument rather than just a data storage variable.

+1

Replying to DrewAPicture:

In many contexts, I'd argue that unless you have intimate knowledge of the codebase, the $defaults array is the best in-line documentation available for incoming args, regardless of whether extract() is used or not.

Definitely agree.

comment:10 follow-up: westi18 months ago

  • Cc westi added
  • Keywords 3.6-early westi-likes needs-patch added
  • Milestone changed from Awaiting Review to Future Release
  • Priority changed from lowest to normal
  • Severity changed from minor to normal

I would love to see a set of patches for this for 3.6-early each tackling a particular area of the code so that they are easy to review and commit.

comment:11 toscho18 months ago

  • Cc info@… added

comment:12 in reply to: ↑ 10 rzen18 months ago

  • Cc brian@… added

Replying to westi:

I would love to see a set of patches for this for 3.6-early each tackling a particular area of the code so that they are easy to review and commit.

With each patch in its own ticket that references back to this ticket, correct?

comment:13 follow-up: scribu18 months ago

I'm pretty sure he meant on the same ticket, this one.

comment:14 jleedy18 months ago

  • Cc joseph@… added

comment:15 jkudish17 months ago

  • Cc jkudish@… added

comment:16 kovshenin17 months ago

  • Cc kovshenin added

comment:17 ethitter17 months ago

  • Cc erick@… added

comment:18 in reply to: ↑ 13 westi17 months ago

Replying to scribu:

I'm pretty sure he meant on the same ticket, this one.

Correct.

Lots of small patches are best for this, but we don't need a ticket for every patch :)

MikeHansenMe17 months ago

author-template.php

MikeHansenMe17 months ago

meda-boxes.php

comment:19 MikeHansenMe17 months ago

  • Cc mdhansen@… removed
  • Keywords needs-testing added

I have a few more but I need to go over them again before submitting them.

comment:20 MikeHansenMe17 months ago

  • Cc mdhansen@… added

MikeHansenMe17 months ago

media.php

cfinke17 months ago

wp-activate.php

comment:21 cfinke17 months ago

  • Cc cfinke@… added

comment:22 follow-up: Viper007Bond17 months ago

MikeHansenMe: You left a debug die() at the bottom of 22400.taxonomy.php.diff.

MikeHansenMe17 months ago

Updated to remove debug.

comment:23 in reply to: ↑ 22 MikeHansenMe17 months ago

Replying to Viper007Bond:

MikeHansenMe: You left a debug die() at the bottom of 22400.taxonomy.php.diff.

Updated, Thanks.

MikeHansenMe17 months ago

Dashboard.php with some minor code clean up as well

MikeHansenMe17 months ago

general-template.php with some minor code cleanup

comment:24 TJNowell16 months ago

  • Cc tom@… added

comment:25 mercime16 months ago

  • Cc mercijavier@… added

comment:26 alexvorn214 months ago

move to WP 3.6?

comment:27 alex-ye13 months ago

  • Cc nashwan.doaqan@… added

I hope to see this in 3.6 , The idea is clear :) , some projects like bbPress follow this ticket and it's make debugging more more easier :)

comment:28 frederick.ding9 months ago

  • Cc frederick+wordpress@… added

jeremyfelt9 months ago

Refreshed patch for extract() use in wp-activate.php

comment:29 jeremyfelt9 months ago

  • Keywords 3.6-early removed

22400.wp-activate.php.2.diff is a refreshed patch to clear extract() from src/wp-activate.php

The use was with returned data from wpmu_activate_signup() which returns an array containing blog_id, user_id, password, title, and meta. $blog_id, $user_id, and $password are all accounted for in the patch. wp-activate.php did not make use of title or meta.

comment:30 DrewAPicture9 months ago

  • Keywords has-patch added; needs-patch removed

comment:31 MikeHansenMe4 months ago

I think these patches will likely need a refresh. I think they will have a much better chance for going into core if we write some unit tests for them to ensure we do not miss anything and break something.

MikeHansenMe3 months ago

wp_get_archives refresh with unit tests

MikeHansenMe11 days ago

Refresh + minor cleanup + unit tests

Note: See TracTickets for help on using tickets.