Make WordPress Core

Opened 14 years ago

Last modified 17 months ago

#15386 reopened defect (bug)

WP_List_Table::get_columns does not work for plugins

Reported by: ptahdunbar's profile ptahdunbar Owned by: scribu's profile scribu
Milestone: Future Release Priority: high
Severity: major Version: 3.1
Component: Administration Keywords: has-patch has-unit-tests dev-feedback close
Focuses: Cc:

Description

Creating a new table based on the WP_List_Table class does not work as the get_columns method is not being called.

The problem is when WP_List_Table::get_column_info() calls get_column_headers() instead of $this->get_columns() where all the column data is stored.

Moving the filter manage_*_columns from get_column_headers() into WP_List_Table::get_column_info() along with passing $this->get_columns() fixes this issue.

Attachments (8)

ticket.15386.get_columns.diff (634 bytes) - added by ptahdunbar 14 years ago.
my-list-table-class.php (1.4 KB) - added by scribu 14 years ago.
Example list table class
my-list-table-plugin.php (984 bytes) - added by scribu 14 years ago.
Example plugin that uses list table class
screen.diff (739 bytes) - added by manutdotcom 13 years ago.
remove array() arg from apply_filters
15386.diff (2.9 KB) - added by johnwatkins0 4 years ago.
my-list-table-plugin-15386.php (407 bytes) - added by bgoewert 20 months ago.
Comment #19 list-table plugin
my-list-table-class.2.php (1.4 KB) - added by bgoewert 19 months ago.
Updated exmaple of table list class
my-list-table-plugin.2.php (840 bytes) - added by bgoewert 19 months ago.
Updated example plugin for table list class

Download all attachments as: .zip

Change History (37)

#1 @scribu
14 years ago

  • Owner set to scribu
  • Status changed from new to accepted

#2 @scribu
14 years ago

  • Milestone changed from Awaiting Review to 3.1

#3 @scribu
14 years ago

  • Keywords close added

The constructor in WP_List_Table adds the get_columns() method as filter with priority 0.

This is for backwards compatibility with older plugins that use the filter directly to register columns for new screens.

#4 @scribu
14 years ago

The likely problem is that get_column_headers() is called before My_Plugin_List_Table is instantiated.

#5 @scribu
14 years ago

  • Keywords needs-patch added; has-patch close removed

my-list-table-plugin.php creates an admin screen and uses an example list table class semi-successfully.

The ajax doesn't work however.

#6 @scribu
14 years ago

  • Severity changed from normal to major

#7 @scribu
14 years ago

  • Priority changed from normal to high

@scribu
14 years ago

Example list table class

@scribu
14 years ago

Example plugin that uses list table class

#8 @scribu
14 years ago

  • Milestone 3.1 deleted
  • Resolution set to worksforme
  • Status changed from accepted to closed

I've only needed to hook into 'get_list_table_*'.

my-list-table-plugin.php now succesfully registers a custom list table class.

@manutdotcom
13 years ago

remove array() arg from apply_filters

#9 @manutdotcom
13 years ago

  • Cc manutdotcom added

When the array() argument is removed from line 24 (see screen.diff attachment), the table does tabulate properly without the need of initializing the WP_List_Table class before the page is loaded using add_action( "load-$screen_id", array( $this, '_init_list_table' ) ) as recommended in my-list-table-plugin.php above. The WP_List_table class can be initialized inside the function that was declared for add_menu_page().

Then I would be able to dynamically render different tables according to the GET variable. Useful for tabbed pages.

I was wondering since get_columns() do not require an input argument, maybe we could remove array() from line 24?

#10 @scribu
13 years ago

In the meantime, WP_List_Table has become internal i.e. is not recommended to be used directly by plugins at this time (WP 3.3).

With this in mind, feel free to open a new ticket.

#11 @scribu
13 years ago

But before that, see #17615 and #18449

#12 @manutdotcom
13 years ago

Thanks, in the meantime, I'll just manually define $this->_column_headers instead of it being automatically populated.

#13 follow-up: @markoheijnen
13 years ago

  • Cc marko@… added

What does of means that it is private. This class is a brilliant asset a developer can use since you want the same look in your plugin as WordPress itself.

Is the code most likely to change? Some information here can be handy for developers who want to use it.

#14 in reply to: ↑ 13 @dd32
13 years ago

Replying to markoheijnen:

What does of means that it is private. This class is a brilliant asset a developer can use since you want the same look in your plugin as WordPress itself.

The private status in this case means exactly what Private access means in applications "This should only be used by the core application at this point in time". It'll be rehashed elsewhere, but put basically: Don't use this while it's Private and expect core to retain compatibility in future releases, It'll be changed and will most likely break whatever you're currently doing with it in a million piece.

It's going to be a useful functionality to Plugin developers, We're just not quite at the point where we can offer it up to developers.

#15 @pcfreak30
8 years ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

This still appears to be an issue. I tracked it down in admin.php -> admin-header.php -> $current_screen->render_screen_meta() -> show_screen_options() -> get_column_headers.

get_column_headers gets called in the header before the table can even be created. Due to get_column_headers using a static, it can not be unset and causes empty result.

Only workaround I found was hooking admin_init, running set_current_screen(), creating the table in a private class property then accessing it on the plugins menu hook to render.

#16 @netweb
8 years ago

  • Milestone set to Awaiting Review

Moving reopened tickets without a milestone back to Awaiting Review for review

#17 @davidbaumwald
4 years ago

This still appears to be an issue. $columns are not being defined when extending WP_List_Table in a plugin.

This ticket was mentioned in PR #165 on WordPress/wordpress-develop by johnwatkins0.


4 years ago
#18

@johnwatkins0
4 years ago

#19 @johnwatkins0
4 years ago

  • Keywords has-patch added; needs-patch removed

Attaching https://core.trac.wordpress.org/attachment/ticket/15386/15386.diff. Also via GitHub: https://patch-diff.githubusercontent.com/raw/WordPress/wordpress-develop/pull/165.diff

Description of the change

This allows the filter in the get_column_headers function to be applied multiple times during page execution. Previously this function used a static variable to ensure the filter was only applied once per screen object per page load.

That static variable is not needed. The function doesn't need to maintain internal state, and I don't see any reason that the performance-related use cases for static function variables apply here more than elsewhere. I'm happy to discuss this further.

Test steps

Before pulling these changes, put this in a file in wp-content/plugins.

<?php
/**
 * Plugin Name: My List Table Plugin
 */

add_action( 'admin_menu', 'my_list_table_plugin_init' );

function my_list_table_plugin_init() {
	add_menu_page( 'Test', 'Test', 'manage_options', 'test', 'my_list_table_plugin_display' );
}

function my_list_table_plugin_display() {
	$table = _get_list_table( 'WP_Comments_List_Table' );
	$table->views();
	$table->prepare_items();
	$table->display();
}

Also make sure your environment has at least one comment.

Before

Activate the plugin. Visit /wp-admin/admin.php?page=test in your local environment. Note that the table displays without the expected table rows.

After

After pulling these changes, visit that page again. The table rows should now display.

Further considerations

If this change were merged, wouldn't it mean that at least the WP_List_Table class and maybe also _get_list_table would no longer need to be private? If so, how would I deprecate _get_list_table in favor of get_list_table? Do we have documentation on that somewhere?

I feel the API around these list tables could be improved, however, and maybe even Gutenbergized. If others agree, then maybe that class and function should stay private for now.

Last edited 4 years ago by johnwatkins0 (previous) (diff)

This ticket was mentioned in Slack in #core by chaion07. View the logs.


3 years ago

#21 @chaion07
3 years ago

Thanks @ptahdunbar for reporting this issue. We reviewed the ticket during a recent Core Triage session. Based on the feedback received from the team, we feel the need for a refresh for the patch and a owner for the ticket. However, we would leave as it is for now and hopefully someone possibly will own the ticket for a better and realistic approach to this. Cheers!

This ticket was mentioned in PR #3735 on WordPress/wordpress-develop by @bgoewert.


20 months ago
#22

  • Keywords has-unit-tests added

@bgoewert
20 months ago

Comment #19 list-table plugin

#23 @bgoewert
20 months ago

I created a refreshed PR. While this fixed the headers/rows not displaying as described in comment 19, it also produced one phpunit failure seen here.

Test Report

Patch tested: PR 3735

Environment

  • OS: Pop!_OS 22.04
  • Localhost: wordpress-develop & Docker Desktop
  • WordPress: trunk
  • Browser: Firefox 107, Chrome 108, GNOME Web 43 (WebKit 2.38.1)
  • Theme: Twenty Twenty-Two
  • Active Plugins: my-list-table-plugin-15386.php

Steps to Reproduce

  1. Download and install my-list-table-plugin-15386.php.
  2. Ensure there is at lease one comment.
  3. Navigate to /wp-admin/options-general.php?page=test
  4. Notice that the table displays without any headers or rows.
  5. Apply or Checkout PR 3735.
  6. Refresh the test page /wp-admin/options-general.php?page=test

Expected Results

  • Table list displays comment headers and rows.

Actual Results

  • ✅ I can see headers/rows.

Supplemental Artifacts

Before applying patch

https://i.imgur.com/JTGBym1.png

After applying patch

https://i.imgur.com/m8WvJnH.png

Last edited 20 months ago by bgoewert (previous) (diff)

This ticket was mentioned in Slack in #core by bgoewert. View the logs.


19 months ago

#25 @SergeyBiryukov
19 months ago

  • Component changed from General to Administration
  • Milestone changed from Awaiting Review to 6.2

#26 @SergeyBiryukov
19 months ago

For reference, the $column_headers static variable that is removed here was added in [18943].

I was wondering, it looks like there was a working example at some point in the two files attached above comment:8, does it no longer work?

@bgoewert
19 months ago

Updated exmaple of table list class

@bgoewert
19 months ago

Updated example plugin for table list class

#27 @bgoewert
19 months ago

  • Keywords dev-feedback close added

The example in comment:8 no longer works as it stands. I've attached an updated version of this example.

According to the class reference, developer usage of WP_List_Table is still in limbo (even though it is widely used). And there is plenty of documentation on it's usage.

WP_List_Table was taken out of private status in 6.1 [54378] #18449

I vote we close.

Version 0, edited 19 months ago by bgoewert (next)

This ticket was mentioned in Slack in #core by costdev. View the logs.


17 months ago

#29 @johnbillion
17 months ago

  • Milestone changed from 6.2 to Future Release

Can somebody clarify what the exact problem reported in this ticket is? What code reproduces the problem and what is the problem?

Note: See TracTickets for help on using tickets.