Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement new timeout/duration/expiration format. #3047

Conversation

costdev
Copy link
Contributor

@costdev costdev commented Aug 1, 2022

  • If the value can be represented as an integer (not a fractional) number of minutes (hours, etc.), use the appropriate constant multiplied by that number.
  • Otherwise, keep the value as is and add a comment with the units for clarity.

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

If the value can be represented as an integer (not a fractional) number
of minutes (hours, etc.), use the appropriate constant multiplied by that number.

Otherwise, keep the value as is and add a comment with the units for clarity.
@@ -1597,7 +1597,7 @@ public static function get_sizes() {
// The max_execution_time defaults to 0 when PHP runs from cli.
// We still want to limit it below.
if ( empty( $max_execution_time ) ) {
$max_execution_time = 30;
$max_execution_time = 30; // 30 seconds.
}

if ( $max_execution_time > 20 ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't touch this line because the comment below explains the units.

However, this condition tests to see if the max_execution_time is greater than 20, but the comment states:

If the max_execution time is set to lower than 20 seconds.

The comment should be changed to reflect what the condition actually checks:

/*
 * If the max_execution_time is set to more than 20 seconds, reduce it a bit to prevent
 * edge-case timeouts that may happen after the size loop has finished running.
 */

Also note that the current comment does not follow the correct multi-line comment format per Documentation Standards. The edited comment above does.

@costdev costdev marked this pull request as ready for review August 1, 2022 05:33
Copy link

@rudlinkon rudlinkon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me according to the new rules which are mentioned in the Trac ticket.

Copy link

@robinwpdeveloper robinwpdeveloper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@audrasjb
Copy link
Contributor

audrasjb commented Sep 9, 2022

Committed in https://core.trac.wordpress.org/changeset/54113

@audrasjb audrasjb closed this Sep 9, 2022
@costdev costdev deleted the update_timeout_duration_expiration_format branch September 19, 2022 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants