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

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'

Updated issue summary.

Tagging

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

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

---

Update: added table type

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.

@jwilson3 sorry! Updated table.

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

Here's a related issues from mobile initiative #1876196: Handling admin pages with tables and #1870944: [Meta] Mobile friendly admin pages

@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!

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?

#10: drupalcore-convert_all_tables_in_core-1876712-10.patch queued for re-testing.

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

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 :).

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

Updated issue summary.

Splits tasks into subissues

Cleans up table

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

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.

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.

@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.

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...

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.

@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.

Remove poll module

@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.

#1948374: #type 'table' allow attributes on table cells
Related issue

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.

@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?

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.

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

Removing theme_file_formatter_table() since not a form element

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

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

Removing theme_node_recent_block() from remaining tasks table.

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

Removed theme_status_report from remaining tasks table

Opening new issue for book conversion

Removing image table conversions

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.

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

Added @ to issues.

Adding theme_system_modules_details to the mix

Related #2005970: In renderable arrays, #type should provide a #theme suggestion, similar to how drupal_prepare_form() works

#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.

@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: New render element #type 'table'
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

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.

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.

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 :)

#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..

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...

#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 :)

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.

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

#46 sounds like a great idea to me.

Added 2013258

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

#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.

Promoting to major as part of #2348381: [META-0 theme functions left] Convert/refactor core theme functions.

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.

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)

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?

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.

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.

Only #1876714: Remove #type 'tableselect' remains as a follow-up to this one, closing this out.