Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#56293 closed defect (bug) (fixed)

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

Reported by: hztyfoon's profile hztyfoon Owned by: audrasjb's profile audrasjb
Milestone: 6.1 Priority: low
Severity: normal Version:
Component: Administration Keywords: has-patch dev-feedback
Focuses: coding-standards Cc:

Description (last modified by SergeyBiryukov)

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;

Attachments (2)

56293.patch (812 bytes) - added by arrasel403 2 years ago.
Patch file added for this ticket
56293.diff (516 bytes) - added by krupalpanchal 2 years ago.

Download all attachments as: .zip

Change History (45)

#1 in reply to: ↑ description @hztyfoon
2 years ago

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]

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

#2 @rudlinkon
2 years ago

  • Keywords needs-patch added

#3 @audrasjb
2 years ago

  • Milestone changed from Awaiting Review to 6.1
  • Owner set to audrasjb
  • Status changed from new to reviewing

Good point!
Moving for 6.1 consideration.

This ticket was mentioned in PR #3029 on WordPress/wordpress-develop by audrasjb.


2 years ago
#4

  • Keywords has-patch added; needs-patch removed

@arrasel403
2 years ago

Patch file added for this ticket

#5 @krupalpanchal
2 years ago

Added Patch

@krupalpanchal
2 years ago

#6 @GaryJ
2 years ago

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.

#7 follow-up: @hztyfoon
2 years ago

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.

#8 @hztyfoon
2 years ago

oh @GaryJ,
Thanks.

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

Last edited 2 years ago by hztyfoon (previous) (diff)

#9 in reply to: ↑ 7 @audrasjb
2 years ago

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.

Last edited 2 years ago by audrasjb (previous) (diff)

#10 @rudlinkon
2 years ago

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

Last edited 2 years ago by rudlinkon (previous) (diff)

#11 @SergeyBiryukov
2 years ago

  • Description modified (diff)
  • Keywords changes-requested added

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,

#12 @hztyfoon
2 years ago

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.

#13 @rudlinkon
2 years ago

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

hz-tyfoon commented on PR #3029:


2 years ago
#14

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.

#15 @hztyfoon
2 years ago

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

hz-tyfoon commented on PR #3029:


2 years ago
#16

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

#17 follow-up: @audrasjb
2 years ago

  • Keywords dev-feedback added; changes-requested removed
  • Priority changed from normal to low

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

#18 in reply to: ↑ 17 @SergeyBiryukov
2 years ago

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.

#19 @audrasjb
2 years ago

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

#20 @audrasjb
2 years ago

  • Keywords dev-feedback removed

#21 follow-ups: @costdev
2 years ago

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

Last edited 2 years ago by costdev (previous) (diff)

#22 in reply to: ↑ 21 @hztyfoon
2 years ago

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

#23 @costdev
2 years ago

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

#24 in reply to: ↑ 21 ; follow-up: @SergeyBiryukov
2 years ago

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.

#25 in reply to: ↑ 24 @hztyfoon
2 years ago

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?

#26 follow-up: @costdev
2 years ago

  • Keywords dev-feedback added

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?

#27 in reply to: ↑ 26 ; follow-ups: @SergeyBiryukov
2 years ago

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.

#28 @costdev
2 years ago

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!

#29 in reply to: ↑ 27 @hztyfoon
2 years ago

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

#30 in reply to: ↑ 27 ; follow-up: @rudlinkon
2 years ago

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?

Last edited 2 years ago by rudlinkon (previous) (diff)

#31 in reply to: ↑ 27 @krupalpanchal
2 years ago

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.

#32 in reply to: ↑ 30 @hztyfoon
2 years ago

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.

#33 @costdev
2 years ago

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.

This ticket was mentioned in PR #3047 on WordPress/wordpress-develop by costdev.


2 years ago
#34

  • 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

#35 @costdev
2 years ago

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

#36 @hztyfoon
2 years ago

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

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


2 years ago

#38 @audrasjb
2 years ago

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

#39 @robinwpdeveloper
2 years ago

PR 3047 looks good to me as well.

#40 @krupalpanchal
2 years ago

PR 3047 Looks good to go.

Version 0, edited 2 years ago by krupalpanchal (next)

#41 @audrasjb
2 years ago

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

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.

audrasjb commented on PR #3029:


2 years ago
#43

closed in favor of PR 3047

Note: See TracTickets for help on using tickets.