#22400 closed enhancement (fixed)
Remove all, or at least most, uses of extract() within WordPress
Reported by: | Viper007Bond | Owned by: | |
---|---|---|---|
Milestone: | 4.0 | Priority: | normal |
Severity: | normal | Version: | 3.4.2 |
Component: | General | Keywords: | needs-codex |
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 (20)
Change History (158)
#1
@
12 years ago
- Keywords needs-patch removed
- Milestone changed from Future Release to Awaiting Review
#2
follow-ups:
↓ 4
↓ 7
↓ 9
@
12 years 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 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.
#3
@
12 years 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
#4
in reply to:
↑ 2
;
follow-up:
↓ 6
@
12 years 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.
#6
in reply to:
↑ 4
@
12 years 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. :)
#7
in reply to:
↑ 2
@
12 years ago
- Cc xoodrew@… added
Replying to Viper007Bond:
Having
$defaults
contain all valid parameters certainly helps a lot but I still thinkextract()
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.
#9
in reply to:
↑ 2
@
12 years ago
- Cc mike@… added
Replying to Viper007Bond:
Having
$defaults
contain all valid parameters certainly helps a lot but I still thinkextract()
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 whetherextract()
is used or not.
Definitely agree.
#10
follow-up:
↓ 12
@
12 years 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.
#12
in reply to:
↑ 10
@
12 years 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?
#18
in reply to:
↑ 13
@
12 years 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 :)
#19
@
12 years ago
- Cc mdhansen@… removed
- Keywords needs-testing added
I have a few more but I need to go over them again before submitting them.
#22
follow-up:
↓ 23
@
12 years ago
MikeHansenMe: You left a debug die()
at the bottom of 22400.taxonomy.php.diff
.
#23
in reply to:
↑ 22
@
12 years ago
Replying to Viper007Bond:
MikeHansenMe: You left a debug
die()
at the bottom of22400.taxonomy.php.diff
.
Updated, Thanks.
#27
@
12 years 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 :)
#29
@
11 years 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.
#31
@
11 years 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.
#34
@
10 years ago
wp-get-archives.diff cleans up the changes and unit tests for wp_get_archives()
sans extract()
. The tests needed to be altered to allow them to run directly or as part of the entire suite. The function needed some rehabilitation as well.
#48
follow-up:
↓ 49
@
10 years ago
To my knowledge extract() takes all params from array so it could be very bad to use it when datas come from user. The documentation says extract can take some additional args to avoid bad behavior e.g prefix.
My question regards shortcodes. I use them all the time so do you recommand to remove extract from all our shortcode callbacks?
#49
in reply to:
↑ 48
;
follow-ups:
↓ 50
↓ 51
@
10 years ago
Replying to jmlapam:
To my knowledge extract() takes all params from array so it could be very bad to use it when datas come from user. The documentation says extract can take some additional args to avoid bad behavior e.g prefix.
My question regards shortcodes. I use them all the time so do you recommend to remove extract from all our shortcode callbacks?
Yes, you should absolutely remove uses of extract()
for the same reasons highlighted at the start of the ticket. That said, unless you're doing something really strange within your shortcode function that somehow makes use of all available variables, or you're using global variables which could be overridden, you need not be too concerned about users passing in something that will be extracted. Even so, you'll be happier without extract. :)
#50
in reply to:
↑ 49
@
10 years ago
Replying to rzen:
That said, unless you're doing something really strange within your shortcode function that somehow makes use of all available variables, or you're using global variables which could be overridden, you need not be too concerned about users passing in something that will be extracted. Even so, you'll be happier without extract. :)
Thanks for all the details. What about shortcodes used in menus and text widgets?
#51
in reply to:
↑ 49
@
10 years ago
Replying to rzen:
Replying to jmlapam:
To my knowledge extract() takes all params from array so it could be very bad to use it when datas come from user. The documentation says extract can take some additional args to avoid bad behavior e.g prefix.
My question regards shortcodes. I use them all the time so do you recommend to remove extract from all our shortcode callbacks?
Yes, you should absolutely remove uses of
extract()
for the same reasons highlighted at the start of the ticket. That said, unless you're doing something really strange within your shortcode function that somehow makes use of all available variables, or you're using global variables which could be overridden, you need not be too concerned about users passing in something that will be extracted. Even so, you'll be happier without extract. :)
Also, note that if you are using shortcode_atts()
on the attributes before calling extract()
, only the attributes you have specified will be returned and extracted into variables. So even if you are using extract()
, someone wouldn't be able to override just any variable.
This ticket was mentioned in IRC in #wordpress-dev by rzen. View the logs.
10 years ago
#94
in reply to:
↑ 90
@
10 years ago
Replying to wonderboymusic:
In 28434:
There are some unit tests for wp_list_categories here : https://core.trac.wordpress.org/attachment/ticket/28083/unit-tests-wp-list-categories.diff
#99
@
10 years ago
- Keywords needs-unit-tests added
wp-delete-term.diff requires unit tests.
wp_delete_term()
has a 3rd argument: $args
.
$args
is an array
that can look like array( 'default' => 3, 'force_default' => true )
None of our unit tests cover this scenario. I don't have the energy to write them right now. If someone else wants to: #patcheswelcome
This ticket was mentioned in IRC in #wordpress-dev by Viper007Bond. View the logs.
10 years ago
#125
in reply to:
↑ 123
;
follow-up:
↓ 129
@
10 years ago
Replying to wonderboymusic:
In 28472:
This one might need to be ported over to GlotPress too; not sure who has the canonical copy.
#126
@
10 years ago
wp_insert_link()
is broken since [28406]/[28408]:
Notice: wpdb::prepare was called incorrectly. The query argument of wpdb::prepare() must have a placeholder. Please see Debugging in WordPress for more information. (This message was added in version 3.9.) in wp-includes/functions.php on line 3247 WordPress database error: [You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'WHERE' at line 1] UPDATE `trunk_links` SET WHERE
#129
in reply to:
↑ 125
@
10 years ago
Replying to rmccue:
Replying to wonderboymusic:
In 28472:
This one might need to be ported over to GlotPress too; not sure who has the canonical copy.
Just did that. Thanks for the mention. I don't there isn't really a canonical copy but I always try to catch up if there are changes made in core.
#130
in reply to:
↑ 35
@
10 years ago
- Keywords reporter-feedback added
Replying to wonderboymusic:
In 28379:
Seems something was broken during this process: http://b.ustin.co/DAFr
@
10 years ago
Fixes bug introduced to wp_get_archives when extract was removed re: https://core.trac.wordpress.org/ticket/22400#comment:130
#131
@
10 years ago
Confirmed - will fix: http://scotty-t.com/
#134
@
10 years ago
- Resolution set to fixed
- Status changed from new to closed
Marking as fixed after ~90 commits. There is only one call to extract()
remaining: load_template()
. Unless someone has a bulletproof refactor that isn't gross, we can leave it.
The reasons for removing extract()
:
- It sucks
- it obfuscates variable assignment
- It isn't allowed by Hack - though this really doesn't matter. HHVM will still interpret PHP code.
- In IDEs, all kinds of code standards flags and warnings appear for variables being used that seemingly don't exist.
- Static analysis tools hate them for the same reasons. We don't have to be slaves to static analysis, but tooling is great for catching human error, and we should attack low-hanging fruit when possible.
#135
follow-up:
↓ 136
@
10 years ago
Shouldn't WordPress Codex be updated to not use extract() in the examples? For example...
http://codex.wordpress.org/Function_Reference/shortcode_atts http://codex.wordpress.org/Function_Reference/wp_parse_args http://codex.wordpress.org/Function_Reference/wp_register_sidebar_widget
#136
in reply to:
↑ 135
;
follow-up:
↓ 137
@
10 years ago
- Keywords needs-codex added; westi-likes needs-testing has-patch needs-unit-tests reporter-feedback removed
Replying to JoTGi:
Shouldn't WordPress Codex be updated to not use extract() in the examples? For example...
Yes. Any help, from you, or any that feels competent, would surely be appreciated.
#137
in reply to:
↑ 136
@
10 years ago
extract() references deleted from
http://codex.wordpress.org/Function_Reference/shortcode_atts http://codex.wordpress.org/Function_Reference/wp_parse_args http://codex.wordpress.org/Function_Reference/wp_register_sidebar_widget
#138
@
10 years ago
Added forbidden extract()
usage to the PHP coding standards: https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#dont-extract
The overwhelming scenario where extract() is used in WP is this:
As long as the
$defaults
array contains all the variables that will be used further down, I don't think it's a problem.