Make WordPress Core

Opened 12 years ago

Closed 10 years ago

Last modified 4 years ago

#22400 closed enhancement (fixed)

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

Reported by: viper007bond's profile 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)

22400.author-template.php.diff (2.9 KB) - added by MikeHansenMe 12 years ago.
author-template.php
22400.meta-boxes.php.diff (8.4 KB) - added by MikeHansenMe 12 years ago.
meda-boxes.php
22400.media.php.diff (5.0 KB) - added by MikeHansenMe 12 years ago.
media.php
22400.wp-activate.php.diff (935 bytes) - added by cfinke 12 years ago.
wp-activate.php
22400.taxonomy.php.diff (2.3 KB) - added by MikeHansenMe 12 years ago.
Updated to remove debug.
22400.dashboard.php.diff (3.7 KB) - added by MikeHansenMe 12 years ago.
Dashboard.php with some minor code clean up as well
22400.general-templates.php.diff (14.1 KB) - added by MikeHansenMe 12 years ago.
general-template.php with some minor code cleanup
22400.wp-activate.php.2.diff (1.3 KB) - added by jeremyfelt 11 years ago.
Refreshed patch for extract() use in wp-activate.php
22400.general-template-w-unit-tests.diff (11.8 KB) - added by MikeHansenMe 11 years ago.
wp_get_archives refresh with unit tests
22400.taxonomy.php.2.diff (4.3 KB) - added by MikeHansenMe 11 years ago.
Refresh + minor cleanup + unit tests
wp-get-archives.diff (12.2 KB) - added by wonderboymusic 10 years ago.
22400.default-widgets.php.diff (20.3 KB) - added by rzen 10 years ago.
Remove extract() on default-widgets.php
22400.default-widgets.php.2.diff (20.3 KB) - added by rzen 10 years ago.
Remove extract() on default-widgets.php
22400.default-widgets.php.3.diff (21.0 KB) - added by rzen 10 years ago.
Take 3: remove extract() throughout default-widgets.php
widgets.diff (11.6 KB) - added by wonderboymusic 10 years ago.
widgets.2.diff (9.4 KB) - added by wonderboymusic 10 years ago.
wp-delete-term.diff (873 bytes) - added by wonderboymusic 10 years ago.
wp_list_categories-unit-tests.diff (15.0 KB) - added by MikeHansenMe 10 years ago.
Updated unit tests with variable ids
wp-insert-attachment.diff (10.2 KB) - added by wonderboymusic 10 years ago.
22400-28379-wp_get_archives.diff (1.9 KB) - added by jtsternberg 10 years ago.
Fixes bug introduced to wp_get_archives when extract was removed re: https://core.trac.wordpress.org/ticket/22400#comment:130

Download all attachments as: .zip

Change History (158)

#1 @scribu
12 years 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.

#2 follow-ups: @Viper007Bond
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.

Last edited 12 years ago by Viper007Bond (previous) (diff)

#3 @MikeHansenMe
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: @ericmann
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.

#5 @tollmanz
12 years ago

  • Cc tollmanz@… added

#6 in reply to: ↑ 4 @rmccue
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 @DrewAPicture
12 years 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 12 years ago by DrewAPicture (previous) (diff)

#8 @johnbillion
12 years ago

  • Cc johnbillion added

#9 in reply to: ↑ 2 @MikeSchinkel
12 years 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.

#10 follow-up: @westi
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.

#11 @toscho
12 years ago

  • Cc info@… added

#12 in reply to: ↑ 10 @rzen
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?

#13 follow-up: @scribu
12 years ago

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

#14 @anonymized_10690803
12 years ago

  • Cc joseph@… added

#15 @jkudish
12 years ago

  • Cc jkudish@… added

#16 @kovshenin
12 years ago

  • Cc kovshenin added

#17 @ethitter
12 years ago

  • Cc erick@… added

#18 in reply to: ↑ 13 @westi
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 :)

@MikeHansenMe
12 years ago

author-template.php

@MikeHansenMe
12 years ago

meda-boxes.php

#19 @MikeHansenMe
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.

#20 @MikeHansenMe
12 years ago

  • Cc mdhansen@… added

@MikeHansenMe
12 years ago

media.php

@cfinke
12 years ago

wp-activate.php

#21 @cfinke
12 years ago

  • Cc cfinke@… added

#22 follow-up: @Viper007Bond
12 years ago

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

@MikeHansenMe
12 years ago

Updated to remove debug.

#23 in reply to: ↑ 22 @MikeHansenMe
12 years ago

Replying to Viper007Bond:

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

Updated, Thanks.

@MikeHansenMe
12 years ago

Dashboard.php with some minor code clean up as well

@MikeHansenMe
12 years ago

general-template.php with some minor code cleanup

#24 @TJNowell
12 years ago

  • Cc tom@… added

#25 @mercime
12 years ago

  • Cc mercijavier@… added

#26 @alexvorn2
12 years ago

move to WP 3.6?

#27 @alex-ye
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 :)

#28 @frederick.ding
11 years ago

  • Cc frederick+wordpress@… added

@jeremyfelt
11 years ago

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

#29 @jeremyfelt
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.

#30 @DrewAPicture
11 years ago

  • Keywords has-patch added; needs-patch removed

#31 @MikeHansenMe
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.

@MikeHansenMe
11 years ago

wp_get_archives refresh with unit tests

@MikeHansenMe
11 years ago

Refresh + minor cleanup + unit tests

#32 @wonderboymusic
11 years ago

In 28342:

audio, video, and playlist shortcodes:

  • Convert all instances of variable variables to array properties
  • Eradicate use of extract()
  • Rename $atts to $html_atts where collision with new $atts (shortcode atts holder) var might occur

See #22400, #27881.

#33 @wonderboymusic
10 years ago

In 28374:

Eliminate use of extract() in wp_insert_category(). Adds unit tests. All unit tests pass.

Props MikeHansenMe.
See #22400.

#34 @wonderboymusic
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.

#35 follow-up: @wonderboymusic
10 years ago

In 28379:

Eliminate use of extract() in wp_get_archives().

Adds unit tests: tests/functions/getArchives.php.
All other unit tests pass.

Props MikeHansenMe, wonderboymusic.
See #22400.

#36 @wonderboymusic
10 years ago

In 28381:

Eliminate use of extract() in wp-includes/theme-compat/comments-popup.php.

See #22400.

#37 @wonderboymusic
10 years ago

In 28382:

Eliminate use of extract() in trackback_url_list().

See #22400.

#38 @wonderboymusic
10 years ago

In 28383:

Eliminate use of extract() in the_title_attribute().

See #22400.

#39 @wonderboymusic
10 years ago

In 28384:

Eliminate use of extract() in Custom_Image_Header::step_2().

See #22400.

#40 @wonderboymusic
10 years ago

In 28385:

Eliminate use of extract() in WP_Post_Comments_List_Table::display(). The method only needs to read singular from $this->_args. Extraction is unnecessary.

Eliminate use of extract() in WP_Comments_List_Table::display(). The method uses no variables. Extraction can be completely removed.

See #22400.

#41 @wonderboymusic
10 years ago

In 28386:

Eliminate use of extract() in WP_List_Table::display(). The method only needs to read singular from $this->_args. Extraction is unnecessary.

See #22400.

#42 @wonderboymusic
10 years ago

In 28387:

Eliminate use of extract() in WP_List_Table::ajax_response():

  • Extracting $this->_args is unnecessary since none of the variables produced are present in the method.
  • total_items and total_pages can be read directly from $this->_pagination_args

See #22400.

#43 @wonderboymusic
10 years ago

In 28388:

Eliminate use of extract() in WP_Plugin_Install_List_Table::get_table_classes(). It only needs to read plural, no need to extract.

See #22400.

#44 @wonderboymusic
10 years ago

In 28389:

Eliminate use of extract() in WP_List_Table::pagination():

  • Set variables instead of extracting $this->_pagination_args
  • Weird: $infinite_scroll is only presently set in subclasses of WP_List_Table and never initialized otherwise.

See #22400.

#45 @wonderboymusic
10 years ago

In 28390:

Eliminate use of extract() in WP_Terms_List_Table::display_rows_or_placeholder():

  • Set variables for $page and $number
  • list(...) = $this->get_column_info() can be removed, as none of the variables returned are used.
  • orderby and search can be checked from $args, leaving no reason to extract

See #22400.

#46 @wonderboymusic
10 years ago

In 28391:

Eliminate use of extract() in post_tags_meta_box() and post_categories_meta_box().

Both functions only need to read taxonomy, yet they extract every available variable from $box['args']. Even if those variables were useful, there is no attempt to pass them to anything, and scope in PHP does not allow them to be scooped up by inner functions without being passed directly.

See #22400.

#47 @wonderboymusic
10 years ago

In 28392:

Eliminate use of extract() in wp_list_authors().

See #22400.

#48 follow-up: @anonymized_10765487
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?

Last edited 10 years ago by anonymized_10765487 (previous) (diff)

#49 in reply to: ↑ 48 ; follow-ups: @rzen
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 @anonymized_10765487
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 @jdgrimes
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.

#52 @wonderboymusic
10 years ago

  • Milestone changed from Future Release to 4.0

#53 @Viper007Bond
10 years ago

Awesome work on this, wonderboymusic!

#54 @wonderboymusic
10 years ago

In 28396:

Eliminate use of extract() in display_setup_form(). Only needs to read password and password_message.

See #22400.

#55 @wonderboymusic
10 years ago

In 28398:

Eliminate use of extract() in wp_link_pages().

Adds unit tests to a new file: tests/post/template.php.
There were previously no unit tests for wp_link_pages().

See #22400.

#56 @wonderboymusic
10 years ago

In 28399:

Eliminate use of extract() in wp_dropdown_pages().

Adds unit tests to: tests/post/template.php.
There was previously only one wimpy assertion for wp_dropdown_pages().

See #22400.

#57 @wonderboymusic
10 years ago

In 28401:

Eliminate use of extract() in wp_list_pages() which, surprisingly, didn't even use any of the extracted variables.

See #22400.

#58 @wonderboymusic
10 years ago

In 28402:

Eliminate use of extract() in Walker_Page::start_el().

See #22400.

#59 @wonderboymusic
10 years ago

In 28403:

Eliminate use of extract() in get_bookmarks().

See #22400.

#60 @wonderboymusic
10 years ago

In 28404:

Eliminate use of extract() in _walk_bookmarks().

See #22400.

#61 @wonderboymusic
10 years ago

In 28405:

Eliminate use of extract() in wp_list_bookmarks().

See #22400.

#62 @wonderboymusic
10 years ago

In 28406:

Eliminate use of extract() in wp_insert_link().

See #22400.

#63 @wonderboymusic
10 years ago

In 28407:

Eliminate use of extract() in get_media_item().

To test, fire open the old media in the console: tb_show('Old Media is Weird', 'media-upload.php?type=image&TB_iframe=true&post_id=0');

See #22400.

#64 @wonderboymusic
10 years ago

In 28408:

(REALLY) Eliminate use of extract() in wp_insert_link().

See #22400.

#65 @wonderboymusic
10 years ago

In 28409:

Eliminate use of extract() in wp_star_rating().

See #22400.

#66 @wonderboymusic
10 years ago

In 28410:

Eliminate use of extract() in wp_terms_checklist().

See #22400.

#67 @wonderboymusic
10 years ago

In 28411:

Eliminate use of extract() in Walker_Category_Checklist::start_el().

See #22400.

#68 @wonderboymusic
10 years ago

In 28412:

Eliminate use of extract() in wp_xmlrpc_server::blogger_editPost().

See #22400.

#69 @wonderboymusic
10 years ago

In 28413:

Update the inline docs for add_shortcode() to eliminate suggestion to use extract().

See #22400.

#70 @wonderboymusic
10 years ago

In 28414:

Eliminate use of extract() in gallery_shortcode().

See #22400.

#71 @wonderboymusic
10 years ago

In 28415:

Eliminate use of extract() in get_the_taxonomies(). Adds unit test.

See #22400.

#72 @wonderboymusic
10 years ago

In 28416:

Eliminate one of the uses of extract() in wp_handle_sideload().

See #22400.

#73 @wonderboymusic
10 years ago

In 28418:

Eliminate the use of extract() in WP_Upgrader.

See #22400.

#74 @wonderboymusic
10 years ago

In 28419:

Set @param back to $output, not $html, in the inline filter docs. This was a copy/paste mistake.

See #22400.

#75 @wonderboymusic
10 years ago

In 28420:

Eliminate the use of extract() in wp_dropdown_users().

See #22400.

#76 @wonderboymusic
10 years ago

In 28421:

Eliminate the use of extract() in the_taxonomies(). Adds unit test.

See #22400.

#77 @wonderboymusic
10 years ago

In 28422:

Eliminate the use of extract() in WP_Tax_Query::get_sql(). All unit tests still pass.

See #22400.

#78 @wonderboymusic
10 years ago

In 28423:

Eliminate the use of extract() in get_objects_in_term(). Only one property (order) was extracted.

See #22400.

#79 @wonderboymusic
10 years ago

In 28424:

Eliminate the use of extract() in wp_validate_auth_cookie().

Don't do anything fancy here, just set the 4 returned properties to variables. This function is semi-important.

See #22400.

#80 @wonderboymusic
10 years ago

In 28425:

Eliminate the use of extract() in wp_mail(). Check the filtered array for each value before re-setting variables.

See #22400.

#81 @wonderboymusic
10 years ago

In 28426:

Eliminate the use of extract() in wp_check_filetype_and_ext().

See #22400.

#82 @wonderboymusic
10 years ago

In 28427:

Eliminate the use of extract() in wp_update_comment(). All unit tests pass.

See #22400.

#83 @wonderboymusic
10 years ago

In 28428:

Eliminate the use of extract() in wp_list_comments(). All unit tests pass.

See #22400.

#84 @wonderboymusic
10 years ago

In 28429:

Eliminate the use of extract() in get_post_reply_link().

See #22400.

@rzen
10 years ago

Remove extract() on default-widgets.php

@rzen
10 years ago

Remove extract() on default-widgets.php

This ticket was mentioned in IRC in #wordpress-dev by rzen. View the logs.


10 years ago

#86 @wonderboymusic
10 years ago

In 28430:

Eliminate use of extract() in WP_Ajax_Response::add(). Just set most of the properties to variables to avoid interpolation churn.

See #22400.

#87 @wonderboymusic
10 years ago

In 28431:

Eliminate use of extract() in get_comment_reply_link().

See #22400.

@rzen
10 years ago

Take 3: remove extract() throughout default-widgets.php

#88 @wonderboymusic
10 years ago

In 28432:

Eliminate use of extract() in wp_dropdown_categories().

See #22400.

#89 @wonderboymusic
10 years ago

In 28433:

Eliminate use of extract() in default-widgets.php.

Props rzen, wonderboymusic.

See #22400.

#90 follow-up: @wonderboymusic
10 years ago

In 28434:

Eliminate use of extract() in wp_list_categories().

See #22400.

#91 @wonderboymusic
10 years ago

In 28435:

Eliminate use of extract() in wp_generate_tag_cloud().

See #22400.

#92 @wonderboymusic
10 years ago

In 28436:

Eliminate use of extract() in Walker_Category::start_el().

See #22400.

#93 @wonderboymusic
10 years ago

In 28437:

Eliminate use of extract() in wp_allow_comment().

See #22400.

#94 in reply to: ↑ 90 @MikeHansenMe
10 years ago

Replying to wonderboymusic:

In 28434:

Eliminate use of extract() in wp_list_categories().

See #22400.

There are some unit tests for wp_list_categories here : https://core.trac.wordpress.org/attachment/ticket/28083/unit-tests-wp-list-categories.diff

#95 @wonderboymusic
10 years ago

In 28438:

In Walker_Category::start_el(), $title might not be set when $args are passed multiple levels into walk_category_tree().

See #22400.

#96 @wonderboymusic
10 years ago

In 28439:

Eliminate use of extract() in WP_Widget_RSS::widget().

See #22400.

#97 @wonderboymusic
10 years ago

In 28440:

Eliminate use of extract() in wp_widget_rss_output().

Add 'items' => 0 to $default_args. When 0, the value is set to 10 (the fallback).
Every other default arg has a default value of 0.

items is expected to always be passed to this function.

See #22400.

#98 @wonderboymusic
10 years ago

In 28441:

Eliminate use of extract() in wp_get_object_terms().

There are 3 properties, just set them to variables. They are used too often to warrant a refactor.

See #22400.

#99 @wonderboymusic
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

@MikeHansenMe
10 years ago

Updated unit tests with variable ids

#100 @wonderboymusic
10 years ago

In 28445:

Eliminate use of extract() in validate_another_blog_signup().

The extracted variables set/overwrite globals. $user does not need to be pulled from validate_blog_form() as it is not used in this function.

See #22400.

#101 @wonderboymusic
10 years ago

In 28446:

Eliminate use of extract() in validate_user_signup().

$orig_username does not need to be pulled from validate_user_form() as it is not used in this function.

See #22400.

#102 @wonderboymusic
10 years ago

In 28447:

Eliminate use of extract() in validate_blog_signup():

  • $orig_username does not need to be pulled from wpmu_validate_user_signup() as it is not used in this function.
  • $user does not need to be pulled from wpmu_validate_blog_signup() as it is not used in this function.
  • For the wpmu_validate_user_signup() portion, rename $result and $errors to $user_result and $user_errors` for disambiguation with the blog values below.

See #22400.

#103 @wonderboymusic
10 years ago

In 28448:

Eliminate use of extract() in wp_xmlrpc_server::mw_editPost() (MetaWeblog API, y'all).

A lot of the extracted variables are overwritten by being explicitly set later.
Only set variables that would otherwise not be present with compact() is called.

See #22400.

#104 @wonderboymusic
10 years ago

In 28449:

Eliminate use of extract() in wp_widget_rss_form().

See #22400.

#105 @wonderboymusic
10 years ago

In 28450:

Eliminate use of extract() in wp_handle_upload().

See #22400.

#106 @wonderboymusic
10 years ago

In 28451:

Eliminate use of extract() in wp_handle_sideload().

See #22400.

#107 @wonderboymusic
10 years ago

In 28452:

Update inline docs for wp_handle_upload|sideload to reflect their non-use of extract().

See #22400.

#108 @wonderboymusic
10 years ago

In 28453:

In wp_xmlrpc_server::mw_editPost, also set $post_type = $postdata['post_type'].

See #22400, [28448].

#109 @wonderboymusic
10 years ago

In 28454:

Eliminate use of extract() in wp_insert_user().

See #22400.

#110 @wonderboymusic
10 years ago

In 28456:

Eliminate use of extract() in request_filesystem_credentials().

The only property that doesn't need to be set to a variable is $password.

See #22400.

#111 @wonderboymusic
10 years ago

In 28457:

Eliminate use of extract() in wp_insert_comment().

See #22400.

#112 @wonderboymusic
10 years ago

In 28458:

Eliminate use of extract() in WP_Comment_Query::query().

See #22400.

#113 @wonderboymusic
10 years ago

In 28459:

(ACTUALLY) Eliminate use of extract() in WP_Comment_Query::query().

See #22400.

#114 @wonderboymusic
10 years ago

In 28460:

The cache key for comments in WP_Comment_Query::query() needs to do wp_array_slice_assoc( $this->query_vars, array_keys( $defaults ) ) instead of compact( array_keys( $defaults ) ). The latter assumes all of those variables are still floating around.

See #22400.

#115 @wonderboymusic
10 years ago

In 28461:

Eliminate use of extract() in wp_update_term().

See #22400.

#116 @wonderboymusic
10 years ago

In 28464:

Eliminate the use of extract() in wp_insert_term().

See #22400.

#117 @wonderboymusic
10 years ago

In 28465:

Eliminate the use of extract() in get_terms().

See #22400.

#118 @wonderboymusic
10 years ago

In 28466:

Eliminate the use of extract() in wp_delete_term().

See #22400.

#119 @wonderboymusic
10 years ago

In 28467:

Restore $soucrce in WP_Upgrader::install_package().

See #22400.

#120 @wonderboymusic
10 years ago

In 28469:

Eliminate the use of extract() in wp_insert_post().

See #22400.

#121 @wonderboymusic
10 years ago

In 28470:

Eliminate the use of extract() in wp_insert_attachment().

wp_insert_attachment() and wp_insert_post() are incredibly similar, but have branched logic. I have annotated many places where they diverge.

See #22400.

#122 @wonderboymusic
10 years ago

In 28471:

Eliminate the use of extract() in get_pages().

See #22400.

#123 follow-up: @wonderboymusic
10 years ago

In 28472:

Eliminate the use of extract() in MO::import_from_reader().

See #22400.

This ticket was mentioned in IRC in #wordpress-dev by Viper007Bond. View the logs.


10 years ago

#125 in reply to: ↑ 123 ; follow-up: @rmccue
10 years ago

Replying to wonderboymusic:

In 28472:

Eliminate the use of extract() in MO::import_from_reader().

See #22400.

This one might need to be ported over to GlotPress too; not sure who has the canonical copy.

#126 @SergeyBiryukov
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

#127 @SergeyBiryukov
10 years ago

In 28475:

Fix wp_insert_link(), broken in [28406]/[28408].

see #22400.

#128 @wonderboymusic
10 years ago

In 28501:

In wp_list_bookmarks(), $categorize should now be $r['categorize].

See #22400.

#129 in reply to: ↑ 125 @markoheijnen
10 years ago

Replying to rmccue:

Replying to wonderboymusic:

In 28472:

Eliminate the use of extract() in MO::import_from_reader().

See #22400.

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 @jtsternberg
10 years ago

  • Keywords reporter-feedback added

Replying to wonderboymusic:

In 28379:

Eliminate use of extract() in wp_get_archives().

Adds unit tests: tests/functions/getArchives.php.
All other unit tests pass.

Props MikeHansenMe, wonderboymusic.
See #22400.

Seems something was broken during this process: http://b.ustin.co/DAFr

@jtsternberg
10 years ago

Fixes bug introduced to wp_get_archives when extract was removed re: https://core.trac.wordpress.org/ticket/22400#comment:130

#132 @wonderboymusic
10 years ago

In 28534:

Fix some bad UI recursion in wp_get_archives() caused by [28379]. It appears that $afterafter was appropriately named.

See #22400.

#133 @SergeyBiryukov
10 years ago

In 28540:

Use correct variable. see [28534], [28538].

see #22400.

#134 @wonderboymusic
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: @JoTGi
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: @knutsp
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 @JoTGi
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 @westonruter
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

Last edited 4 years ago by SergeyBiryukov (previous) (diff)
Note: See TracTickets for help on using tickets.