WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 14 months ago

Last modified 6 months ago

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

22400.author-template.php.diff (2.9 KB) - added by MikeHansenMe 3 years ago.
author-template.php
22400.meta-boxes.php.diff (8.4 KB) - added by MikeHansenMe 3 years ago.
meda-boxes.php
22400.media.php.diff (5.0 KB) - added by MikeHansenMe 3 years ago.
media.php
22400.wp-activate.php.diff (935 bytes) - added by cfinke 3 years ago.
wp-activate.php
22400.taxonomy.php.diff (2.3 KB) - added by MikeHansenMe 3 years ago.
Updated to remove debug.
22400.dashboard.php.diff (3.7 KB) - added by MikeHansenMe 3 years ago.
Dashboard.php with some minor code clean up as well
22400.general-templates.php.diff (14.1 KB) - added by MikeHansenMe 3 years ago.
general-template.php with some minor code cleanup
22400.wp-activate.php.2.diff (1.3 KB) - added by jeremyfelt 2 years ago.
Refreshed patch for extract() use in wp-activate.php
22400.general-template-w-unit-tests.diff (11.8 KB) - added by MikeHansenMe 18 months ago.
wp_get_archives refresh with unit tests
22400.taxonomy.php.2.diff (4.3 KB) - added by MikeHansenMe 15 months ago.
Refresh + minor cleanup + unit tests
wp-get-archives.diff (12.2 KB) - added by wonderboymusic 14 months ago.
22400.default-widgets.php.diff (20.3 KB) - added by rzen 14 months ago.
Remove extract() on default-widgets.php
22400.default-widgets.php.2.diff (20.3 KB) - added by rzen 14 months ago.
Remove extract() on default-widgets.php
22400.default-widgets.php.3.diff (21.0 KB) - added by rzen 14 months ago.
Take 3: remove extract() throughout default-widgets.php
widgets.diff (11.6 KB) - added by wonderboymusic 14 months ago.
widgets.2.diff (9.4 KB) - added by wonderboymusic 14 months ago.
wp-delete-term.diff (873 bytes) - added by wonderboymusic 14 months ago.
wp_list_categories-unit-tests.diff (15.0 KB) - added by MikeHansenMe 14 months ago.
Updated unit tests with variable ids
wp-insert-attachment.diff (10.2 KB) - added by wonderboymusic 14 months ago.
22400-28379-wp_get_archives.diff (1.9 KB) - added by jtsternberg 14 months 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)

comment:1 @scribu3 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.

comment:2 follow-ups: @Viper007Bond3 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 3 years ago by Viper007Bond (previous) (diff)

comment:3 @MikeHansenMe3 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

comment:4 in reply to: ↑ 2 ; follow-up: @ericmann3 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.

comment:5 @tollmanz3 years ago

  • Cc tollmanz@… added

comment:6 in reply to: ↑ 4 @rmccue3 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. :)

comment:7 in reply to: ↑ 2 @DrewAPicture3 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 3 years ago by DrewAPicture (previous) (diff)

comment:8 @johnbillion3 years ago

  • Cc johnbillion added

comment:9 in reply to: ↑ 2 @MikeSchinkel3 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.

comment:10 follow-up: @westi3 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.

comment:11 @toscho3 years ago

  • Cc info@… added

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

comment:13 follow-up: @scribu3 years ago

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

comment:14 @jleedy3 years ago

  • Cc joseph@… added

comment:15 @jkudish3 years ago

  • Cc jkudish@… added

comment:16 @kovshenin3 years ago

  • Cc kovshenin added

comment:17 @ethitter3 years ago

  • Cc erick@… added

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

@MikeHansenMe3 years ago

author-template.php

@MikeHansenMe3 years ago

meda-boxes.php

comment:19 @MikeHansenMe3 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.

comment:20 @MikeHansenMe3 years ago

  • Cc mdhansen@… added

@MikeHansenMe3 years ago

media.php

@cfinke3 years ago

wp-activate.php

comment:21 @cfinke3 years ago

  • Cc cfinke@… added

comment:22 follow-up: @Viper007Bond3 years ago

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

@MikeHansenMe3 years ago

Updated to remove debug.

comment:23 in reply to: ↑ 22 @MikeHansenMe3 years ago

Replying to Viper007Bond:

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

Updated, Thanks.

@MikeHansenMe3 years ago

Dashboard.php with some minor code clean up as well

@MikeHansenMe3 years ago

general-template.php with some minor code cleanup

comment:24 @TJNowell3 years ago

  • Cc tom@… added

comment:25 @mercime3 years ago

  • Cc mercijavier@… added

comment:26 @alexvorn22 years ago

move to WP 3.6?

comment:27 @alex-ye2 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 :)

comment:28 @frederick.ding2 years ago

  • Cc frederick+wordpress@… added

@jeremyfelt2 years ago

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

comment:29 @jeremyfelt2 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.

comment:30 @DrewAPicture2 years ago

  • Keywords has-patch added; needs-patch removed

comment:31 @MikeHansenMe19 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.

@MikeHansenMe18 months ago

wp_get_archives refresh with unit tests

@MikeHansenMe15 months ago

Refresh + minor cleanup + unit tests

comment:32 @wonderboymusic14 months 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.

comment:33 @wonderboymusic14 months ago

In 28374:

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

Props MikeHansenMe.
See #22400.

comment:34 @wonderboymusic14 months 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.

comment:35 follow-up: @wonderboymusic14 months 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.

comment:36 @wonderboymusic14 months ago

In 28381:

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

See #22400.

comment:37 @wonderboymusic14 months ago

In 28382:

Eliminate use of extract() in trackback_url_list().

See #22400.

comment:38 @wonderboymusic14 months ago

In 28383:

Eliminate use of extract() in the_title_attribute().

See #22400.

comment:39 @wonderboymusic14 months ago

In 28384:

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

See #22400.

comment:40 @wonderboymusic14 months 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.

comment:41 @wonderboymusic14 months 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.

comment:42 @wonderboymusic14 months 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.

comment:43 @wonderboymusic14 months 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.

comment:44 @wonderboymusic14 months 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.

comment:45 @wonderboymusic14 months 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.

comment:46 @wonderboymusic14 months 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.

comment:47 @wonderboymusic14 months ago

In 28392:

Eliminate use of extract() in wp_list_authors().

See #22400.

comment:48 follow-up: @jmlapam14 months 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 14 months ago by jmlapam (previous) (diff)

comment:49 in reply to: ↑ 48 ; follow-ups: @rzen14 months 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. :)

comment:50 in reply to: ↑ 49 @jmlapam14 months 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?

comment:51 in reply to: ↑ 49 @jdgrimes14 months 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.

comment:52 @wonderboymusic14 months ago

  • Milestone changed from Future Release to 4.0

comment:53 @Viper007Bond14 months ago

Awesome work on this, wonderboymusic!

comment:54 @wonderboymusic14 months ago

In 28396:

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

See #22400.

comment:55 @wonderboymusic14 months 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.

comment:56 @wonderboymusic14 months 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.

comment:57 @wonderboymusic14 months ago

In 28401:

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

See #22400.

comment:58 @wonderboymusic14 months ago

In 28402:

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

See #22400.

comment:59 @wonderboymusic14 months ago

In 28403:

Eliminate use of extract() in get_bookmarks().

See #22400.

comment:60 @wonderboymusic14 months ago

In 28404:

Eliminate use of extract() in _walk_bookmarks().

See #22400.

comment:61 @wonderboymusic14 months ago

In 28405:

Eliminate use of extract() in wp_list_bookmarks().

See #22400.

comment:62 @wonderboymusic14 months ago

In 28406:

Eliminate use of extract() in wp_insert_link().

See #22400.

comment:63 @wonderboymusic14 months 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.

comment:64 @wonderboymusic14 months ago

In 28408:

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

See #22400.

comment:65 @wonderboymusic14 months ago

In 28409:

Eliminate use of extract() in wp_star_rating().

See #22400.

comment:66 @wonderboymusic14 months ago

In 28410:

Eliminate use of extract() in wp_terms_checklist().

See #22400.

comment:67 @wonderboymusic14 months ago

In 28411:

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

See #22400.

comment:68 @wonderboymusic14 months ago

In 28412:

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

See #22400.

comment:69 @wonderboymusic14 months ago

In 28413:

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

See #22400.

comment:70 @wonderboymusic14 months ago

In 28414:

Eliminate use of extract() in gallery_shortcode().

See #22400.

comment:71 @wonderboymusic14 months ago

In 28415:

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

See #22400.

comment:72 @wonderboymusic14 months ago

In 28416:

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

See #22400.

comment:73 @wonderboymusic14 months ago

In 28418:

Eliminate the use of extract() in WP_Upgrader.

See #22400.

comment:74 @wonderboymusic14 months ago

In 28419:

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

See #22400.

comment:75 @wonderboymusic14 months ago

In 28420:

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

See #22400.

comment:76 @wonderboymusic14 months ago

In 28421:

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

See #22400.

comment:77 @wonderboymusic14 months ago

In 28422:

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

See #22400.

comment:78 @wonderboymusic14 months ago

In 28423:

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

See #22400.

comment:79 @wonderboymusic14 months 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.

comment:80 @wonderboymusic14 months ago

In 28425:

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

See #22400.

comment:81 @wonderboymusic14 months ago

In 28426:

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

See #22400.

comment:82 @wonderboymusic14 months ago

In 28427:

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

See #22400.

comment:83 @wonderboymusic14 months ago

In 28428:

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

See #22400.

comment:84 @wonderboymusic14 months ago

In 28429:

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

See #22400.

@rzen14 months ago

Remove extract() on default-widgets.php

@rzen14 months ago

Remove extract() on default-widgets.php

comment:85 @ircbot14 months ago

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

comment:86 @wonderboymusic14 months 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.

comment:87 @wonderboymusic14 months ago

In 28431:

Eliminate use of extract() in get_comment_reply_link().

See #22400.

@rzen14 months ago

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

comment:88 @wonderboymusic14 months ago

In 28432:

Eliminate use of extract() in wp_dropdown_categories().

See #22400.

@wonderboymusic14 months ago

@wonderboymusic14 months ago

comment:89 @wonderboymusic14 months ago

In 28433:

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

Props rzen, wonderboymusic.

See #22400.

comment:90 follow-up: @wonderboymusic14 months ago

In 28434:

Eliminate use of extract() in wp_list_categories().

See #22400.

comment:91 @wonderboymusic14 months ago

In 28435:

Eliminate use of extract() in wp_generate_tag_cloud().

See #22400.

comment:92 @wonderboymusic14 months ago

In 28436:

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

See #22400.

comment:93 @wonderboymusic14 months ago

In 28437:

Eliminate use of extract() in wp_allow_comment().

See #22400.

comment:94 in reply to: ↑ 90 @MikeHansenMe14 months 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

comment:95 @wonderboymusic14 months 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.

comment:96 @wonderboymusic14 months ago

In 28439:

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

See #22400.

comment:97 @wonderboymusic14 months 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.

comment:98 @wonderboymusic14 months 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.

comment:99 @wonderboymusic14 months 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

@MikeHansenMe14 months ago

Updated unit tests with variable ids

comment:100 @wonderboymusic14 months 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.

comment:101 @wonderboymusic14 months 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.

comment:102 @wonderboymusic14 months 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.

comment:103 @wonderboymusic14 months 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.

comment:104 @wonderboymusic14 months ago

In 28449:

Eliminate use of extract() in wp_widget_rss_form().

See #22400.

comment:105 @wonderboymusic14 months ago

In 28450:

Eliminate use of extract() in wp_handle_upload().

See #22400.

comment:106 @wonderboymusic14 months ago

In 28451:

Eliminate use of extract() in wp_handle_sideload().

See #22400.

comment:107 @wonderboymusic14 months ago

In 28452:

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

See #22400.

comment:108 @wonderboymusic14 months ago

In 28453:

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

See #22400, [28448].

comment:109 @wonderboymusic14 months ago

In 28454:

Eliminate use of extract() in wp_insert_user().

See #22400.

comment:110 @wonderboymusic14 months 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.

comment:111 @wonderboymusic14 months ago

In 28457:

Eliminate use of extract() in wp_insert_comment().

See #22400.

comment:112 @wonderboymusic14 months ago

In 28458:

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

See #22400.

comment:113 @wonderboymusic14 months ago

In 28459:

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

See #22400.

comment:114 @wonderboymusic14 months 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.

comment:115 @wonderboymusic14 months ago

In 28461:

Eliminate use of extract() in wp_update_term().

See #22400.

comment:116 @wonderboymusic14 months ago

In 28464:

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

See #22400.

comment:117 @wonderboymusic14 months ago

In 28465:

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

See #22400.

comment:118 @wonderboymusic14 months ago

In 28466:

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

See #22400.

comment:119 @wonderboymusic14 months ago

In 28467:

Restore $soucrce in WP_Upgrader::install_package().

See #22400.

comment:120 @wonderboymusic14 months ago

In 28469:

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

See #22400.

comment:121 @wonderboymusic14 months 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.

comment:122 @wonderboymusic14 months ago

In 28471:

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

See #22400.

comment:123 follow-up: @wonderboymusic14 months ago

In 28472:

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

See #22400.

comment:124 @ircbot14 months ago

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

comment:125 in reply to: ↑ 123 ; follow-up: @rmccue14 months 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.

comment:126 @SergeyBiryukov14 months 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

comment:127 @SergeyBiryukov14 months ago

In 28475:

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

see #22400.

comment:128 @wonderboymusic14 months ago

In 28501:

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

See #22400.

comment:129 in reply to: ↑ 125 @markoheijnen14 months 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.

comment:130 in reply to: ↑ 35 @jtsternberg14 months 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

@jtsternberg14 months ago

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

comment:132 @wonderboymusic14 months ago

In 28534:

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

See #22400.

comment:133 @SergeyBiryukov14 months ago

In 28540:

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

see #22400.

comment:134 @wonderboymusic14 months 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.

comment:135 follow-up: @JoTGi10 months 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

comment:136 in reply to: ↑ 135 ; follow-up: @knutsp10 months 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.

comment:137 in reply to: ↑ 136 @JoTGi10 months 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
Note: See TracTickets for help on using tickets.