WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 12 months ago

#14424 closed enhancement (fixed)

Eliminate dynamic function definitions, ie create_function

Reported by: ScottMac Owned by: westi
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.0
Component: General Keywords: ongoing-project has-patch
Focuses: performance Cc:

Description

create_function() calls are essentially a form of eval() where the body of the functions is defined at runtime rather than compile time. This patch removes all but one of the calls within the translation code.

There are a few reasons for doing this, one is apc can't cache the results of the function definition and secondly HipHop doesn't support complex create_function definitions.

Most of the references were array_filter / array_map and replaced with a simple foreach.

Others were sorting and a callback function was added.

Attachments (10)

wp-create-function-changes.diff (21.2 KB) - added by ScottMac 5 years ago.
14424.tag-cloud.patch (739 bytes) - added by ocean90 4 years ago.
14424.diff (3.8 KB) - added by obenland 15 months ago.
14424.2.diff (4.1 KB) - added by markjaquith 13 months ago.
refresh
14424.3.diff (4.2 KB) - added by markjaquith 13 months ago.
rename one of the callbacks to be more generic
14424.4.diff (4.1 KB) - added by markjaquith 13 months ago.
without modifying a superglobal
14424.5.diff (1.7 KB) - added by nacin 13 months ago.
14424.6.diff (4.7 KB) - added by markjaquith 13 months ago.
merge 14424.4.diff and 14424.5.diff
14424.7.diff (5.7 KB) - added by markjaquith 13 months ago.
Add a test for get_importers() ordering
14424-fix.diff (433 bytes) - added by TobiasBg 12 months ago.
Replace a forgotten $GET['paged']

Download all attachments as: .zip

Change History (46)

comment:1 @Denis-de-Bernardy5 years ago

  • Component changed from General to Optimization
  • Keywords needs-patch added

Needs a bit more massaging. The first foreach loop, for instance, does not initialize the array.

comment:2 @ScottMac5 years ago

Thanks, I've fixed that now.

comment:3 @ryan5 years ago

  • Milestone changed from Awaiting Review to 3.1

comment:4 @westi4 years ago

  • Owner set to westi
  • Status changed from new to accepted

comment:5 @nacin4 years ago

kses ones are here: #14245.

comment:6 @jane4 years ago

  • Keywords ongoing-project added

comment:7 follow-up: @nacin4 years ago

[16034] missed the ticket.

I think we have that exact helper function in deprecated.php, _funky_javascript_callback().

comment:8 @nacin4 years ago

Related, kind of: #14245

comment:9 @westi4 years ago

(In [16035]) Purger more create_function usage during autop and iso descrambling. See #14424 props ScottMac.

comment:10 in reply to: ↑ 7 @westi4 years ago

Replying to nacin:

[16034] missed the ticket.

I think we have that exact helper function in deprecated.php, _funky_javascript_callback().

Looks like it - duplicated code FTL

comment:12 @nacin4 years ago

(In [16313]) Remove more create_function calls. props huichen, see #14424.

comment:13 follow-up: @nacin4 years ago

huichen was the Google Summer of Code student for HipHip, and one of his tasks was to make WordPress run on it.

Last commit came from here: https://github.com/huichen/wordpress/commit/a1cf1b0b3c496f4f8ee0295200208fada877eae8

comment:14 @automattor4 years ago

(In [16317]) Typo fix. see #14424.

comment:15 @nacin4 years ago

HipHop ticket from Hui: #14642

comment:16 @markjaquith4 years ago

  • Milestone changed from 3.1 to Future Release

Done with this for 3.1.

comment:17 in reply to: ↑ 13 @hakre4 years ago

Replying to nacin:

(In [16313]) Remove more create_function calls. props huichen, see #14424.

And Replying to nacin:

huichen was the Google Summer of Code student for HipHop, and one of his tasks was to make WordPress run on it.

Related: #16059

@ocean904 years ago

comment:18 @ocean904 years ago

  • Keywords has-patch added; needs-patch removed

In r16313 _wp_tag_cloud_name_sort_cb and _wp_tag_cloud_count_sort_cb are introduced but not used. Patch will fix it.

comment:19 @ocean904 years ago

  • Milestone changed from Future Release to 3.3

comment:20 @ryan3 years ago

In [18814]:

Use _wp_tag_cloud_count_sort_cb instead of an anonymous function. Props ocean90. see #14424

comment:21 @ryan3 years ago

  • Milestone changed from 3.3 to Future Release

Punting any further work from 3.3.

comment:22 @nacin3 years ago

In [18815]:

Make the sort-by-object-property functions generic. see #14424.

comment:23 @rmccue18 months ago

  • Keywords has-patch removed

PHP is considering deprecating create_function in 5.6 (~2015). This isn't accepted or anywhere close yet, but as a matter of best practice, we should avoid using it in core.

With the exception of the usage in POMO and wp_generate_tag_cloud, these seem pretty trivial to convert to normal functions. Alternatively, we can wait until it's accepted and nearing release, by which point I hazard a guess we can consider 5.3+ with closures anyway.

comment:24 @nacin18 months ago

I am all for doing whatever it takes to removing all uses of create_function(), even outright refactoring.

@obenland15 months ago

comment:25 follow-up: @obenland15 months ago

  • Keywords has-patch added

Patch converts the remaining ones, outside of POMO and the AtomParser class.

I noticed that importers were sorted twice, once with strcmp() and once with strnatcasecmp(). Is it okay to go with the latter in both cases?

comment:26 @nacin14 months ago

  • Component changed from Optimization to General
  • Focuses performance added
  • Milestone changed from Future Release to 3.9

comment:27 in reply to: ↑ 25 @markjaquith13 months ago

Replying to obenland:

I noticed that importers were sorted twice, once with strcmp() and once with strnatcasecmp(). Is it okay to go with the latter in both cases?

Should be fine.

@markjaquith13 months ago

refresh

@markjaquith13 months ago

rename one of the callbacks to be more generic

@markjaquith13 months ago

without modifying a superglobal

comment:28 @markjaquith13 months ago

So, I don't know whether that $_GET['paged'] superglobal modification was just for the limit, or if other things depend on it. I really don't like it, and would like to remove it.

@nacin13 months ago

@markjaquith13 months ago

merge 14424.4.diff and 14424.5.diff

@markjaquith13 months ago

Add a test for get_importers() ordering

comment:29 @markjaquith13 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 27373:

Eliminate some of our last remaining create_function() instances

  • Moved some into private function callbacks
  • Eliminated some that weren't necessary anymore

props obenland, markjaquith, nacin. fixes #14424

comment:30 follow-up: @nacin13 months ago

In 27374:

Slight renaming in our callback in [27373]. see #14424.

comment:31 @nacin13 months ago

In 27376:

Accept nooped plurals in wp_generate_tag_cloud() / wp_tag_cloud().

Renders topic_count_text_callback more or less obsolete. It can still be used, but passing a plural is easier.

fixes #27262. see #7989, #14424.

comment:32 in reply to: ↑ 30 @lpointet13 months ago

Replying to nacin:

In 27374:

Slight renaming in our callback in [27373]. see #14424.

It seems _uasort_by_first_member callback was directly used in wp-admin/import.php (line 77). This commit leads then to the following PHP warning:

uasort() expects parameter 2 to be a valid callback, function '_uasort_by_first_member' not found or invalid function name

The callback should be updated in that file too, or we may use a new function to merge data from get_importers() and wp_get_popular_importers()?

comment:33 @SergeyBiryukov13 months ago

In 27465:

Use correct function name. see [27374].

props lpointet.
see #14424.

comment:34 @nacin13 months ago

In 27501:

Assign correct variable.

props nendeb55.
fixes #27351. see #14424, [27373].

@TobiasBg12 months ago

Replace a forgotten $GET['paged']

comment:35 @TobiasBg12 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

[27373] forget one instance of $_GET['paged'] (this causes a notice e.g. in the old media uploader), see 14424-fix.diff.

comment:36 @nacin12 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 27675:

Use correct variable. see [27373].

props TobiasBg.
fixes #14424.

Note: See TracTickets for help on using tickets.