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.

[apermo]

@spacedmonkey I opted to do it with a transient. The transient will increase performance for everyone. If a user has an object cache, it will automatically be stored there.

Let me know if you can follow that approach, if so, I'll continue and provide tests for clearing the transient and that the new function works as intended.

[dmsnell]

There’s a clear need articulated in this ticket and I think it’s warranted; while it’s possible to intercept the 'query' hook and rewrite the attachment query, that encourages hard-coding internals in the plugin space. Having a reusable base query provides the behavior without encouraging those awkward couplings.

That being said I am not of the same opinion that we should combine this change with the introduction of a new caching layer. When all we are doing is extracting the internal process for reuse, there are no architectural impacts of this change. When we add a default caching layer there are brand new issues to consider, potentially-surprising performance impacts, and cache-invalidation problems.

I would be far more inclined to merge this without any transients or caches, leaving the state as it is and up to plugins, and wait and see if there’s even a justification for default caching here. As I understand it, this patch is not only about the performance characteristics of the database query itself, but of the entire system reliant on the results, as well as the UX impact of showing everything inside a massive library.

[apermo]

Copied from ​https://github.com/WordPress/wordpress-develop/pull/8688#discussion_r2849835015

While preparing an answer for @westonruter I found this:

​https://github.com/search?q=repo%3AWordPress%2Fwordpress-develop%20%22SELECT%20DISTINCT%20YEAR(%20post_date%20)%20AS%20year%2C%20MONTH(%20post_date%20)%20AS%20month%22&type=code

The query is basically called 4 times throughout core, with minor changes, and in /wp-admin/include/media.php even without a prepare.

Two related two attachments and basically copy and paste. The other two slight variants but close.

I'm thinking of at least using the newly introduced function in both cases. What are your thoughts? Make a reuseable function for all 4, or just the 2 attachments queries?

[westonruter]

Replying to apermo:

Two related two attachments and basically copy and paste. The other two slight variants but close.

I'm thinking of at least using the newly introduced function in both cases. What are your thoughts? Make a reuseable function for all 4, or just the 2 attachments queries?

I think reusing the same query in all cases would make the most sense if it had caching built into it. This would mean moving the transient call inside of the function. In that case, it would only make sense the two attachment instances.

​https://github.com/WordPress/wordpress-develop/pull/8688#discussion_r2850309680

After your ( @dmsnell and @westonruter ) comments I rolled back to a mix of the original draft and last state, adding the filter in the second file, and using the new function in both places. And updated the unit test.

I also rolled back the type cast to int. As pointed out there is the filter and we cannot ensure that the filter would provide the right type. The code using the result will always need to accept string and int. So the type cast would add no really value.
With this approach, we ensure that the behavior is unchanged for any instance without custom code (or a plugin). But both places will use the new function and the filter, plus the function is unit-tested now.

This approach fixes inconsistency in core, will add a unit test, and will improve extensibility without any possible side effects.

The previous approach is still in the commit history up to ad1cec4228254bd6c3bc0a53bcc09787b1226b50