-
Notifications
You must be signed in to change notification settings - Fork 53
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
Fix/classify post action #311
Conversation
Post type necessary to make "handle_bulk_actions-edit-$post_type" work consistently.
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.
@mustafauysal thank you for this update. Please fix the code regarding the post_type
argument or explain the necessity of it.
$actions['classify'] = sprintf( | ||
'<a href="%s">%s</a>', | ||
esc_url( wp_nonce_url( admin_url( 'edit.php?action=classify&ids=' . $post->ID ), 'bulk-posts' ) ), | ||
esc_url( wp_nonce_url( admin_url( sprintf( 'edit.php?action=classify&ids=%d&post_type=%s', $post->ID, $post->post_type ) ), 'bulk-posts' ) ), |
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.
Looks redundant, the hook handle_bulk_actions-edit-$post_type
works regardless of post_type
query argument presence.
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.
@cadic It's not redundant but it can be a bit tricky to realize how it hooked on a single classification request at first glance.
In order to see reproduce the issue, please enable classify for "page" post type only, and click on the "Classify" row action for any given page. You will see nothing happens without passing the correct post_type
.
wp-admin/edit.php
needs to know $post_type
to execute the dynamic hook ("handle_bulk_actions-edit-$post_type"
) accordingly.
I hope this makes sense.
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.
@mustafauysal thank you, I can confirm the action link doesn't work without post_type
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.
@mustafauysal thank you, looks good!
$actions['classify'] = sprintf( | ||
'<a href="%s">%s</a>', | ||
esc_url( wp_nonce_url( admin_url( 'edit.php?action=classify&ids=' . $post->ID ), 'bulk-posts' ) ), | ||
esc_url( wp_nonce_url( admin_url( sprintf( 'edit.php?action=classify&ids=%d&post_type=%s', $post->ID, $post->post_type ) ), 'bulk-posts' ) ), |
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.
@mustafauysal thank you, I can confirm the action link doesn't work without post_type
Description of the Change
These changes address 2 issues with CPT.
post_type
is not supported. (post_row_actions
,page_row_actions
could allow to shown classify action on unregistered post type)post_type
for individual classify action. (eg: when you enable classify for the "page" and click on the row item, it doesn't work due to missingpost_type
on request.Alternate Designs
Benefits
Making ClassifAI work better in admin UI.
Possible Drawbacks
Haven't seen any drawback, changes are relatively small.
Verification Process
foobar
only/wp-admin/edit.php?post_type=foobar
for a postChecklist:
I couldn't add tests at this time, but it might be worth adding some testing 🤷🏻♂️
Changelog Entry