Add tableselect form element
Heine - April 5, 2008 - 10:09
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | forms system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | patch (reviewed & tested by the community) |
Description
The amount of code required to build tables on admin/user/user or admin/content/node is a waste IMO as we could simply create a reusable form element.
Attached patch implements such a form element. It provides:
- Support for tablesort_sql
- Checkboxes or radiobuttons (#multiple => TRUE vs FALSE)
- JS multi-selection via tableselect.js (when #multiple => TRUE and #advanced_select => TRUE)
Most important properties are #header and #options. Structure:
$header = array(
'col1' => 'Title of field 1',
'col2' => 'Title of field 2',
);
$options['some_unique_id'] = array(
'col1' => 'Value of field 1',
'col2' => 'Value of field 2',
);A simple example:
<?php
$header = array(
'title' => t('Title'),
'author' => t('Author'),
);
$query = "SELECT n.nid, n.title, u.name FROM {node} n INNER JOIN {users} u ON n.uid = u.uid";
$result = pager_query(db_rewrite_sql($query));
while($partial_node = db_fetch_object($result)) {
$options[$partial_node->nid] = array(
'title' => check_plain($partial_node->title),
'author' => check_plain($partial_node->name),
);
}
$form['nodes'] = array(
'#type' => 'tableselect',
'#header' => $header,
'#options' => $options,
);
?>If this patch is accepted, we can convert existing tables to use the element.
| Attachment | Size |
|---|---|
| tselect.patch | 3.43 KB |
#1
Here's a module to test the patch.
#2
will be very handy for many other modules too. devel's variable editor, views_build_operations module.
instead of a sample module why not convert a form in core?
#3
+1 for the idea/goal.
Initial review: why not use the normal Drupal formatting for FAPI elements here? I.e. instead of
$element[$key] = array('#type' => 'checkbox', '#processed' => TRUE, '#title' => '', '#return_value' => $key, '#default_value' => isset($value[$key]), '#attributes' => $element['#attributes']);This would be better:
$element[$key] = array('#type' => 'checkbox',
'#processed' => TRUE,
'#title' => '',
'#return_value' => $key,
'#default_value' => isset($value[$key]),
'#attributes' => $element['#attributes'],
);
That's much more readable.
#4
Thanks.
Attached patch has the code style change asked for by Wim Leers and one converted core form (admin/user/user).
#5
I tested and it works as designed. Gets rid of a nasty theme function as well.
#6
I've had a discussion with chx on the use of the element renderer (and rendering the checkboxes twice). Let's polish a bit.
#7
After a discussion with chx, an updated patch. drupal_render has been modified to store rendered content in the element so parent items can reuse it.
Still using #options, to get element validation. #value is in the format expected, and the validator only looks at the keys in #options, not the values.
chx feels matching keys in #header and #options[foo] are problem and should perhaps be something like:
$table['roles']['rows'][$account->uid] = theme('item_list', $users_roles);$table['roles']['header'] = t('Roles') ;
but then #header needs to be useable in tablesort_sql.
Right now, #header is not directly useable in tablesort_sql when there's no sort specified, because tablesort_sql relies on a 0 key.
#8
I wonder whether tablesort_sql could be also fixed... using current (the array pointer is reset afer assigment)...
#9
Here's a patch with the additional tablesort fixed. It uses 0 when set as the default sort, otherwise current. Added 'sql' => '' to remove another notice.
#10
$first = isset($header[0]) ? $header[0] : current($header);is hardly needed. If you have an unindexed list of header elements, then current() --for a reset array-- will return $header[0].#11
I tested and this is good to go. If you are going in one more time as chx suggests, I propose adding a little Doxygen to theme_tableselect(). I will RTBC after those minor improvements.
#12
Thanks for the reviews & help. I initially kept [0] as default for explicitly set keys. But this is indeed nonsense; people can set 'sort'.
#13
thanks for the new doxygen.
#14
I agree with the RTBC. This is one nice patch.
#15
One space missing in doxygen for theme_tableselect. I can't in good conscience mark it needs work, so here's a patch with the extra space.
Pretty please don't credit on commit.
Also, very nice indeed! I saw those theme functions in user module the other day and it seemed like overkill. Now it won't be :)
#16
What does this patch need in order to get in or be rejected?
Benefits:
#17
Did a quick check and this still applies and works. Can we please get some feedback here? Thanks.
#18
I started by reading the PHPdoc of this patch. Based on the PHPdoc, I could not figure out how to use this function or what its purpose is.
Would also be nice if we had tests for this new function.
#19
Thanks for the review.
I think decided a long time to document fapi properties outside of the code. We settled on http://api.drupal.org/api/file/developer/topics/forms_api.html/5 and http://api.drupal.org/api/file/developer/topics/forms_api_reference.html. We can change our minds about that, but at present virtually no fapi elements are documented in code.
I don't see any tests for form.inc at all. Am I missing them? We can start with this one but I'd love some discussion about how we want to test FAPI elements.
#20
Heine has written excellent documentation at http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/elements/RE.... Now, where tp put this in Drupal?
#21
I have shown above that we have documentation waiting for api.drupal.org regarding this new form element. I looked but simply cannot find a suitable place for it in core.
We have zero form testing in core and I don't think this patch is the right place to initiate such an effort. So, I humbly set this to RTBC. I will personally update api.drupal.org if this goes in.
#22
It seems like the easiest way to add a test for this would be the admin/user/user pages since there's an implementation of an actual form in the patch - there's an existing issue here (claimed by a new tester) for writing those tests #276583: TestingParty08: Tests for admin/user/user filters.
#23
Excellent proposal, Catch. I hope that the committers don't think that a test of that page is a prerequisite for this patch but we shall see.