Skip to content

Duplicate logic from WP Core to filter pages before preloading them a…#16144

Merged
hansjovis merged 5 commits intotrunkfrom
QAK-2234-fix-preloading-all-pages-on-pages-overview
Nov 4, 2020
Merged

Duplicate logic from WP Core to filter pages before preloading them a…#16144
hansjovis merged 5 commits intotrunkfrom
QAK-2234-fix-preloading-all-pages-on-pages-overview

Conversation

@herregroen
Copy link
Copy Markdown
Contributor

@herregroen herregroen commented Oct 2, 2020 •
edited
Loading

…nd their indexables

Context

  • WP_Query when on the page overview loads every single page ( but only their ID and parent_id ). This caused our preloading logic to load all data and indexables for every single page.

Summary

This PR can be summarized in the following changelog entry:

  • Fixes an issue where memory could be exhaused on the page overview for users with large amounts of pages.

Relevant technical choices:

  • Two functions from WP Core had to be duplicated with slight adaptations in order to ensure we're filtering the list of all page IDs down to the list of currently shown page IDs in the exact same manner.
  • The CS warning threshold has been increased by 3 as the 3 warnings, class name too many parts and @inheritDoc comments not having short descriptions should not be warnings.

Test instructions

This PR can be tested by following these steps:

  • Make sure you have more pages than fit on a single overview in your admin.
  • Make sure this includes child pages that are shown on the first page of your pages overview.
  • Open the first page of your pages overview.
  • Open query monitor.
  • There should be a single query grabbing all indexables for pages on your current overview.
  • No additional indexables should be fetched.
  • There should be no additional queries for other page indexables.

UI changes

  • This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Documentation

  • I have written documentation for this change.

Quality assurance

  • I have tested this code to the best of my abilities
  • I have added unittests to verify the code works as intended

Fixes https://yoast.atlassian.net/browse/QAK-2234
Fixes #16027

@Djennez
Copy link
Copy Markdown
Member

Djennez commented Oct 2, 2020

Tested, confirmed to limit the amount of IDs that are cached. ✅

I have tested this with core WordPress pages, as well as a custom hierarchical posttype. I switched around the amount of posts per page and confirmed that that the IDs of the posts that are shown on the admin page are the same as the IDs that are fed into the caching function. This may still hurt performance a bit if the limit is set to 100 and all 100 posts need to be indexed, but that is only once, hopefully.

@herregroen one question about a theoretical situation; if there is a plugin that causes the main query to gather a lot of post IDs and there are not hierarchical, we could theoretically run into the same issue. Should we therefore introduce a hard limit on the amount of posts that can be cached by this function? We know WordPress core does not allow more than 100 posts per page, so maybe always only feed the first 100 post IDs to the cache?

@herregroen
Copy link
Copy Markdown
Contributor Author

herregroen commented Oct 2, 2020

@Djennez If that plugin does so in the same way that the page overview does then we'll work together seamlessly as it's not based on the post type being hierarchical but rather the data-structure.

If that plugin is fetching full post objects then our caching won't make a difference as the main issue is the post_content being fetched and we only fetch that ourselves if the data structure is as used by hierarchical post types.

andizer
andizer approved these changes Nov 4, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@andizer andizer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CR done 👍

@hansjovis hansjovis self-assigned this Nov 4, 2020
@hansjovis
Copy link
Copy Markdown
Contributor

hansjovis commented Nov 4, 2020

Acceptance test: ✅

@hansjovis hansjovis added the changelog: bugfix Needs to be included in the 'Bugfixes' category in the changelog label Nov 4, 2020
@hansjovis hansjovis added this to the 15.4 milestone Nov 4, 2020
@hansjovis hansjovis merged commit 108edc0 into trunk Nov 4, 2020
@hansjovis hansjovis deleted the QAK-2234-fix-preloading-all-pages-on-pages-overview branch November 4, 2020 09:53
@Djennez Djennez mentioned this pull request Mar 25, 2021
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@andizer andizer andizer approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@hansjovis hansjovis

Labels

changelog: bugfix Needs to be included in the 'Bugfixes' category in the changelog

Projects

None yet

Milestone

15.4

Development

Successfully merging this pull request may close these issues.

14.9 Breaks Site (WP DB Memory Error) if WP Author has Too Many Posts/Pages

4 participants

@herregroen @Djennez @hansjovis @andizer