WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#34508 closed defect (bug) (fixed)

list_table_primary_column doesn't work following some manage_$screen_columns

Reported by: soulseekah Owned by: wonderboymusic
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch
Focuses: administration Cc:

Description

The list_table_primary_column filter short-circuits when using the manage_users_columns filter to add/remove columns instead of extending the WP_List_Table class.

For example, let's add a column to WP_Users_List_Table and try setting it as primary:

add_filter( 'manage_users_columns', function( $columns ) {
    $columns['company'] = 'Company';
    return $columns;
} );

So far so good. Now let's set the 'company' column as primary.

add_filter( 'list_table_primary_column', function( $column, $screen ) {
    if ( $screen != 'users' ) return;
    return 'company';
}, null, 2 );

Expect it to work and yet it doesn't. Why?

Let's take a look at the get_primary_column_name function in https://github.com/WordPress/WordPress/blob/master/wp-admin/includes/class-wp-list-table.php#L939

See the short circuit?

if ( empty( $column ) || ! isset( $columns[ $column ] ) ) {
    $column = $default;
}

Well turns out that the $columns are retrieved via get_columns which is unfiltered in some subclasses. Namely WP_Users_List_Table, which returns:

public function get_columns() {
    $c = array(
        'cb'       => '<input type="checkbox" />',
        'username' => __( 'Username' ),
        'name'     => __( 'Name' ),
        'email'    => __( 'Email' ),
        'role'     => __( 'Role' ),
        'posts'    => __( 'Posts' )
    );
    if ( $this->is_site_users )
        unset( $c['posts'] );
    return $c;
}

Without reusing the filter there we're simply screwed. Note that this doesn't happen for all builtin subclasses. For example WP_MS_Users_List_Table does it right: https://github.com/WordPress/WordPress/blob/master/wp-admin/includes/class-wp-ms-users-list-table.php#L160

So let's add the missing filters to get_columns, shall we?

Attachments (1)

34508.patch (589 bytes) - added by SergeyBiryukov 6 years ago.

Download all attachments as: .zip

Change History (4)

#1 @soulseekah
6 years ago

Affected:

  • wp-admin/includes/class-wp-plugin-install-list-table.php
  • wp-admin/includes/class-wp-users-list-table.php
  • wp-admin/includes/class-wp-comments-list-table.php
  • wp-admin/includes/class-wp-plugins-list-table.php
  • wp-admin/includes/class-wp-themes-list-table.php
  • wp-admin/includes/class-wp-terms-list-table.php
  • wp-admin/includes/class-wp-ms-themes-list-table.php
  • wp-admin/includes/class-wp-ms-themes-list-table.php
  • wp-admin/includes/class-wp-links-list-table.php

As far as I can tell.

#2 @SergeyBiryukov
6 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.4

As a simple fix, WP_List_Table::get_primary_column_name() could use get_column_headers(), which does apply the filter. We already use it in WP_List_Table::get_column_info().

With 34508.patch, your snippets work for me.

Last edited 6 years ago by SergeyBiryukov (previous) (diff)

#3 @wonderboymusic
6 years ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from new to closed

In 35565:

List Tables: to ensure that get_primary_column_name() can match columns that were added via the "manage_{$screen->id}_columns" filer, call get_column_headers() instead of $this->get_columns().

List Table classes and WP_Screen are already tangled together. The parent list table constructor adds a filter that is called by a function that references the instance globally, even though we have access to it via composition directly in the class that is adding the filter. So the fact that functions here have to be called that reference a screen instance we already have access to is what one might call... less than elegant.

#OOP

Props SergeyBiryukov.
Fixes #34508.

Note: See TracTickets for help on using tickets.