Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#39591 closed enhancement (fixed)

Introduce wp_doing_cron()

Reported by: tfrommen's profile tfrommen Owned by: swissspidy's profile swissspidy
Milestone: 4.8 Priority: normal
Severity: normal Version:
Component: Cron API Keywords: has-patch
Focuses: Cc:

Description

Similar to wp_doing_ajax(), I'd consider a wp_doing_cron() function handy.

Core could make use of it 10 times already (three times of which lookups can be cached), and even more plugin authors.

Patch is about to follow.

Attachments (2)

39591.patch (6.3 KB) - added by tfrommen 7 years ago.
39591.diff (6.3 KB) - added by swissspidy 7 years ago.

Download all attachments as: .zip

Change History (14)

@tfrommen
7 years ago

#1 @rmccue
7 years ago

+1 from me. While it's not really a huge difference between checking a constant and checking a function return (it's also likely less performant), adding filterability might help with cron drop-ins (see this Cavalcade ticket).

@dd32 Do you have an opinion here?

#2 follow-up: @dd32
7 years ago

I don't see any harm in adding this.

It could make life easier for dropins such as cavalcade, but for BC purposes I would expect having to define the constant for a while to come yet, as it's unlikely those checking if cron is in play are going to utilise these methods until they drop support for 4.7 (which is quite some way off I wouls suspect).

Only nit-pick would be that the patch assigns the $doing_cron variable in some cases where it'd otherwise be called repetitively, I'm not sure that level of optimising is needed - but it can't hurt I guess.

#3 in reply to: ↑ 2 ; follow-up: @tfrommen
7 years ago

Replying to dd32:

Only nit-pick would be that the patch assigns the $doing_cron variable in some cases where it'd otherwise be called repetitively, I'm not sure that level of optimising is needed - but it can't hurt I guess.

I just followed that path of the wp_doing_ajax() changes. :) The changeset didn't introduce new variables, but where it made sense, the result of the check (using DOING_CRON) was cached.

And, yeah, caching data that is to be used multiple times (instead of querying it multiple times) is not a bad idea, right? ;)

#4 in reply to: ↑ 3 ; follow-up: @rmccue
7 years ago

Replying to tfrommen:

And, yeah, caching data that is to be used multiple times (instead of querying it multiple times) is not a bad idea, right? ;)

It's not querying though: as a constant, it's a constant and is basically instant; as a function, it's essentially static (small overhead of filtering, but that is never going to be significant). No point caching it.

#5 @schlessera
7 years ago

Regarding the caching bit, I think it would be necessary to keep the internal workings consistent. Otherwise a function might start with wp_doing_cron() giving true, and later continue with it giving false, because a filter might have kicked in.

I don't think these functions are meant to be able to handle changes halfway in-between.

#6 @SergeyBiryukov
7 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.8

#7 in reply to: ↑ 4 ; follow-up: @tfrommen
7 years ago

I'm very happy this is going to get merged sometime soon. :)

Yet I want to address a few things.

Replying to rmccue:

It's not querying though: as a constant, it's a constant and is basically instant; as a function, it's essentially static (small overhead of filtering, but that is never going to be significant). No point caching it.

I'm no English native speaker, so sorry for not using the exact same words you would use here.
That being said, for me, it is querying. It is asking someone else for information. Multiple times, knowing that I'd be needing the information multiple times right from the beginning. And even more, that information isn't going to change (because it is the value of a constant, as we all know).

Caching that information in order to look it up later (which is instant) is a good thing, in my opinion.

Also, since the constant is not defined in all situations (in fact, in WordPress core, it is only defined when doing a CRON task), noone can simply check the constant. They'd have to check defined( 'DOING_CRON' ) && DOING_CRON, which is not only looking up a constant.

To make that clear, I am not saying, nor assuming, to really boost anyone's WordPress site by these micro-optimizations. That's just nonsene. However, caching that kind of data clearly isn't a bad thing at all.

Furthermore, you, Ryan, said it yourself: wrapping the check in a function will introduce some (tiny) overhead. So I don't see your reasoning in doing this multiple times, instead of only once, for the cost of a local variable.

As to the introduction of that function in general. Even if the overhead would be bigger, having a public API (that is not using implementation details!) is a good thing as well. Currently, the only thing one can do is to check the constant. If the storage of the CRON status definition ever were to change (I know, I know: won't happen; but still), all of the code directly working with the constant would have to be adapted.
If there were a function, which is part of the public API, and thus to be used in favor of checking the constant, one could, some day far far away in the future, change how doing a CRON task is defined.

Having the possibility to filter the value, is a big plus, too, yes.
I believe this is one of the major reasons for having wp_doing_ajax(), isn't it?

Just wanted to leave this here. For discussion. :)

Last edited 7 years ago by tfrommen (previous) (diff)

#8 in reply to: ↑ 7 @rmccue
7 years ago

Replying to tfrommen:

Also, since the constant is not defined in all situations (in fact, in WordPress core, it is only defined when doing a CRON task), noone can simply check the constant. They'd have to check defined( 'DOING_CRON' ) && DOING_CRON, which is not only looking up a constant.

My point was that the defined check doesn't really cost anything, apart from the mental overhead of writing it. :) (Specifically, it's 3 PHP opcodes for `defined('X') && X, which is basically an instant lookup.)

To make that clear, I am not saying, nor assuming, to really boost anyone's WordPress site by these micro-optimizations. That's just nonsene. However, caching that kind of data clearly isn't a bad thing at all.

I'm more just saying that you're not saving anything by storing in a variable. The performance is never really going to matter. Saving in a variable does increase the indirection though. Compare:

$foo = a();
$bar = b();
$baz = c();
echo $foo . $bar . $baz;

vs

echo a() . b() . c();

It's more that there's no real need for a variable. If I'm reading the code, I now potentially need to read the variable declaration rather than just seeing it inline with the rest of the code. The clarity from the direct use can be better, and the performance difference is negligible, so might as well go with the clearer option. :)

(I don't really care, but there are downsides to unneeded caching. It's not fair to say it's always a good thing. :))

Currently, the only thing one can do is to check the constant. If the storage of the CRON status definition ever were to change (I know, I know: won't happen; but still), all of the code directly working with the constant would have to be adapted.

It's possible already to do basically whatever you want with cron replacements, so I'm not sure I see that argument.

---

The main argument against this is that it's essentially a "mode" of WordPress, and therefore shouldn't be able to change during a request. It usually doesn't really make sense to be in cron mode, then not be in cron mode.

However, unit tests do occasionally want to do things like this, so having it filterable is very handy for that. This was one of the key points on #25669 (wp_doing_ajax()).

One other thing I forgot to note: cron should always be lowercase. It's an abbreviation of "chronological", not an acronym. Apart from that (and potentially the variable), patch looks good to me.

@swissspidy
7 years ago

#9 follow-up: @swissspidy
7 years ago

39591.diff updates the inline docs.

See also #38673 for a wp_disallow_file_mods() proposal.

#10 in reply to: ↑ 9 @tfrommen
7 years ago

Replying to swissspidy:

39591.diff updates the inline docs.

See also #38673 for a wp_disallow_file_mods() proposal.

Thanks, @swissspidy.

As a side note: if you don't like CRON in uppercase, someone should have a look at wp-cron.php. This is where I took it from in the first place. ;)

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


7 years ago

#12 @swissspidy
7 years ago

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

In 40575:

Cron API: Add a new wp_doing_cron() helper function.

This replaces DOING_CRON checks via the constant.

Props tfrommen.
Fixes #39591.

Note: See TracTickets for help on using tickets.