Use 'MINUTE_IN_SECONDS' for consistency in 'update.php' file

[hztyfoon]

In the 'wp-includes/update.php' file there are multiple instances where variable $timeout set to plain integer 30 like:

$timeout = 30;

I think It should be like this (MINUTE_IN_SECONDS also used multiple time in this file) as done here in this commit [53714]:

$timeout = MINUTE_IN_SECONDS / 2;

[hztyfoon]

I think It should be like this (MINUTE_IN_SECONDS also used multiple time in this file) as done here in this commit #53714:

⬆️ Sorry, I linked wrong ticket there. I meant this commit: [53714]

[audrasjb]

Good point!
Moving for 6.1 consideration.

Trac ticket: https://core.trac.wordpress.org/ticket/56293#comment:3

[arrasel403]

Patch file added for this ticket

[krupalpanchal]

Added Patch

[GaryJ]

The 56293.diff patch seems unnecessarily complex. Just because we want to say 2 hours or 5 hours, doesn't mean we need to derive the 2 or the 5 from some division of the MINUTE_IN_SECONDS constant.

[hztyfoon]

hey @krupalpanchal ,
I checked your 56293.diff​ and looks to me your changes aren't what I suggested.

To me,
2 * HOUR_IN_SECONDS makes more sense than ( MINUTE_IN_SECONDS / 30 ) * HOUR_IN_SECONDS
same goes for the other line's change.

Because, When I see 2 * HOUR_IN_SECONDS it clearly makes me understand that it's twice an hour in seconds but when there's something like ( MINUTE_IN_SECONDS / 30 ) * HOUR_IN_SECONDS it makes it more hard to read.

My point was these constants like (MINUTE_IN_SECONDS, HOUR_IN_SECONDS) can make the timing numbers more readable for a non programmer. But, I don't see how your changes make it more readable.

[hztyfoon]

oh @GaryJ,
Thanks.

U've beat me by almost MINUTE_IN_SECONDS * 2 seconds. 😊

[audrasjb]

The 56293.diff patch seems unnecessarily complex. Just because we want to say 2 hours or 5 hours, doesn't mean we need to derive the 2 or the 5 from some division of the MINUTE_IN_SECONDS constant.

2 * HOUR_IN_SECONDS makes more sense than ( MINUTE_IN_SECONDS / 30 ) * HOUR_IN_SECONDS
same goes for the other line's change.

Agreed.

[rudlinkon]

@arrasel403 thanks for your patch but @audrasjb doing some extra care about this issue so I think the PR has a better solution than the patch. But you ( @arrasel403 ) are most welcome as a new contributor.

[SergeyBiryukov]

Thanks everyone!

// Three seconds, plus one extra second for every 10 themes.
$timeout = MINUTE_IN_SECONDS / 20 + (int) ( count( $themes ) / 10 )

I think MINUTE_IN_SECONDS / 20 is also too complicated :) Keeping that as is, especially with the "Three seconds" comment above, seems more readable to me:

// Three seconds, plus one extra second for every 10 themes.
$timeout = 3 + (int) ( count( $themes ) / 10 );

Same goes for this:

'timeout'    => $doing_cron ? MINUTE_IN_SECONDS / 2 : MINUTE_IN_SECONDS / 20,

This might be more readable:

// Half a minute when doing cron tasks, 3 seconds otherwise.
'timeout'    => $doing_cron ? MINUTE_IN_SECONDS / 2 : 3,

[hztyfoon]

Thanks @SergeyBiryukov ,

I'm not sure if it's a coincident but I was just about to make the same comment in the PR. Agreed about MINUTE_IN_SECONDS / 20 being complicated.

[rudlinkon]

Thank you @SergeyBiryukov, it would be better if you suggest this change request in the PR.

Hey @audrasjb,
I can see U've used MINUTE_IN_SECONDS / 20 in line number 190, 411 & 692.
In my humble opinion, looks to me using MINUTE_IN_SECONDS / 20 also makes it more complicated & less readable.

@SergeyBiryukov also suggesting the same.
So, I think may be those 3 changes may not be necessary.

[hztyfoon]

Thanks @rudlinkon for your suggestion.
I made the comment in the PR.

thanks @peterwilsoncc for reviewing.

• 0.5 * MINUTE_IN_SECONDS is possibly ok

This one also sounds solid, readable & performance firendly to me.

The reviews of this PR didn't show up in the trac.
Hopefully, this comment is going to show up in the trac ticket.
😊

[audrasjb]

PR updated as per @SergeyBiryukov 's proposal.
@peterwilsoncc suggested to replace MINUTE_IN_SECONDS / 2 with 0.5 * MINUTE_IN_SECONDS.

I personally find the former more readable than the latter.
That's not super important, but let's collect some feedback from other committers before shipping this tiny enhancement ;)

[SergeyBiryukov]

Replying to audrasjb:

@peterwilsoncc suggested to replace MINUTE_IN_SECONDS / 2 with 0.5 * MINUTE_IN_SECONDS.

As noted in comment:10:ticket:55647, the former gives int(30) while the latter gives float(30), so we might need to write it like this to avoid unexpected type changes:

(int) ( 0.5 * MINUTE_IN_SECONDS )

That said, the timeout parameter of WP_Http::request() is documented as float, so a float may be fine here, and casting to (int) may be redundant.

I don't have a strong preference on the format we choose here, but ideally the instances from [53714] should be updated to use the same format.

[audrasjb]

Alright, let's at least keep the consistency with [53714] and stick with the division.

[costdev]

Taking a snippet from ​a review comment I left on the PR:

IMO, it's reasonable to expect a reader won't think:
30 milliseconds : 3 milliseconds or 30 minutes : 3 minutes.

Instead, for known values, I'd suggest that we only make use of MINUTE_IN_SECONDS when the value exceeds 59 seconds.

i.e. 15, 30, 55, MINUTE_IN_SECONDS, 2 * MINUTE_IN_SECONDS, 30 * MINUTE_IN_SECONDS, HOUR_IN_SECONDS

[hztyfoon]

Replying to costdev:

Taking a snippet from ​a review comment I left on the PR:

Thanks @costdev 😊
I've checked your review in the PR. Timeout seconds like 35, 55 (may be even 45) doesn't sound standard to me. I'm not sure if these are used anywhere in the core codebase. If these are used, then I don't suppose using MINUTE_IN_SECONDS / 2 + 5 instead of 35 would be more readable. Same goes for 55 & 45.

for 30 however, both MINUTE_IN_SECONDS / 2 & 0.5 * MINUTE_IN_SECONDS can make more sense than plain 30 IMO.

Just noting, if your proposed for known values, I'd suggest that we only make use of MINUTE_IN_SECONDS when the value exceeds 59 seconds is implemented then we may do that throughout the codebase for consistency. Same goes for the 4 instances of MINUTE_IN_SECONDS / 2 in [53714].

[costdev]

@hztyfoon Yep, 35, 45, 55 aren't standard timeouts, they were just examples to show how we could just hardcode < 60 and use MINUTE_IN_SECONDS for 60+.

It's highly unlikely that the reader will interpret $timeout = 30 as referring to milliseconds or minutes, especially since 30 minutes would just be written as 30 * MINUTE_IN_SECONDS.

I see it as much more helpful towards consistency if we only use the constants when it's additive, so that we don't end up with examples like 30 * MINUTE_IN_SECONDS and HOUR_IN_SECONDS / 2 in the codebase.

[SergeyBiryukov]

Replying to costdev:

Instead, for known values, I'd suggest that we only make use of MINUTE_IN_SECONDS when the value exceeds 59 seconds.

I agree, this makes sense.

For the four instances of MINUTE_IN_SECONDS / 2 in [53714], that could be a comment instead of the constant:

define( 'FS_TIMEOUT', 30 ); // 30 seconds.

[hztyfoon]

Thanks a lot @costdev 👍👍👍
Great point. Didn't get it earlier, got it now.

If I understand correctly, one of the point @costdev made is (written in my own english 👇):

If MINUTE_IN_SECONDS / 2 is implemented then there can be a point/logic/explanation for using both 30 * MINUTE_IN_SECONDS and HOUR_IN_SECONDS / 2.
And that wouldn't be helpful towards consistency.

This can also make the case for possibly replace MINUTE_IN_SECONDS / 2 with 0.5 * MINUTE_IN_SECONDS as suggested by @peterwilsoncc in the PR review.

So, I think the following proposed change by @SergeyBiryukov is better.

For the four instances of MINUTE_IN_SECONDS / 2 in [53714], that could be a comment instead of the constant:

define( 'FS_TIMEOUT', 30 ); // 30 seconds.

Also,
just came to mind about following

Replying to SergeyBiryukov:

Replying to costdev:

Instead, for known values, I'd suggest that we only make use of MINUTE_IN_SECONDS when the value exceeds 59 seconds.

in the case of 90, wouldn't "plain 90 with an added comment beside it" be better than 1.5 * MINUTE_IN_SECONDS? [note: `90` exceeds `59`]
Any thought?

[costdev]

in the case of 90, wouldn't "plain 90 with an added comment beside it" be better than 1.5 * MINUTE_IN_SECONDS? [note: `90` exceeds `59`] Any thought?

I think that's a fair point. "90 seconds" is more commonly used than "one and a half minutes" or "a minute and a half". While it breaks the consistency of exceeds 59, I believe it's the only edge case.

That would mean, going forward, we would have (constant names are obviously not real, just examples):

<?php
define( 'WAIT', 5 ); // 5 seconds

define( 'RETRYING_IN', 10 ); // 10 seconds

define( 'UNZIP_TIMEOUT', 30 ); // 30 seconds

define( 'DB_UPGRADE_TIMEOUT', MINUTE_IN_SECONDS );

define( 'UNDERWATER_TIMEOUT', 90 ); // 90 seconds

define( 'FREE_PIZZA_TIMEOUT', 30 * MINUTE_IN_SECONDS );

define( 'ONE_HOUR_PHOTO_TIMEOUT', HOUR_IN_SECONDS );

What does everyone think of that format?

[SergeyBiryukov]

Replying to costdev:

What does everyone think of that format?

Sounds good. I guess the rule of thumb would be:

• 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.

[costdev]

Sounds good. I guess the rule of thumb would be:

• 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.

Nicely summarized @SergeyBiryukov, that makes it really clear!

[hztyfoon]

Replying to SergeyBiryukov:

I guess the rule of thumb would be:

• 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.

Thanks @SergeyBiryukov.
That sounds very clear 👍👍👍.

[rudlinkon]

Replying to SergeyBiryukov:

Sounds good. I guess the rule of thumb would be:

• 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.

So for this case 90 minutes will be 90 * MINUTE_IN_SECONDS instead of 1.5 * HOUR_IN_SECONDS or HOUR_IN_SECONDS + 30 * MINUTE_IN_SECONDS and all the like values will be so so, isn't it?

[krupalpanchal]

Replying to SergeyBiryukov:

Replying to costdev:

What does everyone think of that format?

Sounds good. I guess the rule of thumb would be:

• 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.

That sounds clear 👍. Using a constant is more clear than making an arithmetic formula.

[hztyfoon]

Replying to rudlinkon:

So for this case 90 minutes will be 90 * MINUTE_IN_SECONDS instead of 1.5 * HOUR_IN_SECONDS or HOUR_IN_SECONDS + 30 * MINUTE_IN_SECONDS and all the like values will be so so, isn't it?

Thannks a ton @rudlinkon.
That's really a good point. 👍👍👍
I'm not sure if 90 * MINUTE_IN_SECONDS make more sense than 1.5 * HOUR_IN_SECONDS.

Let's see what others think about this.

[costdev]

I think both make sense, but 90 * MINUTE_IN_SECONDS is more familiar than 1.5 * HOUR_IN_SECONDS - think of some sport event durations, for example.

It also follows the rule of thumb that @SergeyBiryukov laid out:

I guess the rule of thumb would be:

• 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.

• 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

[costdev]

I have submitted ​PR 3047 so that we can see the proposed changes in place and move from there.

[hztyfoon]

Thanks @costdev,
Checked the PR. Looking pretty good to me.

[audrasjb]

The current approach (​PR 3047) looks good to me.

[robinwpdeveloper]

​PR 3047 looks good to me as well.

[krupalpanchal]

​PR 3047 Looks good to go.

[audrasjb]

In 54113:

Coding Standards: Clarify time units for various timeout or expiration values.

This changeset implements a clearer and more consistent timeout/duration/expiration format. It updates time durations used in various files, as per WordPress coding standards:

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

Follow-up to [11823], [13177], [21996], [37747], [53714].

Props hztyfoon, audrasjb, arrasel403, krupalpanchal, GaryJ, SergeyBiryukov, peterwilsoncc, rudlinkon, costdev, robinwpdeveloper.
Fixes #56293.
See #55647.

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

closed in favor of PR 3047