Follow-up to: #80855: Add element #type table and merge tableselect/tabledrag into it

Goal

  • Consistent usage of #type 'table' instead of custom theme functions, etc.

Remaining tasks

Module Theme function name Issue
aggregator theme_aggregator_categorize_items #1938894: Convert aggregator theme tables to table #type
block theme_block_admin_display_form #1938898: Convert block theme tables to table #type Assigned to: brantwynn
book theme_book_admin_table #1968982: Convert book theme tables to table #type
field_ui theme_field_ui_table #1938900: Convert theme_field_ui_table into a template
file theme_file_widget_multiple #1938902: Convert file theme tables to table #type
filter theme_filter_admin_format_filter_order #1938904: Convert filter theme tables to table #type
filter theme_filter_admin_overview #1938904: Convert filter theme tables to table #type
forum theme_forum_list #1938906: Convert forum theme tables to table #type
forum theme_forum_topic_list #1938906: Convert forum theme tables to table #type
image theme_image_style_effects #1938910: Convert image theme tables to table #type
language theme_language_admin_overview_form_table #1938912: Convert language content setting table theme to a twig template
language theme_language_negotiation_configure_browser_form_table #2422481: Convert language negotiation theme table to table #type
language theme_language_negotiation_configure_form #1898422: language.module - Convert theme_ functions to Twig
locale theme_locale_translate_edit_form_strings #1938916: Convert locale theme tables to table #type
menu theme_menu_overview_form #1938918: Convert menu theme tables to table #type Assigned to: botanic_spark
node theme_node_search_admin #1938920: Convert node_search_admin theme tables to table #type
shortcut theme_shortcut_set_customize #1938924: Convert shortcut theme tables to table #type
simpletest theme_simpletest_test_table #1938926: Convert simpletest theme tables to table #type Assigned to: sun
system theme_system_date_format_localize_form no longer in core 25/9/15
system theme_system_modules_uninstall #2151113: Convert theme_system_modules_uninstall() to Twig
system theme_system_modules_details #2151109: Convert theme_system_modules_details() to Twig
system theme_status_report #2013258: Simplify render for theme_status_report()
taxonomy theme_taxonomy_overview_terms #1938932: Remove taxonomy_overview_terms() and taxonomy_overview_vocabularies() theme functions in favour of table #type
taxonomy theme_taxonomy_overview_vocabularies #1938932: Remove taxonomy_overview_terms() and taxonomy_overview_vocabularies() theme functions in favour of table #type
update theme_update_version #1938934: Convert update theme tables to table #type
user theme_user_admin_permissions #1938938: Convert theme_user_admin_permissions() to table #type Assigned to: duellj
user theme_user_admin_roles #1938938: Convert theme_user_admin_permissions() to table #type Assigned to: duellj

Blocks

Related isssues

Spin-offs

#1877338: Convert language admin form to new #type 'table'

Files: 
CommentFileSizeAuthor
#17 1876712-17-table-conversion.patch55.1 KBduellj
FAILED: [[SimpleTest]]: [MySQL] 52,553 pass(es), 46 fail(s), and 29 exception(s). View
#10 drupalcore-convert_all_tables_in_core-1876712-10.patch5.72 KBnagwani
PASSED: [[SimpleTest]]: [MySQL] 50,477 pass(es). View

Comments

sun’s picture

Issue tags: +Novice

Technically, this should be a straightforward task — the change notice explains the conversion. So let's see whether a novice contributor is able to master this :-)

kim.pepper’s picture

I'm not sure how many different form tables require an update (I could only find one), so I created a specific issue for it here #1877338: Convert language admin form to new #type 'table'

kim.pepper’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

The first showcase conversion is happening in #1877338: Convert language admin form to new #type 'table'

Before converting too many, we might want to resolve #1876718: Allow modules to inject columns into tables more easily first, not sure.

Fabianx’s picture

Issue tags: +Twig

Tagging

jwilson3’s picture

For anyone looking for a quick list of theme functions that generate tables.... see rows 79 - 101 and rows 140 - 151 of this slightly old (oct 2012) list/analysis of core theme functions. HTH

kim.pepper’s picture

I grabbed the relevant rows out of that spreadsheet for easy access.

Module Theme function name Where in Code What is it really?
aggregator theme_aggregator_categorize_items function form table
block theme_block_admin_display_form template table
book theme_book_admin_table function (unusable) form table draggable
field_ui theme_field_ui_table function (unusable) table
file theme_file_formatter_table function table
file theme_file_widget_multiple function form table draggable
filter theme_filter_admin_format_filter_order function form table draggable
filter theme_filter_admin_overview function form table draggable
forum theme_forum_list template table
forum theme_forum_topic_list template table
image theme_image_anchor function (unusable) table (custom widget)
image theme_image_style_effects function (unusable) form table draggable
image theme_image_style_list function (unusable) table w/ operations
language theme_language_admin_overview_form_table function form table draggable
language theme_language_negotiation_configure_browser_form_table function form table
language theme_language_negotiation_configure_form function form table draggable
locale theme_locale_translate_edit_form_strings function form table w/ pager
menu theme_menu_overview_form function form table draggable
node theme_node_recent_block function table w/ more link
node theme_node_search_admin function form table
poll theme_poll_choices function form table draggable
shortcut theme_shortcut_set_customize function form table draggable
simpletest theme_simpletest_test_table function table
system theme_status_report function table
system theme_system_date_format_localize_form function form table
system theme_system_date_time_settings function form table
system theme_system_modules_fieldset function form table
system theme_system_modules_uninstall function form table
system theme_table function table
taxonomy theme_taxonomy_overview_terms function (unusable) form table w/ operations, pager
taxonomy theme_taxonomy_overview_vocabularies function (unusable) form table w/ weight, operations
update theme_update_version function (unusable) table
user theme_user_admin_permissions function form table
user theme_user_admin_roles function form table draggable

---

Update: added table type

jwilson3’s picture

It would have been helpful to see the other column which indicates if its a table, or a form table, or a form table draggable... ;) but anyway.

kim.pepper’s picture

@jwilson3 sorry! Updated table.

sun’s picture

Most of these are going to be eliminated by the config entity conversions that we're doing currently.

Also, #1876718: Allow modules to inject columns into tables more easily needs major attention, since it will probably change the API of #type 'table'.

nagwani’s picture

Status: Active » Needs review
FileSize
5.72 KB
PASSED: [[SimpleTest]]: [MySQL] 50,477 pass(es). View

Not sure if this is still valid. If yes, need a quick review

Status: Needs review » Needs work

The last submitted patch, drupalcore-convert_all_tables_in_core-1876712-10.patch, failed testing.

andypost’s picture

andypost’s picture

Status: Needs work » Needs review

@nagwani Yes this makes a lot of sense! This is a tremendous cleanup for theme functions.
Some of the tables would be replaces by entity list controllers some probably by views but anyway this great!

nagwani’s picture

So do we take it module wise or function wise. IMO, it would make sense to convert all required functions in module and push it in, before doing another module. Any suggestions?

duellj’s picture

duellj’s picture

Assigned: Unassigned » duellj
Status: Needs review » Needs work

I'm hoping to tackle this during the sprint this weekend. Should we split this out module-wise into sperate issues?

duellj’s picture

Status: Needs work » Needs review
FileSize
55.1 KB
FAILED: [[SimpleTest]]: [MySQL] 52,553 pass(es), 46 fail(s), and 29 exception(s). View

Here's the first round of table conversions, I'll be working on the rest tomorrow. This patch converts the following theme functions to table types:

- language_negotiation_configure_form
- language_negotiation_configure_form
- language_content_settings_table
- system_modules_uninstall
- system_date_format_localize_form
- taxonomy_overview_vocabularies
- taxonomy_overview_terms
- user_admin_roles

There's some issues converting some of the tables that relay on cell attributes (like the permissions table, where the module name column needs a class and a colspan), and it doesn't appear that table form types don't allow columns to have attributes. Maybe a separate issue needs to be opened?

I doubt tests will pass, since this is only my first pass on these conversions. Let's see what happens anyway :).

Status: Needs review » Needs work

The last submitted patch, 1876712-17-table-conversion.patch, failed testing.

duellj’s picture

Title: Convert all tables in core to new #type 'table' » [meta] Convert all tables in core to new #type 'table'

Given the size of this patch I'm going to open up individual issues for each module. Issue summary update coming soon.

duellj’s picture

Issue summary: View changes

Updated issue summary.

duellj’s picture

Issue summary: View changes

Splits tasks into subissues

duellj’s picture

Issue summary: View changes

Cleans up table

aczietlow’s picture

I can help with some of the conversions. Let me know what you've done and haven't done yet.

duellj’s picture

Thanks aczietlow! If you take a look at the remaining tasks table in the issue summary, you could definitely jump in on any issue that's active or needs work.

joelpittet’s picture

Title: [meta] Convert all tables in core to new #type 'table' » [meta] Convert all form tables in core to new #type 'table'

Changing the title to hopefully unconfuse, some tables need to be #type=>table as it's a wrapper with form related features for #theme=>table. Most tables that use #tabledrag or #tableselect should use #type=>table, and most of the rest should stay with #theme=>table.

Correct me if I am mistaken.

duellj’s picture

@joelpittet: any time a table contains form elements, it has to be #type => table, even if it's not a tabledrag or tableselect. Thanks for the title change. The remaining tasks should be cleaned up to remove non-form table conversions.

joelpittet’s picture

Since #type=>table uses #theme=>table in the back and then adds some properties and hooks for preparing it for #theme=>table

@see line 583 of system.module
http://drupalcode.org/project/drupal.git/blob/HEAD:/core/modules/system/...

    '#value_callback' => 'form_type_table_value',
    '#process' => array('form_process_table'),
    '#element_validate' => array('form_validate_table'),
    ...
    '#pre_render' => array('drupal_pre_render_table'),

We can build out some of the tables that need classes on cells using the #theme=>table structures

I am not sure how this will affect the form API and it may not work with #tableselect or #tabledrag but for the others it may...

tstoeckler’s picture

Title: [meta] Convert all form tables in core to new #type 'table' » [meta] Convert all tables in core to new #type 'table'

I don't actually see why we wouldn't convert *all* tables in core, even just for consistency. For non-form tables the changes should be pretty trivial anyway.

joelpittet’s picture

@tstoeckler I would say because it adds a bunch of hooks for form handling and properties for tabledrag/tableselect specifically for forms that a normal table doesn't need. Also, the way you build a table for #type=>table vs #theme=>table is similar but slightly different for the #rows for how the elements are nested.

joelpittet’s picture

Issue summary: View changes

Remove poll module

joelpittet’s picture

Title: [meta] Convert all tables in core to new #type 'table' » [meta] Convert form tables in core to new #type 'table'

@tstoeckler further to my point above: because #type=>table is defined in form.inc and #theme=>tableis defined in theme.inc, the separation seems to make sense. If they were merged both into form.inc so people didn't use #theme=>table but instead used #type=>table I feel it would be a bit strange for people looking to "theme" a table, where all the other theme functions live currently in theme.inc. If we did it the other way around and add the hooks on a normal #theme we will end up with the mess we have now were form tables were built in a myriad of different ways. The main purpose of #type=>table is to consolidate and clean up code that used #tabledrag and #tableselect, to eventually remove those files and functions.

@see the block and new type info at the top of this issue.

joelpittet’s picture

tstoeckler’s picture

Well you actually made my point :-):

Also, the way you build a table for #type=>table vs #theme=>table is similar but slightly different for the #rows for how the elements are nested.

I think we should avoid such inconsistencies.

The fact that #type => table is defined in form.inc is unfortunate but simply a cause of our highly-coupled form/render/theme systems. I don't think that's a strong argument against this.

joelpittet’s picture

@tstoeckler, I'm still not seeing your point, maybe we are saying the same thing?

What would you propose happen to #theme=>table if we replace everything with #type=>table?

tstoeckler’s picture

Well #type => table still calls #theme => table by itself, so nothing would change. It's only a change to what we want to enforce as the proposed usage and what we use throughout core.

tstoeckler’s picture

Issue summary: View changes

Update issue with patches attributed to all tables that they represent.

duellj’s picture

Issue summary: View changes

Removing theme_file_formatter_table() since not a form element

Cottser’s picture

I updated the issue summary to remove theme_node_recent_block() per #23 because it's not a form table.

Cottser’s picture

Assigned: duellj » Unassigned
Status: Needs work » Active

Status should just be 'active' now that this is a meta issue, unassigning as well.

Cottser’s picture

Issue summary: View changes

Removing theme_node_recent_block() from remaining tasks table.

andypost’s picture

Roles and Vocabularies listings are depends on conversion to their ListControllers
#1872870: Implement a RoleListController and RoleFormController
#1891690: Use EntityListController for vocabularies

andypost’s picture

Issue summary: View changes

Removed theme_status_report from remaining tasks table

duellj’s picture

Issue summary: View changes

Opening new issue for book conversion

duellj’s picture

Issue summary: View changes

Removing image table conversions

c4rl’s picture

It seems to me that given the improvements of #1848064: Allow to filter modules by arbitrary search strings on the Modules page we could feasibly get rid of the fieldsets on /admin/modules and replace the whole thing with a table, so we'd add theme_system_modules_details to the list above.

c4rl’s picture

Sorry, I misspoke -- those are details, not fieldsets :)

c4rl’s picture

Issue summary: View changes

Added @ to issues.

c4rl’s picture

Issue summary: View changes

Adding theme_system_modules_details to the mix

thedavidmeister’s picture

Title: [meta] Convert form tables in core to new #type 'table' » [meta] Convert all tables in core to new #type 'table'

#type and #theme aren't about usage intentions or whether we're "inside" or "outside" the form API (we're always just "inside HTML" for the purpose of rendering strings), #type sets defaults for use within drupal_render() while #theme optionally defines a theme() callback for rendering #children within drupal_render().

#type is not limited to form elements or the form API, see #type => 'html_tag' rendering meta and script tags, #type => 'page' or #type => 'markup' for examples.

I'd personally love to see all tables in core converted to #type table, keeping in mind that all #type => 'table' renderable arrays are by default also #theme => 'table' renderable arrays thanks to system_element_info()

I agree with #29 in that just because something lives in form.inc instead of common.inc or similar isn't a strong reason to derail efforts to improve consistent use of APIs across the board.

If there's a strong technical use-case to discriminate between "form tables" and "not form tables" we should create #type => 'form_table' to achieve this kind of separation/distinction rather than rely on splitting them across #theme and #type, the zoning of which can get pretty arbitrary depending on who you talk to.

joelpittet’s picture

@thedavidmeister, I do see that I am not correct, though it's in form.inc that is not a good reason.

Most of my reasoning is around the change order: [#1876710]
Which indicates type table's intent was to help consolidate 'tabledrag' and 'tableselect', which seemed at the time to be form related.

Also noticed #type=>table is adding a bit more of overhead than #theme=>table
Callbacks:

'#value_callback' => 'form_type_table_value',
'#process' => array('form_process_table'),
'#element_validate' => array('form_validate_table'),
'#pre_render' => array('drupal_pre_render_table'),

And #rows on #type=>table uses #rows the same way as #theme=>table BUT 'drupal_pre_render_table' transforms the children_element() into #rows in #type=>table, so rows are built in completely different ways (one for the theme way, and for building form tree elements). **which has made me threaten my computer screen a few times:P**

end rant

thedavidmeister’s picture

And #rows on #type=>table uses #rows the same way as #theme=>table BUT 'drupal_pre_render_table' transforms the children_element() into #rows in #type=>table, so rows are built in completely different ways (one for the theme way, and for building form tree elements). **which has made me threaten my computer screen a few times:P**

I haven't tried my hand at a table conversion yet but this sounds heinous, all the more reason to try and get things consolidated or formally split. I'm sure it's as frustrating for contrib module maintainers to alter this stuff as it is to convert it.

Also noticed #type=>table is adding a bit more of overhead than #theme=>table

Has this been profiled anywhere? Is this overhead per-table or is it per-row or even worse, per-cell? If it's particularly bad then that could be justification to split "form tables" and "other tables".

I'm not saying definitely don't split the tables, I'm just saying that if we do want to split "types" of table lets actually make different #types and formally document which should be used where. For example, you could make a #type => 'table_fast' that just sets #theme => 'table' but not the extra callbacks and that would then work the same way as #theme => 'table' does currently but be way easier to learn/use for new developers.

tstoeckler’s picture

Has this been profiled anywhere? Is this overhead per-table or is it per-row or even worse, per-cell?

As far as I'm far no one has, but that's a good thing; that would clearly be premature optimization. @joelpittet is certainly right that there's a little overhead when using #type => table, but that is a couple function calls per table. The only function that probably is noticable at all in terms of the table rendering is the pre_render callback that transforms element_children() into #rows. And once we have all instances in core converted we could even consider making theme_table() render the children directly. So in terms of directly comparing the 2 drupal_render() calls of #type table and #theme table you might notice a small difference, but in terms of the page rendering time of a Drupal page, this is totally negligible.

thedavidmeister’s picture

And once we have all instances in core converted we could even consider making theme_table() render the children directly.

+1. That's a great example of why I'm pushing for more consistency, so we can actually do things like this and be confident they won't cause more problems than they solve :)

thedavidmeister’s picture

#2012812: drupal_render() can't distinguish between empty strings from theme() and no hook being matched is relevant to what you're suggesting @tstoeckler. theme_foo(), if referenced as #theme => 'foo' is *supposed* to be completely responsible for rendering children already, according to all our documentation at least..

tstoeckler’s picture

Well, currently I'm not available of any #theme => table actually containing children. #type => table ones do have children, but they are put into #rows in #pre_render. Looking at theme_table() I do not see it doing anything with its children at all.
Hmm...

thedavidmeister’s picture

#44 - no, currently the system doesn't work in that way (theme function always handling children). It says it does but it seems like there's plenty of exceptions to this.

Let's work to get everything consistent then consider at our next steps :)

Cottser’s picture

At least in some cases (see #1938926-18: Convert simpletest theme tables to table #type), #type table is very slow. I'm wondering if we should create an issue to improve its performance.

joelpittet’s picture

@Cottser profiled simpletest conversion and we are getting huge performance issues around 35-40% slower.
#1938926-19: Convert simpletest theme tables to table #type

thedavidmeister’s picture

#46 sounds like a great idea to me.

thedavidmeister’s picture

Issue summary: View changes

Added 2013258

Cottser’s picture

Issue tags: -Novice

Removing novice tag, I don't think this has proven to be a good fit.

moshe weitzman’s picture

#1938926: Convert simpletest theme tables to table #type has landed, with some good accessibility and other improvements to #tableselect. Let's keep on going with these conversions as they block some theme simplifications.

Cottser’s picture

xjm’s picture

Priority: Normal » Major
jibran’s picture

Can we close this now only two children are open now? #1938912: Convert language content setting table theme to a twig template and #1938930: Convert theme_system_modules_details() to #type table if I am not missing any others.

tstoeckler’s picture

I just re-opened #1938900: Convert theme_field_ui_table into a template, that was closed prematurely. (That doesn't mean we should keep this open, just saying)

Cottser’s picture

I don't think we should close the meta early just because there are a small number of child issues left. What's the motivation @jibran?

jibran’s picture

What's the motivation @jibran?

When there are at least two issues remain to fix a meta. I have seen @catch close meta as duplicate and switch the proprieties of children according to meta.

Cottser’s picture

Thanks for the clarification @jibran :) Interesting, I don't recall having seen that before. Personally I think we should leave this as is, it's not contributing to critical counts or anything.

lokapujya’s picture

Issue summary: View changes
akalata’s picture

Issue summary: View changes

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.