Make WordPress Core

Opened 12 years ago

Closed 8 years ago

Last modified 8 years ago

#21330 closed enhancement (fixed)

Allow filtering of expiration in set_transient()

Reported by: wpsmith's profile wpsmith Owned by: chriscct7's profile chriscct7
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: Cache API Keywords: has-patch commit
Focuses: Cc:

Description

I recently needed to change an expiration of a transient and expected pre_set_site_transient_* filter to be able to do that. However, the filter only affects the value. I recommend adjusting this to affect both.

Attachments (12)

21330.patch (674 bytes) - added by wpsmith 12 years ago.
Site Transient Filter Adjustment, first pass
21330_opt3.patch (538 bytes) - added by wpsmith 12 years ago.
Option 3: New filter
21330_opt2.patch (534 bytes) - added by wpsmith 12 years ago.
Option 2: Same filter different 2nd value.
21330_opt1.patch (882 bytes) - added by wpsmith 12 years ago.
Option 1: Reworked, removed extract()
21330_opt4.patch (919 bytes) - added by wpsmith 12 years ago.
Option 4: Combo of Option 3 & modified Option 1
21330.diff (1.2 KB) - added by nacin 12 years ago.
21330.2.patch (1.6 KB) - added by chriscct7 8 years ago.
21330.3.patch (1.3 KB) - added by chriscct7 8 years ago.
21330.4.patch (1.6 KB) - added by chriscct7 8 years ago.
21330.5.patch (1.1 KB) - added by chriscct7 8 years ago.
21330.6.patch (1.1 KB) - added by chriscct7 8 years ago.
extra space in the previous patch
21330.7.patch (1.2 KB) - added by chriscct7 8 years ago.
Updated filter names

Download all attachments as: .zip

Change History (49)

#1 follow-up: @obenland
12 years ago

AFAIK, filters are only supposed to filter the first supplied argument. So $expirationprobably should get its own filter.

#2 in reply to: ↑ 1 @wpsmith
12 years ago

Replying to obenland:

AFAIK, filters are only supposed to filter the first supplied argument. So $expirationprobably should get its own filter.

Yes that's right. I knew that! Adjusted patch for one filter.

@wpsmith
12 years ago

Site Transient Filter Adjustment, first pass

#3 follow-up: @nacin
12 years ago

extract( $options, EXTR_OVERWRITE ); is dangerous and not something we're likely going to do here, as it doesn't fit patterns found throughout. Please do not replace patches, we can't see what changed — I actually can't tell if this was the original patch, or a replacement.

#4 in reply to: ↑ 3 @wpsmith
12 years ago

Replying to nacin:

extract( $options, EXTR_OVERWRITE ); is dangerous and not something we're likely going to do here, as it doesn't fit patterns found throughout. Please do not replace patches, we can't see what changed — I actually can't tell if this was the original patch, or a replacement.

My apologies. The first one was VERY much similiar to the one that's there now using extract. I've added a couple more options.

@wpsmith
12 years ago

Option 3: New filter

@wpsmith
12 years ago

Option 2: Same filter different 2nd value.

@wpsmith
12 years ago

Option 1: Reworked, removed extract()

#5 follow-up: @nacin
12 years ago

I don't understand how 21330_opt2.patch would work in a plugin.

Could you go a bit more into your use case, perhaps some sample code?

#6 in reply to: ↑ 5 @wpsmith
12 years ago

Replying to nacin:

I don't understand how 21330_opt2.patch would work in a plugin.

Could you go a bit more into your use case, perhaps some sample code?

The expiration time is always set (right? if not set, it's set to 0 by the time of the filter) and is always an integer. For example, wp_check_browser_version() has a set expiration that is known. So...assuming I knew the $key (93ce43dc55bd29ecc99e83ca12b5d7f4) in this example.

add_filter( 'pre_set_site_transient_browser_93ce43dc55bd29ecc99e83ca12b5d7f4', 'wps_change_browser_transient' );
function wps_change_browser_transient( $val ) {
    if ( is_int( $val ) && 604800 == $val )
        $val = 1209600;

    return $val;
}

However, I am not convinced that this is the best solution...

Last edited 12 years ago by wpsmith (previous) (diff)

@wpsmith
12 years ago

Option 4: Combo of Option 3 & modified Option 1

#7 @obenland
12 years ago

In 21330_opt4.patch, what do you need the option part for? Value and expiration are both filterable at this point.

@nacin
12 years ago

#8 @nacin
12 years ago

If we were to do this, it would probably look like 21330.diff.

#9 @nacin
12 years ago

  • Type changed from defect (bug) to enhancement

#10 @nacin
12 years ago

  • Summary changed from Change pre_set_site_transient_* to affect $expiration to Allow filtering of expiration in set_transient()

#11 @DrewAPicture
12 years ago

  • Cc xoodrew@… added

#12 @mordauk
10 years ago

I haven't personally had the need to do this, but it seems perfectly reasonable that a plugin would want to. For example, in Easy Digital Downloads we use transients for caching some earnings data. If our cache is too aggressive or too weak, a user could better tweak our transient expirations to better suit their site.

#13 @wonderboymusic
10 years ago

  • Keywords dev-feedback removed
  • Milestone changed from Awaiting Review to 3.9

Let's decide soon in 3.9

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.


10 years ago

#15 @nacin
10 years ago

As evidenced by IRC, naming this filter is a pain.

But also:

<?php
add_action( 'set_transient_my_transient', function( $value, $expiration ) {
    $desired_expiration = 600;
    if ( $expiration != $desired_expiration ) {
        set_transient( 'my_transient', $value, $desired_expiration );
    }
});

#16 @nacin
10 years ago

  • Keywords 2nd-opinion added
  • Milestone changed from 3.9 to Future Release

@chriscct7
8 years ago

#17 @chriscct7
8 years ago

  • Keywords commit added; 2nd-opinion removed
  • Milestone changed from Future Release to 4.4
  • Owner set to chriscct7
  • Status changed from new to accepted

Refreshed and good to go

#18 @DrewAPicture
8 years ago

  • Keywords commit removed
  • Owner changed from chriscct7 to wonderboymusic
  • Status changed from accepted to assigned

@chriscct7: Only one instance of the filter needs a hook doc, the second one needs a duplicate hook comment ;-)

#19 @chriscct7
8 years ago

@drewapicture my bad! Thanks

#20 @wonderboymusic
8 years ago

  • Keywords needs-refresh added
  • Owner changed from wonderboymusic to chriscct7

@chriscct7
8 years ago

#21 @chriscct7
8 years ago

  • Keywords commit added; needs-refresh removed

#22 @obenland
8 years ago

  • Keywords commit removed

In 21330.diff the filters had two different names. Was that changed intentionally?

#23 @chriscct7
8 years ago

I can move it back to using the same filter. I thought it'd be easier to set the transients using a single filter. Just a sec

@chriscct7
8 years ago

#24 @chriscct7
8 years ago

  • Keywords commit added

Ok, new patch moves it back to 2 filters and adds a full docbloc for the second one now that it's a unique filter.

#25 @DrewAPicture
8 years ago

I'm tempted to recommend that we skip adding the second filter pending the resolution of #28290. I don't want to end up in a situation where we introduce a new "site_transient" filter here in 4.4, and a version later end up having to introduce a new one to match the "network_transient" vernacular.

#26 follow-up: @DrewAPicture
8 years ago

@jeremyfelt: What would be your preference on the naming of the site transient hook? Should we go with "site" or "network" following [34778]?

Last edited 8 years ago by DrewAPicture (previous) (diff)

#27 in reply to: ↑ 26 @jeremyfelt
8 years ago

Replying to DrewAPicture:

@jeremyfelt: What would be your preference on the naming of the site transient hook? Should we go with "site" or "network" following [34778]?

I think sticking with site makes sense for consistency. I don't foresee a near term strategy for handling cross-network transients, so these functions will probably stay as is for a bit.

#28 @wonderboymusic
8 years ago

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

In 34865:

Transients, add filters for $expiration:

  • 'expiration_pre_set_site_transient_' . $transient
  • 'expiration_pre_set_transient_' . $transient


Props chriscct7, wpsmith, nacin.
Fixes #21330.

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


8 years ago

#30 @chriscct7
8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@ocean90 and I discussed right after this was committed to changing the filter names to expiration_of_transient_{transient} and expiration_of_site_transient_{transient}. Attached is a patch for that.

@chriscct7
8 years ago

@chriscct7
8 years ago

extra space in the previous patch

@chriscct7
8 years ago

Updated filter names

#31 @chriscct7
8 years ago

Patch 7 uses pre_set_transient_{$transient}_expiration and pre_set_site_transient_{$transient}_expiration which @DrewAPicture preferred

#32 @SergeyBiryukov
8 years ago

Looking at the IRC discussion, pre_set_transient_{$transient}_expiration is still a potential namespace conflict, which we were trying to avoid.

Let's go with expiration_of_(site_)transient_{$transient}.

#33 @chriscct7
8 years ago

  • Keywords has-patch commit removed
  • Resolution set to fixed
  • Status changed from reopened to closed

Per comment:32 closing as this is what is currently in, per [34865]

#34 @chriscct7
8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Actually its not.

#35 @chriscct7
8 years ago

  • Keywords has-patch commit added

21330.6.patch is the one requested in comment:32

#36 @ocean90
8 years ago

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

In 34877:

Transients: Rename filters added in [34865].

  • expiration_pre_set_transient_$transient => expiration_of_transient_$transient
  • expiration_pre_set_site_transient_$transient => expiration_of_site_transient_$transient

Fix hook docs.

Props chriscct7.
Fixes #21330.

#37 @SergeyBiryukov
8 years ago

In 34878:

Transients: Clarify the expiration_of_site_transient_$transient filter description.

See #21330.

Note: See TracTickets for help on using tickets.