#56293 closed defect (bug) (fixed)
Use 'MINUTE_IN_SECONDS' for consistency in 'update.php' file
Reported by: | hztyfoon | Owned by: | audrasjb |
---|---|---|---|
Milestone: | 6.1 | Priority: | low |
Severity: | normal | Version: | |
Component: | Administration | Keywords: | has-patch dev-feedback |
Focuses: | coding-standards | Cc: |
Description (last modified by )
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)
Change History (45)
#1
in reply to:
↑ description
@
2 years ago
#3
@
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
#6
@
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:
↓ 9
@
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.
#9
in reply to:
↑ 7
@
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.
#10
@
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.
#11
@
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
@
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
@
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.
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:
↓ 18
@
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
@
2 years ago
Replying to audrasjb:
@peterwilsoncc suggested to replace
MINUTE_IN_SECONDS / 2
with0.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
@
2 years ago
Alright, let's at least keep the consistency with [53714] and stick with the division.
#21
follow-ups:
↓ 22
↓ 24
@
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
#22
in reply to:
↑ 21
@
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
@
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:
↓ 25
@
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 exceeds59
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
@
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 both30 * MINUTE_IN_SECONDS
andHOUR_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 exceeds59
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:
↓ 27
@
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:
↓ 29
↓ 30
↓ 31
@
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
@
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
@
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:
↓ 32
@
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?
#31
in reply to:
↑ 27
@
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
@
2 years ago
Replying to rudlinkon:
So for this case 90 minutes will be
90 * MINUTE_IN_SECONDS
instead of1.5 * HOUR_IN_SECONDS
orHOUR_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
@
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
@
2 years ago
I have submitted PR 3047 so that we can see the proposed changes in place and move from there.
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
2 years ago
2 years ago
#42
Committed in https://core.trac.wordpress.org/changeset/54113
⬆️ Sorry, I linked wrong ticket there. I meant this commit: [53714]