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

[Viper007Bond]

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.

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.

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.

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.

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

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.

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

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.

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.

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.

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?

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

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

author-template.php

meda-boxes.php

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

media.php

wp-activate.php

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

Updated to remove debug.

Replying to Viper007Bond:

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

Updated, Thanks.

Dashboard.php with some minor code clean up as well

general-template.php with some minor code cleanup

move to WP 3.6?

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