Improve `media_library_months_with_files` inside `wp_enqueue_media()`

[apermo]

As described in the filter, the query can and will be expensive for large media libraries.
The downside of the given implementation is, that it requires a fully custom implementation. If you want to just store the result of the query in a transient there is currently no way to achieve this.
Ideal would be a get_media_library_months_with_files() that would have been added to media_library_months_with_files at it's introduction.

<?php
function wp_enqueue_media( $args = array() ) {
        //...

        /**
         * Allows overriding the list of months displayed in the media library.
         *
         * By default (if this filter does not return an array), a query will be
         * run to determine the months that have media items.  This query can be
         * expensive for large media libraries, so it may be desirable for sites to
         * override this behavior.
         *
         * @since 4.7.4
         *
         * @link https://core.trac.wordpress.org/ticket/31071
         *
         * @param stdClass[]|null $months An array of objects with `month` and `year`
         *                                properties, or `null` for default behavior.
         */
        $months = apply_filters( 'media_library_months_with_files', null );
        if ( ! is_array( $months ) ) {
                $months = $wpdb->get_results(
                        $wpdb->prepare(
                                "SELECT DISTINCT YEAR( post_date ) AS year, MONTH( post_date ) AS month
                                FROM $wpdb->posts
                                WHERE post_type = %s
                                ORDER BY post_date DESC",
                                'attachment'
                        )
                );
        }

        //...
}

Since that can't be done now without the uncertainty if it could break things depending on the priority used by third parties.

I see 2 reasonable approaches to improve this.

1. Add a do_action( 'post_media_library_months_with_files', $months ); just after the query. This could be used to fill a transient that could be read in the filter media_library_months_with_files, the downside would be that splitting the get and set of the transient, would be counterintuitive.
2. Move the query to a distinct funtion get_media_library_months_with_files() which could be used inside media_library_months_with_files to fill a transient. This approach seems the more favorable to me, and I will prepare this as a PR.

While it is unlikely that the query will ever change, I still think it should not be required to copy core code, just to create a transient.

This introduces get_media_library_months_with_files() just as described in 63279, this allows an easy and consistent way to create transients in media_library_months_with_files without the need to copy and paste WordPress core code.

Trac ticket: https://core.trac.wordpress.org/ticket/63279#ticket

[spacedmonkey]

@apermo Could we consider adding caching at the same time as creating this as new function?

[apermo]

I wanted to keep it simple, but I would love it.

Caching via transient with a filterable TTL?

What default TTL would you suggest?

[spacedmonkey]

WordPress doesn't do refactors for the sake of refactors. So there to be a benefit the change, other than code tidiness.

Creating a function would be contain the media_library_months_with_files and add caching would add the benefit.

I would use the wp_cache_get / wp_cache_set apis, not transients. I would then delete the cache key, in clean_post_cache like this.

  if ( 'attachment' === $post->post_type ) {
     wp_cache_delete( 'cache_key', 'post-queries' );
  }

[apermo]

I know that WordPress does not refactor for the sake of refactoring.

As I tried to point out, this is already a benefit.

Current status quo:

You can use media_library_months_with_files to short circuit the query, but you will either need to come up with some function (for example a loop that just guesses the results) or copy code from core.

With my suggested change:

The dev can use the new get_media_library_months_with_files() to use the same query the core uses.

---

I have that very use case, and I did both, and I don't like that I had to copy code from core, and while the loop from a start date to the current month works fine for one project, the editors post content in the far future before scheduling it correctly, so that project has uploads in the year 2222.

I do see your point that the caching API looks beneficial, but in this case I disagree that this would be the correct approach.

Without the use of an object cache, wp_cache_set() will not be persistent, I can for sure say that this query is fired at maximum once per page load, so without object cache doing this will have no advantage. And for me, who want's to do a more persistent cache, it's also no advantage.

I do agree though with your suggestion to clear/repopulate the cached result when a new file was updated.

For small to medium bloggers, this change would change nothing, and they will not install an object cache, so they wouldn't see a difference if we added wp_cache_set()

For maintainers of large installations (such as myself) having get_media_library_months_with_files() would be a trivial way to add some more permanent cache to media_library_months_with_files without the need to copy and paste core code.

Don't see it as refactoring wp_enqueue_media(), see it as introducing get_media_library_months_with_files() which is then used by core, to not repeat oneself.

As you pointed out, WordPress does not refactor just to refactor, but there is the other mantra, that core should only come with features that are for the bigger majority, so adding some caching that does nothing for the majority, and will likely not cut it for high end blogs, is also contradicting that mantra.

After rethinking your suggestion, I think I'd still store the result for my project in a transient, maybe with multiple days life time, and redo the transient everytime media has been uploaded.

Using a transient though would benefit every user, with or without object cache. And with object cache the transient is stored in the object cache instead of the database. So from my point I'd still either keep it like it is, or add the transient with a clear and maybe fill after upload and a filter for the lifetime of the transient.

I'm be happy to hear your thought and maybe a third opinion.