Make theme_table() renderable array compliant
| Project: | Drupal |
| Version: | 8.x-dev |
| Component: | forms system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | vangorra |
| Status: | needs work |
Having dealt with some tables in Drupal, I found them quite difficult to construct. That’s why I have written a Forms API implementation of tables.
Now, you can create tables directly in Forms API without the need for theme(‘table’) in custom theme functions.
<?php
$form = array(
'system' => array(
'#type' => 'table_table',
'#header' => array(
0 => NULL,
1 => array('data' => 'Name', 'field' => 'name', 'sort' => 'asc'),
),
0 => array(
'status' => array (
'#type' => 'checkbox',
'#default_value' => '1',
),
'name' => 'table',
),
1 => array(
'status' => array (
'#type' => 'table_cell',
'status' => array (
'#type' => 'checkbox',
'#default_value' => '1',
),
),
'name' => 'taxonomy',
),
),
);
?>In #type => 'table_table', you can specify the header of the table in the #header attribute. It takes a regular header structure as returned by tablesort_init(). Table sorting is fully supported (with pager_query and so on). You can now populate the rest of your #type => 'table_table with the rows. These rows (0, 1 in the example above) contain the cells (‘status’ and ‘name’ in the example).
There are three possibilities: If the cell is a string, it is expanded to array('#value' => $cell) and put into a table cell automatically. If it is an array (thus an arbitrary form element such as checkbox, radio or textfield), it is wrapped by a table cell. If it is a table cell, nothing specifically happens.
This behavior implies that you can place arbitrary form elements inside a table cell without writing specific theme functions that handle these. They are automatically generated by Forms API.
Attached is a module that implements the table. I’d like some people to check my code and if this approach makes sense. The module does also include a sample table at admin/settings/table that maps the SQL table ‘system’ to a table that can be browsed.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| table.module | 4.79 KB | Ignored | None | None |
#1
I forgot to say that I will go on and convert this module to a patch for Drupal core that replaces theme('table') and converts all tables to this model removing the need for specific theme functions.
#2
Now minus the
var_export.#3
I agree with the theory, and tried it earlier this year
The result was a nice big demo and something very similar to what you did.
(I called the elements formtable, tr and td. It's not as tied to the tablebuilder as yours, it chooses Form API - like syntax instead ... but hence doesn't support tablesort.)
My implimentation (80% docs) is attached.
I'm using it in several places already.
Everything I glanced at in your code made sense (note glanced, it's past my bedtime), but the expand function looks a bit inelegant.
maybe I'll sleep on it.
#4
It sounds like you propose to remove them theme_table() function to which I say ouch. Not everything the uses that function is a form.
#5
I like the general idea a lot, but why not just extend FAPI so that drupal_render() knows to render the elements in each row and then call theme('table')?
#6
I had a conversation with chx yesterday evening and he told me that this data model (rows instead of columns) could result in quite some difficulties using
#type => checkboxes.@nevets you can build your tables using this model even without drupal_get_form(). Just construct the array in the same way and use drupal_render() to get the HTML representation.
#7
I like the idea and the implementation, but I doubt that it warrants a new core module. Maybe a .inc file? The table_elements() return value could then be added instead to system_elements(). We've already got tablesort.inc. Maybe that should be changed to table.inc, and these functions included there.
#8
Oh, I never intended it to be a module. That is just for development/reviewing purposes becasue it makes it easier to show what it does without the need to reroll patches all the time (see #2). If this is considered for inclusion in core, I will happily provide a patch. Note that this definitely won't make it into 4.8/5.0.
#9
Minor correction. The pain is with radio buttons and no matter what you do, if you use #type radio instead of #type radios you need advanced mojo.
'table_row' => array(),this is unnecessary. hook_elements is for defaults, if you have no defaults, omit it, noone checks whether a specific type is defined in any hook_elements.#10
It's true that defaults is our only current core use. But e.g. in formbulder.module we need a way to answer the question "What are all the defined form element types (so we can offer a UI for building them)?". It's best IMO if we return all eliment types, whether or not they have defaults.
And we seem to do this already in system_elements:
<?php$type['item'] = array();
?>
#11
THis should not be a separate module.
I wrote one of these awhile ago http://cvs.drupal.org/viewcvs/drupal/contributions/sandbox/drumm/extra_e...
#12
I like Drumms version.
#13
+1
#14
I have started developing a much more enhanced version which when finished will integrate into the formapi.
see http://cvs.drupal.org/viewcvs/drupal/contributions/sandbox/gordon/table_...
#15
Feature freeze requires this to be sent to D7.
#16
maybe you could get some inspiration/code from: http://drupal.org/node/103171
#17
subscribe. we'll *have* to get this in for D7
#18
I did some quick testing of drumm's form element and still works in 5 (it was built for 4.7). I only had to change form_render => druapl_render(). See his code at http://cvs.drupal.org/viewvc.py/drupal/contributions/sandbox/drumm/extra.... I think this is a nice, small implementation.
#19
subscribing
#20
Lots of different implementations by various developers. Can we revive this and get a consensus on the best patch to take forward?
#21
See #297893: #theme Form elements into tables ... I don't know if my patch is a duplicate of this, or something kinda different.
#22
Anyone want to pick this up? we have #tableselect now, but thats a bit different.
#23
Another related discussion with a nice patch is here: http://drupal.org/node/528932
#24
Here is a work in progress, proof-of-concept patch, that aims at properly implementing theme_table() in the renderable array model.
The basic structure of a table is now:
<?php$table = array(
'#type' => 'table',
'#columns' => array(
'col1' => array(
'#markup' => t('Column 1'),
),
'col2' => array(
'#markup' => t('Column 2'),
),
),
'row1' => array(
'col1' => array(
'#markup' => t('Cell 1'),
),
'col2' => array(
'#markup' => t('Cell 2'),
),
),
);
?>
The table is rendered in the order of the columns in the #columns parameter, and only columns that are in the #columns parameter are rendered. That should allow to add a column and reorder columns easily.
#25
The last submitted patch failed testing.
#26
Thanks for working on this.
- '#theme' => 'table',- '#header' => $header,
- '#rows' => $rows,
+ '#markup' => format_table($header, $rows),
The problem here is that we have a string by the time any alter hook comes along. exactly when is a module supposed to come in an add its extra column? if thats not possible, then this patch is a regression.
#27
@Moshe: all the format_table() are temporary back-ward compatible hacks. I have done a full conversion in node_revision_overview(), but I'm still trying to figure out if this is the correct API, from a DX perspective. Feedback is more then welcome.
#28
Thanks. I just looked at node_revision_overview(). Seems a bit verbose to me. The #tableselect syntax looks a bit more terse. See http://heine.familiedeelstra.com/new-tableselect-form-element-in-core
#29
subscribe.
I am closing http://drupal.org/node/528932 in favour of this thread.
I've written a patch which declares a new FAPI element call 'tableform'. It will theme a form structure using theme_table. My goal is to get this element included in Drupal 7, so any feedback will be greatly appreciated.
Example of usage:
<?php
$form = array(
'table' => array(
'#type' => 'tableform',
'#header' => array('col1', 'col2', 'col3'),
'#caption' => 'Hello',
'#tabledrag' => array(
'action' => 'order',
'relationship' => 'sibling',
'group' => 2,
'hidden' => false,
),
),
);
for($i = 0; $i < 10; $i++) {
$form['table']["row-$i"] = array(
'col1' => array(
'#type' => 'checkbox',
'#default_value' => rand(0,1),
),
'col2' => array(
'#type' => 'checkbox',
'#default_value' => rand(0,1),
),
'col3' => array(
'#type' => 'select',
'#default_value' => 0,
'#options' => array_combine(range(0,10),range(0,10)),
),
);
}
$form['table']['row-1']['#draggable'] = false;
?>
#30
Heh. That's not unlike what I did back in 2006.
Where do the cells #attributes end up? the code looks like it goes on the td, but we also need behaviour would be for it to go on the element.
It'll need a lot of testing to make it into D7, I'm thinking.
I like the idea, of course, but good luck!
#31
@dman Actually the #attributes apply to the form element itself, here is a patch that will use '#cell_attributes' to apply attributes the .
Example:
<?php
$form = array(
'table' => array(
'#type' => 'tableform',
'#header' => array('col1', 'col2', 'col3'),
'#caption' => 'Hello',
'#tabledrag' => array(
'action' => 'order',
'relationship' => 'sibling',
'group' => 2,
'hidden' => false,
),
),
);
for($i = 0; $i < 10; $i++) {
$form['table']["row-$i"] = array(
'col1' => array(
'#type' => 'checkbox',
'#default_value' => rand(0,1),
'#attributes' => array(
'class' => array('test_field_class'),
),
'#cell_attributes' => array(
'class' => array('table_cell', 'test_cell_class'),
),
),
'col2' => array(
'#type' => 'checkbox',
'#default_value' => rand(0,1),
),
'col3' => array(
'#type' => 'select',
'#default_value' => 0,
'#options' => array_combine(range(0,10),range(0,10)),
),
);
}
$form['table']['row-1']['#draggable'] = false;
?>
#32
+++ includes/form.inc 28 Aug 2009 18:16:22 -0000@@ -2197,6 +2197,224 @@ function form_process_tableselect($eleme
+ if(is_int($tabledrag['group'])) {
All the
if(should be modified intoif (to comply with coding standards. Many occurences in the patch.+++ includes/form.inc 28 Aug 2009 18:16:22 -0000@@ -2197,6 +2197,224 @@ function form_process_tableselect($eleme
+ $attributes['id'],
+ $tabledrag['action'],
+ $tabledrag['relationship'],
+ $tabledrag['group'],
+ isset($tabledrag['subgroup']) ? $tabledrag['subgroup']:NULL,
+ isset($tabledrag['source']) ? $tabledrag['source']:NULL,
+ isset($tabledrag['hidden']) ? $tabledrag['hidden']:TRUE,
These array items have whitespace at the end of the line which should be removed.
#33
@TheRec Thanks for the feedback, here is the patch after its been run through the coder module. I've adjusted the styling to fit the coding standards.
#34
In a similar theme, the row will need #attributes also, I'm thinking.
And formbuilder really really expects every element_children() to have a #type, so it should be
$row[#type]='tablerow'I guess.Right now, it can be guessed from context, but all the other form recursion routines may be surprised to find an untyped element in the tree.
#35
@dman The patch provides functionality for the row to have #attributes (See example below). In my experience with working with the form api, any element that does not provide a #type will be assigned type #type of 'markup'. I've been creating form with empty child elements (who contain subchildren) for some time, it all seems to work so long as the empty child has the #tree property set.
My focus on this patch has been to make the process of placing a new/existing form in a table as simple as possible. I specifically chose to omit the creation of a 'tablerow' as (without tableform) would not provide any functionality and to keep the FAPI docs free of these types. What would the benefit of creating a 'tablerow' element?
Setting #attributes for a row:
<?phpfor($i = 0; $i < 10; $i++) {
$form['table']["row-$i"] = array(
'#attributes' => array(
'class' => array('rowclass'),
),
'col1' => array(
'#type' => 'checkbox',
'#default_value' => rand(0,1),
'#attributes' => array(
'class' => array('test_field_class'),
),
'#cell_attributes' => array(
'class' => array('table_cell', 'test_cell_class'),
),
),
'col2' => array(
'#type' => 'checkbox',
'#default_value' => rand(0,1),
),
'col3' => array(
'#type' => 'select',
'#default_value' => 0,
'#options' => array_combine(range(0,10),range(0,10)),
),
);
}
?>
#36
Cool on the #attributes then, I must have missed where that got passed in.
The row is not 'markup' and shouldn't assume it is. Markup has a #value, and does not have children. It may happen to work for you right now, but it leaves an assumption floating. Conceptually, I think this construct should be labelled clearly and exposed so form_alter, theming, and other code can see what's going on. For now, it's just a label (possibly redundant), but it is important enough to be explicit about. For code clarity.
Saying that a row is a row may be marginally more strict, and marginally less simple, but it's clearer and more consistent with the rest of FAPI. IMO.
And it can't hurt what you've already got...
#37
@dman I agree with your statement. The need for a tablerow type would not hurt, of course, I'll probably implement a tablecell type too as this is a cleaner solution than using the '#cell_attributes' property anyway.
I figure I'll have the process callback add the tableform_row and tableform_cell types as needed. This way we still get the flexibility I was looking for in the beginning and the extendability you speak of. I'll see if I can hammer this code out this weekend.
#38
Cool.
It would be OK if the tablecell was optional, if there is only a single form element to render. That will be the 'simple' common case, but if you want to nest more elements, or be explicit, then a tablecell wrapper would be handy. Like the tablebuilder currently does, allowing either a string or an array with data and attributes, depending on the need.
I think this functionality has been missing for ages (ever since I wanted it in 4.7!) so I hope it gets support!
#39
I'm not sure if everyone here knows that we already can put tables into a render() structure. Search for $page['tracker'], for example. I don't see much need for this patch anymore.
#40
@moshe weitzman: you can place tables in a render() structure, but you can't place a render() structure into a table. This issue is dedicated to that.
@vangorra: Why are you hijacking this issue? Have you seen that there is an already fairly advanced patch in #24?
#41
@Damien Tournoud: It is not my intent to hijack this issue. It was suggested to me to merge my issue with this one in order to get more eyes on the patch. I did see the patch in post #24 and see that we are solving the same problem with differing approaches.
It is my opinion that multiple suggestions to the same solutions helps foster open source development and in the end, can only benefit the project as a whole. I felt it necessary to post my patch as it compliments the existing theme functions rather than modifying them, a strategy that avoids the need to modify existing core and contrib modules.
#42
Taking dman's feedback into account, I have refactored the tableform code to better utilise the theme API. The patch includes 3 FAPI elements: tableform, tableform_row, tableform_cell. I'm looking forward to anybody's feedback.
Known Issues:
Rough Element Doc:
tableform
A form element that generates a table of other form elements. All children of a tableform should be of type tableform_row. If the type is ommited, it will be set. If the type is not a tableform_row, it will be removed.
Properties:
tableform_row
An element that represents a row for a tableform. Children can be a FAPI elements. If child type is not specified, then it assumed that it is a tableform_cell and set as such.
Properties:
tableform_cell
Provides a container for multiple FAPI elements to be included in the same cell of a table.
#43
Coding standards review... hopefully it will help :)
+++ includes/form.inc 31 Aug 2009 15:58:09 -0000@@ -2196,6 +2196,232 @@ function form_process_tableselect($eleme
+ * Format a table based on a form element
Should be "Formats"
+++ includes/form.inc 31 Aug 2009 15:58:09 -0000@@ -2196,6 +2196,232 @@ function form_process_tableselect($eleme
+ else if (isset($element[$element['#header']])) {
...
+ else if ($tabledrag['auto_config_group']) {
According to coding standards it should be elseif without a space.
+++ includes/form.inc 31 Aug 2009 15:58:09 -0000@@ -2196,6 +2196,232 @@ function form_process_tableselect($eleme
+ * Prepares a formtable and its rows to be themed. Also initializes the
+ * tabledrag options.
The short description of the function should be one sentence of less than 80 characters. You can be more precise under, after skipping a line.
+++ includes/form.inc 31 Aug 2009 15:58:09 -0000@@ -2196,6 +2196,232 @@ function form_process_tableselect($eleme
+ * @param $element
...
+ * @param $element
...
+ * @param $element
Missing parameter description.
+++ includes/form.inc 31 Aug 2009 15:58:09 -0000@@ -2196,6 +2196,232 @@ function form_process_tableselect($eleme
+ // Ensure that the children are tableform_row types
Missing a dot a the end of the line.
+++ includes/form.inc 31 Aug 2009 15:58:09 -0000@@ -2196,6 +2196,232 @@ function form_process_tableselect($eleme
+ // Set the #type for cell that should have it. (if the dev is being lazy)
"(if the dev is being lazy)", maybe this does not have its place in core ;)
This review is powered by Dreditor.
#44
@TheRec Thank you for the review. It looks like the coder module can't catch everything. Also, the Dreditor is pretty wicked.
I've made changes and cleaned the code a bit more in the process. Hopefully, I didn't stray away from the standards again.
#45
Note, with these patches #565496: Allow dynamic attaching of other types of stuff to render() structures and #564880: DX: Change the parameters of drupal_add_tabledrag() to an array of options, draggable tables might be built this way:
$table['#attached_tabledrag'][] = $tabledrag_options_array;#46
@dropcube: Both those patches you propose would really compliment the formtable as well as any other FAPI elements. Have you had a chance to work with the formtable patch? I would really appreciate your feedback.
#47
Since #565496: Allow dynamic attaching of other types of stuff to render() structures has been committed, I will implement a $tableform['#attachment']['tabledrag'] to the tableform element. On an additional note, this newest version of the patch cleans thing up just a bit more. Stay tuned for attachment support. :)
**Quick update. It is not elegant to implement the tabledrag '#attachment' property until #564880: DX: Change the parameters of drupal_add_tabledrag() to an array of options is approved.
#48
The last submitted patch failed testing.
#49
subscribing
#50
sub
#51
Tagging absolutely critical clean-ups for D7. Do not touch this tag. Please either help with this or one of the other patches having this tag.
#52
I'm new to this thread. This could help #616240: Make field UI screens extensible from contrib very much...
It looks like, since patch #44, the only pending remark is:
"we could add tabledrag through
$tableform['#attached']['tabledrag'] = $options;instead of$tableform['#tabledrag'] = $options;" - which requires #564880: DX: Change the parameters of drupal_add_tabledrag() to an array of options"(comment #47)
Is that really a blocker, or can it be changed in a followup ?
Is any of the people following this thread confident enough to mark this RTBC ?
#53
I'm also new to this thread and have not read it yet, but am subscribing.
As an FYI and crosslink, as of http://drupal.org/node/602522#comment-2219964, _theme_table_cell() now has it that if a cell is an array with a 'data' property, and the value of that 'data' property is an array, then it gets drupal_render()'d. This was needed for that issue, but maybe some of you have ideas for how that can be made better, especially with the work done on this issue.
#54
I see nothing here that can't live in contrib for Drupal 7. It's a nice new feature for Drupal 8.
#55
Except it would be really great for core Field UI - #616240: Make field UI screens extensible from contrib (required to let fieldgroup tie into the UI from contrib)
But yeah, doesn't seem like it will happen in D7 now.