-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Make the filter links links in the posts list table filterable #9950
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
base: trunk
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -261,23 +261,41 @@ protected function get_edit_link( $args, $link_text, $css_class = '' ) { | |||||
| $class_html = ''; | ||||||
| $aria_current = ''; | ||||||
|
|
||||||
| if ( ! empty( $css_class ) ) { | ||||||
| $edit_filter_link_vars = compact( 'url', 'link_text', 'css_class' ); | ||||||
|
|
||||||
| /** | ||||||
| * Filters the links created for filtering the posts list table. | ||||||
| * | ||||||
| * @since 6.9.0 | ||||||
| * | ||||||
| * @param array $edit_filter_link_vars { | ||||||
| * The edit filter link variables. | ||||||
| * | ||||||
| * @type string $url The formatted link string. | ||||||
| * @type string $link_text The link text. | ||||||
| * @type string $css_class The class HTML. | ||||||
| * } | ||||||
| * @param array $args URL parameters for the link. | ||||||
| */ | ||||||
| $edit_filter_link_vars = apply_filters( 'edit_filter_links', $edit_filter_link_vars, $args ); | ||||||
|
|
||||||
| if ( ! empty( $edit_filter_link_vars['css_class'] ) ) { | ||||||
| $class_html = sprintf( | ||||||
| ' class="%s"', | ||||||
| esc_attr( $css_class ) | ||||||
| esc_attr( $edit_filter_link_vars['css_class'] ) | ||||||
| ); | ||||||
|
|
||||||
| if ( 'current' === $css_class ) { | ||||||
| if ( 'current' === $edit_filter_link_vars['css_class'] ) { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if other CSS classes had been added? This seems like an ideal place to use
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. with all of the attribute activity going on here we could also benefit at large from using the HTML API. $processor = new WP_HTML_Tag_Processor( '<a>LINK_TEXT</a>' );
$processor->next_token();
$processor->set_attribute( 'href', $edit_filter_link_vars['url'] );
$processor->set_attribute( 'class', $edit_filter_link_vars['css_class'] ?? null );
$processor->set_attribute( 'aria-current', 'page' );
$processor->next_token();
$processor->set_modifiable_text( $edit_filter_link_vars['link_text'] );
return $processor->get_updated_html();
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dmsnell what about #9950 (comment) ?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. well I don’t know. to be honest I didn’t fully understand the new filter, but I think the HTML API can play a nice role in constructing the HTML wherever it’s created, even if that’s before filtering. |
||||||
| $aria_current = ' aria-current="page"'; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| return sprintf( | ||||||
| '<a href="%s"%s%s>%s</a>', | ||||||
| esc_url( $url ), | ||||||
| esc_url( $edit_filter_link_vars['url'] ), | ||||||
| $class_html, | ||||||
| $aria_current, | ||||||
| $link_text | ||||||
| $edit_filter_link_vars['link_text'] | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
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.
Is this the right filter name? Shouldn't it be singular?
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.
Or rather, this filter still is difficult to understand.
Since it is specifically for filtering the link used in a column of the post list table, maybe:
post_list_column_edit_linkThere 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.
But it also seems strange that the link itself is not being filtered. It's the array that goes into constructing the link. That being the case, then it should be something like:
post_list_column_edit_link_vars.But that feels wrong too. Why not just let the HTML be constructed as it had been before, and then add a filter for the resulting HTML? This feels better to me:
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.
Then the HTML Tag Processor can be used to manipulate the edit link as desired.
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 will note that one problem I see over and over again with this kind of filtering is that we really don’t have a solution for proper escaping. by passing HTML through a filter we have mushed together actual HTML and escaped text nodes. this makes it hard to properly filter, because people would need to use the HTML Processor if they want to filter it after that. they should not use string functions.
I’ve been working on some ideas for improving this situation but they aren’t going to be ready for 6.9.0 and they aren’t going to fully address this.
at least when we are talking about the constituent parts we can agree that the values are “raw PHP strings.” once we mush things we lose the ability to track which parts are which.
this is a long-winded answer to a simple question, and it’s not in any way unique to this change, but it’s the reason I don’t feel like I have a simple response.
I’m reluctant to pass the HTML Processor or Tag Processor as the input to filters, but this is a kind of example where we could be exploring newer semantic filters.
maybe we could export the proposed attributes and children of the
Aelement and let people much with them.The original proposal seems simpler and raises fewer questions than this version of the patch.