-
Notifications
You must be signed in to change notification settings - Fork 365
Add methods to hide filled and expired posts #2449
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Adds two new methods to hide filled and expired posts - Will investigate to see if it's possible to do the same, but more performantly and without modifying `meta_query`
- Use get_posts to retrieve a list of Job IDs that are either expired, or marked as filled - Store IDs in a transient for caching/performance - Transient for the moment is set to cache for a day - Clear transients on post update if the post status changes, or post meta is updated - Transients are then regenerated when searching if the relevant "hide" option is enabled
- Updates the early return so that return properly if neither "hide_*" option is enabled, but if one is, allow the method to continue.
Add checks for: - is_main_query - is_archive - 'job_listing' is the post type To ensure that we don't impact any non-main queries, queries that don't pertain to 'job_listing', and that might also happen on archive pages.
- && 'job_listing' === $query->get( 'post_type' )
- Remove now unnecessary method to retrieve expired posts by ID - Exclude expired posts by setting the query to 'publish' post status only, if $hide_expired is enabled - Removed related expired transient code - Set the get_filled_job_listings() method to retrieve all posts, rather than 5
- When using get_posts() to retrieved posts with a _filled meta value of 1, make sure we're only querying posts with a 'publish' post status.
- Removes $query->set() to hide $expired posts - Instead, changes the 'public' argue on register_post_status( 'expired' ) to 'true' or 'false' depending on whether we're on a search page, or the 'job_manager_hide_expired' option is enabled (or not in either case) - Also changes the filled jobs transient delete so that it triggers on 'update_post_meta', vs. 'save_post'
| [ | ||
| 'label' => _x( 'Expired', 'post status', 'wp-job-manager' ), | ||
| 'public' => true, | ||
| 'public' => $is_expired_public, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jom Here's the refactored implementation for setting 'public on the post status registration. The logic is slightly different than discussed but noted in the comment. In testing:
- Job listings in WP Admin show and are counted as expected
- Expired Jobs are hidden from search results (or not, if the option is disabled)
- Expired Jobs, when viewed logged out, show the 'Expired' notice instead of a 404
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable name threw me off a bit, but I think the ultimate logic is correct!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up reverting this due to issues checking conditionals like is_archive(). Committed 1a7b7da
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main issue we were avoiding with the other strategy was $query->set( 'post_status', 'publish' ) removing other custom public statuses from the search. I actually wonder if this was fixed in core (5.9) with https://core.trac.wordpress.org/changeset/49830. It should now be respecting our exclude_from_search when registering the expired post status. Did you find in testing that expired posts were showing up in search results still?
I wonder if we should be setting exclude_from_search based on our setting. Since that fix in core, I'm guessing we're hiding expired results from search even when we shouldn't be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, gotcha.
exclude_from_search is already set to true:
WP-Job-Manager/includes/class-wp-job-manager-post-types.php
Lines 395 to 407 in 0568f04
| register_post_status( | |
| 'expired', | |
| [ | |
| 'label' => _x( 'Expired', 'post status', 'wp-job-manager' ), | |
| 'public' => true, | |
| 'protected' => true, | |
| 'exclude_from_search' => true, | |
| 'show_in_admin_all_list' => true, | |
| 'show_in_admin_status_list' => true, | |
| // translators: Placeholder %s is the number of expired posts of this type. | |
| 'label_count' => _n_noop( 'Expired <span class="count">(%s)</span>', 'Expired <span class="count">(%s)</span>', 'wp-job-manager' ), | |
| ] | |
| ); |
This does not exclude expired jobs from WP Search.
This seems to be getting overwritten by the exclude_from_search parameter on the post-type registration:
| 'exclude_from_search' => false, |
If this is set to true, then posts with an expired post status are, as expected, excluded from search -- but so is any job type post.
When I was testing the 'public' => manipulation, it worked for $GET_['s'] but did not work for archive pages as helper methods were always returning false. Maybe there's a way around that, that I haven't thought of?
I'm wondering if another option would be to keep the $query->set(), but instead of setting it to publish, retrieve all post statuses with get_post_stati(), remove what we don't want, and then pass everything else. Though, that seems a bit... hacky?
$post_statuses = get_post_stati();
$excluded_statuses = [ 'future', 'draft', 'pending', 'trash', 'auto-draft', 'preview', 'private', 'request-pending', 'request-confirmed', 'request-failed', 'request-completed', 'expired' ];
$valid_statuses = array_diff( $post_statuses, $excluded_statuses );
$query->set( 'post_status', $valid_statuses );
IIUC, this is more to do with the issue report here https://core.trac.wordpress.org/ticket/44737, which is sitting stale. Making the suggested fix does fix the behaviour, but then that's a whole core-related thing.
cc @yscik
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be getting overwritten by the exclude_from_search parameter on the post-type registration:
I saw https://core.trac.wordpress.org/ticket/44737, but then saw the lines get changed in https://core.trac.wordpress.org/ticket/48556 and thought that would fix it, but now I can see what it is doing in the } elseif ( ! $this->is_singular ) { block.
I'm wondering if another option would be to keep the $query->set(), but instead of setting it to publish, retrieve all post statuses with get_post_stati(), remove what we don't want, and then pass everything else. Though, that seems a bit... hacky?
This is what I was going to suggest before I went down the 44737 rabbit hole because it seems like WP is doing this in a way that would work for us:
https://github.com/WordPress/WordPress/blob/32e94b4de108fb090ebc678b48113b8954f7f188/wp-includes/class-wp-query.php#L2587-L2593
I think if we did this we could just removed expired (I'm guessing)? It is a bit hack-y, but might be okay.
The only other thought I had is modifying the global that holds the post status objects here after our conditionals and then restore public => true in the posts_selection action, but that might feel more hacky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…to quiet PHPCS. The change to `update_post_meta` broke tests, so revert that for now.
jom
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do some more testing later, but this is looking good! I added a few minor comments.
| /** | ||
| * This code checks if (1) we're on a search or (2) the option to hide expired job listings is enabled. | ||
| * If either condition is false, we're going to set the register_post_status() 'public' arg to false, | ||
| * otherwise it will be true. | ||
| * | ||
| * This will prevent single expired job listings from showing a '404', instead of an | ||
| * 'Expired' notice, when viewing the single job listing and not logged in. | ||
| * | ||
| * This will also address historical issues stemming from addressing the issue raised here | ||
| * https://github.com/Automattic/WP-Job-Manager/issues/1884. | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this comment. Really helped with figuring out the logic.
| [ | ||
| 'label' => _x( 'Expired', 'post status', 'wp-job-manager' ), | ||
| 'public' => true, | ||
| 'public' => $is_expired_public, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable name threw me off a bit, but I think the ultimate logic is correct!
wp-job-manager-functions.php
Outdated
|
|
||
| /** | ||
| * Delete filled_jobs_transient and expired_jobs_transient transients when a job listing is saved or updated. | ||
| * | ||
| * @param int $post_id The ID of the post being saved. | ||
| * @param WP_Post $post The post object being saved. | ||
| */ | ||
| function delete_filled_job_listing_transient_on_post_meta_update( int $post_id, WP_Post $post ): void { | ||
|
|
||
| if ( 'job_listing' === $post->post_type ) { | ||
|
|
||
| if ( '1' === get_post_meta( $post_id, '_filled', true ) ) { | ||
| delete_transient( 'hide_filled_jobs_transient' ); | ||
| } | ||
| } | ||
|
|
||
| } | ||
| add_action( 'save_post', 'delete_filled_job_listing_transient_on_post_meta_update', 10, 2 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious about why this is separated from the other logic in this PR? Do we think this function is useful in the public API?
There is also the [possibly edge] case where a job went from filled to not filled. Do you think that is worth flushing this transient after each job listing safe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jom Ah, I hadn't realised the functions file was dedicated to the API. I'll move this to post-types.php.
One thing I'm having difficulty with is getting the method to trigger on updated_post_meta. What I'd want to do ideally, following Peter's comment above, is:
- Set the transient
- If a job has the
_filledmeta updated, either add that ID to the transient or remove it
Clearing it on every post save is far too much and makes the transient pointless imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always get a little nervous about manipulating cached data (besides flushing it) because there are so many weird scenarios that could affect it. However, I suppose the weird ones would just happen for the life of the transient. In this case, one possible strangeness could happen in post status changes because to populate the transient you're querying published posts. While the query is inefficient generally, I think it isn't so bad for it to run it again when a possible change has been detected.
What issues were you experiencing with updated_post_meta? It be possible to do a slightly lightweight flush on that and transition_post_status.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jom So I've learned that I was doing a check like:
if ( '_filled' !== $meta_key || 'job_listing' !== get_post_type( $object_id ) ) {
return;
}
But in debugging, I noticed that $meta_key is only ever returning _edit_lock, so what actually works is:
if ( '_edit_lock' !== $meta_key || 'job_listing' !== get_post_type( $object_id ) ) {
return;
}
But I don't imagine checking against _edit_lock is all that useful. I'm stumped on how to check that the _filled meta was changed. I've committed what I have now at de746c4
- Renamed to $is_expired_hidden to (hopefully) make the true/false more readable.
…the transient will return an empty array if there are no post IDs anyway.
Setting 'public' to 'false' dynamically workes fine with $_GET['s'], however, the same method fails to return true when checking is_archive(), is_tax(), is_wpjm_*(), etc. This previous implementation is able to check conditionals as expected, keeps the code in one place, and works.
Refactors the maybe_hide_expired_posts() method to set the 'public' static to 'false', then back to 'true', during/after query generation.
| add_filter( 'wp_insert_post_data', [ $this, 'fix_post_name' ], 10, 2 ); | ||
| add_action( 'add_post_meta', [ $this, 'maybe_add_geolocation_data' ], 10, 3 ); | ||
| add_action( 'update_post_meta', [ $this, 'update_post_meta' ], 10, 4 ); | ||
| add_action( 'updated_post_meta', [ $this, 'delete_filled_job_listing_transient' ], 10, 4 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should be update_post_meta and not updated ? I couldn't find the updated_post_meta hook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a sneaky dynamic one: http://developer.wordpress.org/reference/hooks/updated_(meta_type)_meta/
| ! is_admin() | ||
| && $query->is_main_query() | ||
| && ( $query->is_search() || $query->is_archive() ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to add a comment here clarifying the reason for this logic: We want the filtering to apply only to search and archive public facing queries. We don't want it to apply in the admin panel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Committed 27b3be7
| ! is_admin() | ||
| && $query->is_main_query() | ||
| && ( $query->is_search() || $query->is_archive() ) | ||
| ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here about the comment explaining the logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Committed 27b3be7
| public function make_expired_public(): void { | ||
|
|
||
| global $wp_post_statuses; | ||
|
|
||
| if ( isset( $wp_post_statuses['expired'] ) ) { | ||
| $wp_post_statuses['expired']->public = true; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn´t find any information about this global. Can you make a brief summary on what is it and what is this supposed to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jom and I discussed this global in a call. AFAIK it's first mentioned here in post.php:
function register_post_status( $post_status, $args = array() ) {
global $wp_post_statuses;
if ( ! is_array( $wp_post_statuses ) ) {
$wp_post_statuses = array();
}
It is an array of all post statuses. What we're doing here is checking that the expired post status is registered, and if it is, we either set its public argument to true or false via the maybe_hide_expired_job_listings() method.
From there, if $hide_expired is true, $this->make_expired_private(); sets the public argument to false during query generation, after which it sets it back to true with add_action:
WP-Job-Manager/includes/class-wp-job-manager-post-types.php
Lines 732 to 741 in 82e75e6
| if ( | |
| ! is_admin() | |
| && $query->is_main_query() | |
| && ( $query->is_search() || $query->is_archive() ) | |
| ) { | |
| $this->make_expired_private(); | |
| add_action( 'posts_selection', [ $this, 'make_expired_public' ] ); | |
| } |
|
Closing as ~2 years stale. At this point a new PR would be more appropriate for taking this forward. |
Fixes #1884
Changes proposed in this Pull Request
get_filled_job_listings(), (2),get_expired_job_listingsand (3)maybe_hide_filled_expired_job_listings_from_searchpost__not_inparameter is applied to$query->setwhen not in WP Admin, and when on a WP search results page.Unlike the previous implementation, this one relies on
get_posts()andmeta_querybut without the expensiveORrelation.Because
expiredjobs already have their post status updated toexpiredif a post's expiry date is set in the past or elapses, we don't need tometa_queryfor expired posts but can instead look forjob_listingswith anexpiredpost status.We don't have the same luxury with
filledposts, but we can do a singularmeta_querywithout any relations to retrieve these.My understanding is that these changes should be far less expensive than the previous implementation.
To further help with performance, the results of
get_filled_job_listingsandget_expired_job_listingsare stored in a transient that is expired every 24 hours, which lines up with the various job expiration methods.Either transient is then cleared on post update if: (1) we're looking at a job listing, (2) the '_filled' post meta has been updated, or (3) the post status has changed from
publishtoexpired,trashor vice-versa.Testing instructions
Expiredwith an expiry date in the past, orFilledby checking theFilledcheckbox.hide_filled_jobs_transientandhide_expired_jobs_transientis not set. With WP CLI these can be checked withwp transient get hide_filled_jobs_transientandwp transient get hide_expired_jobs_transientexpiredorfilledIf you would like to test on a large dataset, WP CLI can be used to bulk create posts:
Bulk create
ExpiredpostsBulk generate
FilledpostsFilledposts are a little more troublesome because we're working with meta values, andwp post generate, as well aswp post create, do not touch these values.So, you could do something like:
The above can be added to a file on your environment, like
create-filled-posts.sh. Make sure tochmod +x $fileand then./$file.shto execute theforloop and create your posts.Note that when creating posts programmatically you may need to manually clear the transients.