Web-stories-wp: filter_poster_attachments is interferring with many unrelated queries

[WPprodigy]

This pre_get_posts filtering is a bit too broad: https://github.com/google/web-stories-wp/blob/main/includes/Media.php#L216

If the only purpose is truly "Reduces unnecessary noise in the media library", then need to be sure that is the only condition it is being activated on. Because right now, this meta query can be tied to all sorts of various other queries that can leave a largely negative performance impact on a site.

[swissspidy]

Hi there, thanks for raising this.

Do you have an example of such an unrelated query this is running against? The goal was to only filter attachment queries, so any other ones should be skipped.

[WPprodigy]

The ones I saw mostly causing issues were from unrelated 'post_type' => 'any' queries. Simple example:

$query = new WP_Query(
  'per_page' => 100,
  'post_type' => 'any',
  'tax_query' => [
    [
      'taxonomy' => 'custom_tax',
      'field'    => 'ID',
      'terms'    => [1,2,3],
    ]
  ],
);

Resulting in roughly:

SELECT SQL_CALC_FOUND_ROWS  wp_posts.ID FROM wp_posts  LEFT JOIN wp_term_relationships ON (wp_posts.ID = wp_term_relationships.object_id) LEFT JOIN wp_postmeta ON (wp_posts.ID = wp_postmeta.post_id AND wp_postmeta.meta_key = XXX ) WHERE XXX=XXX  AND ( 
  wp_term_relationships.term_taxonomy_id IN (XXX)
) AND ( 
  wp_postmeta.post_id IS NULL
) AND wp_posts.post_type IN (XXX, XXX, XXX, XXX, XXX, XXX, XXX, XXX, XXX) AND (wp_posts.post_status = XXX) GROUP BY wp_posts.ID ORDER BY wp_posts.post_date DESC LIMIT XXX, XXX

[WPprodigy]

The goal was to only filter attachment queries, so any other ones should be skipped.

That goal feels too wide-reaching IMO. Can't really dictate what a person is trying to do with their custom queries, maybe they need to loop over all attachment posts for something.

Seems like the goal should just be to target the WP admin attachments screen, to prevent the aforementioned clutter?

[swissspidy]

Seems like the goal should just be to target the WP admin attachments screen, to prevent the aforementioned clutter?

Yeah that might work. Plus perhaps the REST API as well.

[spacedmonkey]

Instead of using custom post meta, we could use a custom taxonomy to hide the attachments. This would be better for performance. However, it would require a migrate of data. We already have been talking about generating different image size via a background task, maybe migrate this data at the same time.

[luizcruz]

Hi,

This happened with such brutal intensity that it took off the website that we keep at Automattic down. We remove the plugin from operation until we have a solution. Following here, gentlemen.

[swissspidy]

Instead of using custom post meta, we could use a custom taxonomy to hide the attachments. This would be better for performance. However, it would require a migrate of data. We already have been talking about generating different image size via a background task, maybe migrate this data at the same time.

Yeah it would be a good thing for sure, but doesn't address the issue of interfering with unrelated queries. Two separate tasks I think.

[spacedmonkey]

Another more simple solution, is to remove this logic all together. This would mean the posters image would be shown in the media library, which isnt the worst thing.

[swissspidy]

Another more simple solution, is to remove this logic all together. This would mean the posters image would be shown in the media library, which isn't the worst thing.

I tend to agree. This will happen anyway once you deactivate the Web Stories plugin.

I think there is one blocker for this though, which is basically #3228. Due to poor naming, it is not clear to the user that the images is an autogenerated poster for the video.

Plus, the video is not set as the poster's parent:

Compare this to a poster image manually uploaded to a video:

So perhaps we could do solve this in multiple steps:

1. Remove any check from the query filter
2. Fix poster naming
3. Fix poster parent association
4. Remove query filter altogether

Thoughts?

[spacedmonkey]

1. Fix poster parent association

As I recall, attachments do not support parent attachment relationships out of the box. As in, parent value should be a post / page and not another attachment. Should the parent value be set to the story's id? What value does the parent value add?

[swissspidy]

No, they actually do. As I tested in my previous comment, when you manually add a poster to a video in the WP media library it will set the video as the poster‘s parent.

My thought about the parent value was that it gives the user information about where the poster is used, so they don‘t accidentally delete the poster. But perhaps I‘m just overly cautious.

[spacedmonkey]

Okay, so should we create 4 tickets or 1 for this issue?

[swissspidy]

Tickets around this (new and old):

• [x] Poster generation: Set video as the poster's parent (#5544)
• [x] Use custom taxonomy instead of post meta for marking video posters (#5545)
• [x] Improve generated video poster file name #3228
• [x] Consider dropping filter_poster_attachments for anything not in the editor #5546

[spacedmonkey]

There are now breakout issues for #5545 solves #5635 and #5546 solves #5636. I have also created #5637 to implement the taxonomy queries in a couple of places. I have created another follow ticket to remove post meta completely #5638

[swissspidy]

All of these tickets have now been resolved 🎉

We're now using taxonomy queries, and only apply them within our own context.

This is gonna be in our next release, so @WPprodigy keep an eye out for that :)

Let us know should there be any more issues.