Adding a basic cache-flushing mechanism

[nicholasio]

Introduces a basic cache flushing mechanism. #4.

[stevegrunwell]

I dig the general approach of caching the rather-static site details, but it looks like this PR is implementing some very specific logic to a framework that's meant to be rather agnostic by default.

What are your thoughts on additional filtering within the Thing::build() method to enable additional, potentially-cached content before any other queries are run? This could give site-specific code the ability to change the values returned from the cache, potentially solving the same problem in a more isolated way.

As always, thank you for your pull request and please accept my apologies that I missed the notification email!

[stevegrunwell]

Rather than redefining these values in ThingTest.php, they should be added to the test bootstrap file for easy re-use.

[stevegrunwell]

Is there a reason this isn't leveraging the get_key() function defined just above it?

[stevegrunwell]

If $object isn't being used, we don't really need it as an argument.

[stevegrunwell]

This could be further simplified to leverage the update_post_cache() function you wrote above:

array_map( __NAMESPACE__ . '\update_post_cache', $author_posts );

This eliminates the foreach loop and allows us to leverage functionality we've already defined.

[stevegrunwell]

I've recently learned this lesson the hard way, but you want to use the 'times' argument sparingly when testing — if you're expecting a result to match the return value of a function call, it can be assumed that the function was called. Counting the number of times it was called is usually not particularly helpful, unless it's more of a "I need to make sure this function was called X number of times, because it has other side effects."

For example, consider the following functions:

/**
 * Create a heading.
 *
 * @param string $text The text for the new heading.
 * @return string A new <h1> element.
 */
function make_heading() {
  track_headings( $text );

  return '<h1>' . shout( $text ) . '</h1>';
}

/**
 * Convert a string to uppercase and append exclamation marks.
 *
 * @param string $string The string to modify.
 * @return string The string but, you know, louder.
 */
function shout( $string ) {
  return strtoupper( $string ) . '!!!';
}

/**
 * Keep track of headings being used on the page.
 *
 * By the time I wrote this, the example was going off the rails a bit. Sorry 'bout that.
 *
 * @global $all_headings
 *
 * @param string $heading The heading text.
 */
function track_headings( $heading ) {
  global $all_headings;

  $all_headings[] = $heading;
}

When testing make_heading(), we don't necessarily care that shout() was called, as long as the result is what we expect:

public function test_make_heading() {
  $this->assertEquals(
    '<h1>THIS IS A HEADING!!!</h1>',
    make_heading( 'this is a heading' )
  );
}

Since track_headings() doesn't have an effect on the output, however, we may want to ensure that gets called:

public function test_make_heading_calls_track_headings() {
  M::passthruFunction( __NAMESPACE__ . '\track_headings', array(
    'times' => 1,
    'args'  => array( 'this is a heading' ),
  ) );

  make_heading( 'this is a heading' );
}

I'll be the first to admit that I've been overzealous about using WP_Mock for :allthethings: in the past, and the result has been tightly-coupled tests that break if any detail of the implementation changes. Hopefully I can serve as a cautionary tale! 😄

[nicholasio]

To be honest, I'm not a huge fan of WP_Mock, it's boring to mock all those basic functions and I always have the feeling that the tests are biased and it's not really testing anything, The only reason I added times was because I was seeing it being used across tests. I'm totally fine removing it. if you think it makes sense :)

For these tests, if the function call doesn't happen the test will fail anyway, so times is not that important here.

[stevegrunwell]

If the date of the cache time is stored in the {$cache_key}_last_update cache, why duplicate it here?

[nicholasio]

The cache expires after 12h. So the post might not chang but cache can still expire. For debug purposes that date was meant to be when the cache was recreated and not when the post was last updated

[nicholasio]

@stevegrunwell I pushed some changes to address your feedback! Thanks for you time looking into this.

By adding filtering on Thing::build() you mean moving the cache purging logic to cache.php by hooking into those filters/actions? If so that makes sense to me, I only kept the logic there because there was already some caching logic there. Let me know what you think.

[nicholasio]

Merging the latest changes here, I'll take another pass and recreate the tests.

@stevegrunwell are you still open to merge this PR?

[stevegrunwell]

@nicholasio Definitely still open, my time on Schemify has been really scattered. Hoping to be able to get back to this towards the end of this week :)