get_bookmarks() indefinitely caches randomly ordered results

[prettyboymp]

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.

[SergeyBiryukov]

$orderby == 'rand()'

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

[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.

[prettyboymp]

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.

[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.

[westi]

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]

switching to separate cache with shorter cache timeout

[c3mdigital]

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

[markoheijnen]

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

[c3mdigital]

Replying to markoheijnen:

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

Agreed.

[c3mdigital]

refresh of 2nd patch

[MikeHansenMe]

Refreshed and minor code cleanup

[DrewAPicture]

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]

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.

[boonebgorges]

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.

[lukecavanagh]

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

[lukecavanagh]

Skip cache by order if random

[lukecavanagh]

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

[boonebgorges]

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.

[boonebgorges]

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.

[boonebgorges]

In 37565:

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

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