WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 4 weeks ago

Last modified 4 weeks ago

#41699 closed defect (bug) (fixed)

Expired transients are not flushed by cron

Reported by: bor0 Owned by: dd32
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.8.1
Component: Options, Meta APIs Keywords: has-patch
Focuses: Cc:

Description

Hi there.

Is there a special reason why we delete expired transients only on get_transient attempt (https://github.com/WordPress/WordPress/blob/master/wp-includes/option.php#L662), but not in cron.php?

I believe https://github.com/WordPress/WordPress/blob/master/wp-admin/includes/upgrade.php#L1748-L1759 belongs in cron.php. Otherwise we're having leftover transients until it's attempted to retrieve them.

I will assume two cases from here:

  • It was done due to performance reasons. In which case there could've been a button in the wp-admin somewhere to flush the expired transients, or allow the users to enable/disable cron deletion through a config param in wp-config.php.
  • It was a bad design decision. In which case we can copy the excerpt from upgrade.php to cron.php somewhere.

In any case, I believe the fix is easy and doesn't have a big impact, while at the same time it allows us to keep the DB from being polluted.

Attachments (4)

41699.diff (725 bytes) - added by Mte90 3 months ago.
41699.2.diff (9.6 KB) - added by dd32 8 weeks ago.
41699.3.diff (12.3 KB) - added by dd32 5 weeks ago.
41699.4.diff (1.6 KB) - added by dlh 4 weeks ago.

Download all attachments as: .zip

Change History (22)

#1 @bor0
3 months ago

  • Keywords dev-feedback added

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


3 months ago

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


3 months ago

#4 @SergeyBiryukov
3 months ago

  • Milestone changed from Awaiting Review to 4.9

@Mte90
3 months ago

#5 @Mte90
3 months ago

Looking at the code there is already a loop of all the hooks with unschedule (https://github.com/WordPress/WordPress/blob/master/wp-cron.php#L115).
But unschedule is used to remove in the system the scheduled hooks and not cleaning.

Looking at the code in upgrade.php the query is running for a multisite environment but if we are running wp-cron.php we have to act on the cron stuff of the site that is running.

So looking on the code of a famous wordpress plugin for clean the transient (https://plugins.trac.wordpress.org/browser/delete-expired-transients/trunk/includes/class.DelxtransCleaners.php) I made that patch that run the query before the die(); of wp-cron.

#6 @bor0
8 weeks ago

  • Keywords has-patch added; dev-feedback removed

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


8 weeks ago

@dd32
8 weeks ago

#8 @dd32
8 weeks ago

The correct code to copy for this is the section from schema.php: https://github.com/WordPress/WordPress/blob/master/wp-admin/includes/schema.php#L587-L607

For some background on when the cleanup code was added, check out #20316

At the time, it wasn't uncommon for people to be abusing transients, but it was rare for it to be causing issue.
Clearing on DB upgrade was acceptable as it didn't need to happen *that* often, once every few months was plenty.

Unfortunately since adding that code, more plugins have abused it in even more wonderful ways, passing even more unique keys through to transients.. causing this need for it to occur more often.

One of the suggestions from #20316 was for it to be part of wp_scheduled_delete() which cleans up the Trash'd posts/comments.
41699.2.diff is a mostly untested first scratch at such a patch putting it into it's own function. The patch queues the job up in admin.php which doesn't seem like the best location, but does mean that it'll only be added for existing sites that are used.

#9 @Mte90
8 weeks ago

Probably is better a cron that daily clean up the unflushed transients and avoid that the admin do this kind of operation.

#10 @westonruter
5 weeks ago

  • Owner set to dd32
  • Status changed from new to assigned

Who is owning this to fix for 4.9? I'll assign to @dd32 since he uploaded the last patch.

Closely related to #38899 (Deletion of auto-drafts and trashed posts never gets scheduled unless user accesses admin pages).

@dd32
5 weeks ago

#11 @dd32
5 weeks ago

Here's another partially tested patch. 41699.3.diff Not completely happy with it, and can't really test it, so I'm going to leave it here for a bit longer in the hope someone else weighs in.

#12 @westonruter
5 weeks ago

@dd32 what specifically aren't you happy with?

#13 @dd32
4 weeks ago

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

In 41963:

Transients: Clear expired transients from the database in a daily cron task.

Fixes #41699

@dlh
4 weeks ago

#14 @dlh
4 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I apologize if it was intentional, but [41963] left out the new wp_schedule_event() for 'delete_expired_transients'. 41699.4.diff would add it back and fixes a couple of formatting glitches with the delete_expired_transients() function.

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


4 weeks ago

#16 @johnbillion
4 weeks ago

  • Status changed from reopened to reviewing

#17 @SergeyBiryukov
4 weeks ago

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

In 42008:

Transients: After [41963], add missing cron task for delete_expired_transients().

Props dlh.
Fixes #41699.

#18 @dd32
4 weeks ago

Thanks @SergeyBiryukov! (Sorry folks, was offline & missed the file in the checkin)

Note: See TracTickets for help on using tickets.