WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 3 years ago

#18356 closed defect (bug) (fixed)

get_bookmarks() indefinitely caches randomly ordered results

Reported by: prettyboymp Owned by: boonebgorges
Milestone: 4.6 Priority: normal
Severity: normal Version:
Component: Query Keywords: good-first-bug has-patch
Focuses: Cc:

Description

The get_bookmarks() function caches the results of all requests to the function in a single cache result so it can clear all the items when a new book mark is added. However, randomly ordered results shouldn't be cached indefinitely. This specifically affects the Links widget as it never refreshes when set to random order.

Attachments (6)

18356.diff (1.8 KB) - added by prettyboymp 8 years ago.
18356.2.diff (1.6 KB) - added by prettyboymp 8 years ago.
switching to separate cache with shorter cache timeout
18356.refresh.patch (1.3 KB) - added by c3mdigital 6 years ago.
refresh of 2nd patch
18356.3.diff (1.7 KB) - added by MikeHansenMe 5 years ago.
Refreshed and minor code cleanup
18356.4.diff (1.4 KB) - added by boonebgorges 4 years ago.
18356.5.diff (944 bytes) - added by lukecavanagh 3 years ago.
Skip cache by order if random

Download all attachments as: .zip

Change History (28)

#1 @kawauso
8 years ago

  • Keywords needs-patch added

@prettyboymp
8 years ago

#2 @prettyboymp
8 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

#3 follow-up: @SergeyBiryukov
8 years ago

$orderby == 'rand()'

Shouldn't it be $orderby == 'rand' (without brackets)?

#4 follow-up: @westi
8 years ago

I'm not sure why you are re-implementing the expiration functionality that is already built into the caching system here.

If we need to use a different cache bucket and add expiration for RAND requests lets do that but don't implement a second level of expiration when we have one already.

#5 in reply to: ↑ 3 @prettyboymp
8 years ago

Replying to SergeyBiryukov:

$orderby == 'rand()'

Shouldn't it be $orderby == 'rand' (without brackets)?

It's 'rand()' at that point because the $orderby variable is reused when building the query in the case statement.

#6 in reply to: ↑ 4 ; follow-up: @prettyboymp
8 years ago

Replying to westi:

I'm not sure why you are re-implementing the expiration functionality that is already built into the caching system here.

If we need to use a different cache bucket and add expiration for RAND requests lets do that but don't implement a second level of expiration when we have one already.

The problem with creating a new cache bucket is that you can't purge it automatically after modifying the bookmarks.

#7 in reply to: ↑ 6 @westi
8 years ago

Replying to prettyboymp:

Replying to westi:

I'm not sure why you are re-implementing the expiration functionality that is already built into the caching system here.

If we need to use a different cache bucket and add expiration for RAND requests lets do that but don't implement a second level of expiration when we have one already.

The problem with creating a new cache bucket is that you can't purge it automatically after modifying the bookmarks.

Yes, but if it has a shortish expiration which is what you are suggesting here to deal with the RAND issue then that doesn't really matter.

@prettyboymp
8 years ago

switching to separate cache with shorter cache timeout

#8 @c3mdigital
6 years ago

  • Keywords has-patch needs-testing removed
  • Resolution set to wontfix
  • Status changed from new to closed

Not sure if many people are still using bookmarks and blogrolls.

#9 follow-up: @markoheijnen
6 years ago

Not sure if that is a reason to not fix something that is broken.

#10 @SergeyBiryukov
6 years ago

  • Keywords has-patch added
  • Resolution wontfix deleted
  • Status changed from closed to reopened

#11 @c3mdigital
6 years ago

  • Keywords needs-refresh added

#12 in reply to: ↑ 9 @c3mdigital
6 years ago

Replying to markoheijnen:

Not sure if that is a reason to not fix something that is broken.

Agreed.

@c3mdigital
6 years ago

refresh of 2nd patch

@MikeHansenMe
5 years ago

Refreshed and minor code cleanup

#13 @MikeHansenMe
5 years ago

  • Keywords needs-refresh removed

#14 @DrewAPicture
4 years ago

  • Component changed from General to Query
  • Keywords needs-refresh added

This doesn't strictly belong in the Query component, but I'd be interested to hear feedback from @wonderboymusic or @boonebgorges on this. Too many bookmarks/links-related tickets get stuck in the General component and never come out.

Couple notes on the patch as-is:

  • Needs a refresh
  • The reiteration of the filter will need a duplicate hook comment
  • The expiration should use MINUTE_IN_SECONDS instead of 60

@boonebgorges
4 years ago

#15 @boonebgorges
4 years ago

  • Keywords 2nd-opinion added; needs-refresh removed

A separate cache bucket for randomly ordered results seems fine to me. It makes for a funny structure to the cache - the non-random results are stored in a multi-d array in a single cache slot, while random results get their own top-level cache slot - but I guess that's not such a big deal, given that the cache strategies are different in the two cases.

I don't see a good reason to expose 'expire' as a function parameter. In 18356.4.diff, I moved it to a filtered value.

This seems like a good fix IMO, but would like a second opinion before moving forward.

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


3 years ago

#17 @boonebgorges
3 years ago

  • Keywords needs-patch good-first-bug added; has-patch 2nd-opinion removed
  • Milestone changed from Awaiting Review to Future Release

After discussion, it seems like a more straightforward fix may be to skip the cache for random results. This is more consistent with other rand() treatments throughout WP.

#18 @lukecavanagh
3 years ago

I am going to work on a patch. Should have something up by tomorrow.

@lukecavanagh
3 years ago

Skip cache by order if random

#19 @lukecavanagh
3 years ago

  • Keywords has-patch added; needs-patch removed

@boonebgorges can you review this patch and let me know any changes are needed.

#20 @boonebgorges
3 years ago

  • Milestone changed from Future Release to 4.6
  • Owner set to boonebgorges
  • Status changed from reopened to accepted

Thanks, @lukecavanagh. I think this looks good. I'm going to venture where few others would dare go, and add the ability to write tests for bookmark/link functions.

#21 @boonebgorges
3 years ago

In 37563:

Add tests for get_bookmarks() cache.

This changeset adds a unit test factory so that bookmark/link fixtures can be
created during tests.

Why are we writing tests for functionality that has been deprecated for years?
Because it's the Right Thing to Do.

See #18356.

#22 @boonebgorges
3 years ago

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

In 37565:

In get_bookmarks(), don't cache if 'orderby=rand'.

Props lukecavanagh, prettyboymp, c3mdigital, MikeHansenMe.
Fixes #18356.

Note: See TracTickets for help on using tickets.