Make WordPress Core

Opened 12 years ago

Closed 7 weeks ago

Last modified 7 weeks ago

#21521 closed enhancement (fixed)

Audit use of set_time_limit()

Reported by: ryan's profile ryan Owned by: jorbin's profile jorbin
Milestone: 6.7 Priority: normal
Severity: normal Version: 3.4.1
Component: Bootstrap/Load Keywords: dev-feedback good-first-bug has-patch
Focuses: docs Cc:

Description

Core calls this half a dozen times. The call in wp_get_http() interferes with unit tests. Unit tests will terminate 60 seconds after wp_get_http() is called. Let's justify each use of set_time_limit() and remove what we can.

Change History (14)

#2 @kurtpayne
12 years ago

  • Cc kpayne@… added

#3 @kurtpayne
12 years ago

wp-admin/includes/class-wp-upgrader.php line @ line 174

@set_time_limit( 300 );

Changeset [11005] and ticket #7875


wp-admin/includes/update-core.php @ line 517

@set_time_limit( 300 );

Changeset [9164] and ticket #5560


wp-admin/network/sites.php @ line 97

wp-admin/network/sites.php @ line 103

set_time_limit( 60 );

This was traced back to the import of ms-edit.php in changeset [12603]
One of these came from the mu trac in changeset 860.

The second one was expanded upon in changeset 1237.


wp-includes/class-pop3.php @ line 60

set_time_limit($timeout);

wp-includes/class-pop3.php @ line 67

set_time_limit($timeout);

These are in a 3rd party library


wp-includes/comment @ line 1817

@ set_time_limit( 60 );

It came originally from comment-functions.php and I lost the trail there


wp-includes/functions.php @ line 492

@set_time_limit( 60 );

Original changeset [2416]

#4 @SergeyBiryukov
12 years ago

The first two instances from comment:3 were both introduced in [9164]. The one from wp_update_core() was moved to WP_Upgrader.

The last two instances were both introduced in [1812].

The one from do_enclose() was moved to wp_get_http_headers() and after [6390] ended up in wp_get_http().

The one from pingback() is still there.

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

#6 @ryan
12 years ago

In [22259]:

Remove set_time_limit() from sites.php. Props dllh. fixes #19486 see #21521

#7 @ryan
12 years ago

  • Milestone changed from 3.5 to Future Release
  • Type changed from defect (bug) to enhancement

#8 @nacin
11 years ago

  • Component changed from General to Bootstrap/Load

#9 @chriscct7
9 years ago

  • Keywords needs-patch dev-feedback added

What is left to be done?

#10 @jorbin
3 months ago

  • Focuses docs added
  • Keywords good-first-bug added
  • Milestone set to 6.7

Let's get this finished up for 6.7! I think the writing the remaining inline docs needed would be a good first bug for someone.

Here are all of the current uses of set_time_limit:

  • wp-admin/includes/file.php - wp_edit_theme_plugin_file

This has an inline doc and is necessary for the loopback to ensure plugin file updates don't cause white screens

  • wp-admin/includes/update-core.php - update_core

This could use an inline doc.

  • wp-admin/includes/class-wp-automatic-updater.php - WP_Automatic_Updater::update

This includes an inline doc about the loopback request.

  • wp-admin/includes/class-wp-upgrader.php - ‎WP_Upgrader::install_package

This could use an inline doc.

  • wp-admin/includes/ajax-actions.php - wp_ajax_get_revision_diffs

This is not documented. It was introduced to fix #24757.

  • wp-includes/ID3/module.audio.mp3.php

This is a 3rd party library. I'm ok with it as is.

  • wp-includes/PHPMailer/SMTP.php

This is a 3rd party library. I'm ok with it as is.

  • wp-includes/comment.php - pingback

This could use an inline comment explaining that this function is used to call external servers. It's been in WP basically forever.

  • wp-includes/class-pop3.php

I think an inline comment could be added but it's also fine if one isn't since this is sort of a 3rd party code.

  • wp-includes/deprecated.php

As this code is deprecated, I don't think it's necessary for there to be an inline doc.

This ticket was mentioned in Slack in #core by jorbin. View the logs.


7 weeks ago

This ticket was mentioned in PR #7378 on WordPress/wordpress-develop by sleddd.


7 weeks ago
#12

  • Keywords has-patch added; needs-patch removed

Wrote remaining inline docs for each instance of set_time_limit to help complete set_time_limit() audit.

Trac ticket: https://core.trac.wordpress.org/ticket/21521

#13 @jorbin
7 weeks ago

  • Owner set to jorbin
  • Resolution set to fixed
  • Status changed from new to closed

In 59039:

Bootstrap/Load: Ensure uses of set_time_limit are documented why.

set_time_limit can cause unexpected behavior so it general should be avoided. There are instances though where they should be used so those instances should be properly documented.

Props Rcrayno, ryan, kurtpayne, jorbin.
Fixes #21521. See #19487.

#14 @jorbin
7 weeks ago

@craynor - My apologies, I mistyped your user name. I corrected it so that it shows up on your profile. Thank you for your help here!

Note: See TracTickets for help on using tickets.