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: | needs work |
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 | Status | Test result | Operations |
|---|---|---|---|---|
| tselect.patch | 3.43 KB | Idle | Failed: Failed to install HEAD. | View details | Re-test |
#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.
#24
Re-roll to CVS HEAD.
#25
Thanks for the reroll, lilou.
Note that we finally have the above mentioned test written and RTBC. so this form element should proceed soon.
#26
I have code reviewed the patch, and run the new simpletest for User Administration. 35 passes, 0 fails, 0 exceptions. RTBC.
#27
Hm. Is there a reason we don't convert admin/content/node and admin/content/comment to use this same construct?
#28
No reason - we are just trying to keep the patch small, and rightfully so because it has taken a long time to get it in already. I'll make sure those pages get converted after this goes in. I also signed up to do the docs.
#29
Patch in #24 misses most of the changes to user.admin.inc. #15 no longer applies due to the #value => #markup change.
#30
I'll convert other forms in separate issues.
#31
Comment admin overview converted on http://drupal.org/node/311072.
This makes me wonder if we should introduce an #empty property to display a default empty message in the entire table width.
#32
I just noticed the expand functions changed naming convention.
#33
Yes, an #empty property would be required. Subscribing...
#34
subscribe
#35
Say, Heine? While you're fixing things up, +function theme_tableselect($element) { needs some PHPDoc.
#36
as moshe pointed out this would be really helpful for #322287: Add file.module to provide UI for managing files
#37
I've renamed expand_tableselect to form_process_tableselect as well as add support for an #empty attribute (which is assumed to be t('No items exist.') if not set). No PHPDoc yet, and I think my change still requires some work. Worth a look, though.
#38
(Forgot to mention: Keep in mind that the user admin page still prints debugging output instead of saving.)
#39
Converted comments, made #empty '' when not set and #value array() when there are no items.
#40
code looks great. comment and user admin tests pass. rtbc.
#41
The last submitted patch failed testing.
#42
failure was due to HEAD not installing properly
#43
#44
The last submitted patch failed testing.
#45
See: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476
#46
Sorry, I am confused. This was RTBC before. Why CNR now?
#47
You're right.
#48
The last submitted patch failed testing.
#49
reroll
#50
Non-empty patch file
#51
Code looks good (still). The admin/user/user tests still pass - lets commit this.
#52
Very very nice!
#53
I'm beginning to think this issue is cursed. Please - commit or say whats wrong, folks. If this patch won't apply anymore, at least say if it is committable once fixed.
#54
At various points, I've asked some themers in #drupal to take a look at this issue, and so far haven't had any takers. So if you can grab someone who would be likely to deal with overriding this function who could provide an informed review, that would certainly expedite its commit process.
A FAPI guru wouldn't hurt either.
#55
webchick - please reconsider he request for more reviewers. this has been given a thimbs up by: webchick: heine, moshe, chx, wim leers, catch, drewish, Dave Reid , Arancaytar. i think this group has quite some themeing know how. i'm not sure who else to ask.
#56
re-roll to keep up with HEAD...
#57
The last submitted patch failed testing.
#58
Let's try with Unix linebreaks.
#59
Issue was already RTBC, and the changes in #58 were only to sync with HEAD.
#60
Chasing HEAD.
#61
The last submitted patch failed testing.
#62
test bot still doesn't like the file naming test.
#63
Ok, I finally got a chance to sit down for awhile with this patch tonight. This is a good patch, and eliminates a lot of duplicate code in favour of a structured, consistent way to deal with "I have a big table of stuff I want to select ala admin/build/node and such." It also makes said code a lot more legible.
I don't believe Dries's earlier comments about the PHPDoc are addressed, however. I, too, can't figure out what these new functions do purely from their descriptions. This is particularly an issue because one of these functions is "themer-facing":
/**+ * Render the element as a table, with the expanded children in the first row.
Sorry, but... what? :) This sounds like something that might be an inline comment within the function. But the purpose of the PHPDoc summary line is to signal to people who are looking at a page like http://api.drupal.org/api/group/themeable/7, "Oh, THAT'S the function I need for X." The other theme_TYPE theme functions tend to be like this:
* Format a set of radio buttons.So maybe something like:
* Format a table with checkboxes or radio buttons to select items within it.Then...
+ * @param $element+ * An associative array containing the properties of the element.
All other theme functions in the vicinity here include a line directly after this like this:
* Properties used: title, value, id, required, error.Let's add one here too as well, please.
* Create the right amount of checkboxes or radios to populate the table.Should be "correct" amount. Also, this is missing a @param and @return.
Now, aside from documentation-related concerns, I also have a couple of naming-related ones too.
Looking at the default implementation in system_elements():
+ $type['tableselect'] = array(+ '#input' => TRUE,
+ '#advanced_select' => TRUE,
+ '#multiple' => TRUE,
+ '#process' => array('form_process_tableselect'),
+ '#options' => array(),
+ '#empty' => '',
+ );
I can figure out (or mostly confidently guess) by reading most of these what they mean, but I'm stuck on '#advanced_select' .. I have no idea to what that's referring. Could this be renamed to something a bit closer to what it actually flags? Maybe #select_all ?
This confusion is also a good indication that both theme_tableselect() and form_process_tableselect() could use some more inline comments to explain what's going on in there. My general rule of thumb is one inline comment every 4-5 lines of code (or the closest place where it logically makes sense).
Not for this patch, since I imagine this is some silly FAPI internals thing, but it's really odd that checkboxes have a #processed property and #radios have a #spawned property.
+ if (empty($options)) {+ $rows[] = array(array('data' => t('No users available.'), 'colspan' => '7'));
+ $form['accounts'] = array(
+ '#markup' => theme('table', $header, $rows),
+ );
+ }
+ else {
+ $form['accounts'] = array(
+ '#type' => 'tableselect',
+ '#header' => $header,
+ '#options' => $options,
+ );
+ }
Shouldn't this be an #empty property instead?
One final thing is that radio/radios, checkbox/checkboxes, text/textarea, form, date, file, etc. are all semi-"native" things in HTML. "tableselect" is not. At one point when I asked for reviews in #drupal, I remember someone being confused by this (though it doesn't appear they commented here to say this). However, we had a good round of bikeshed discussion and couldn't come up with anything better, and this patch has already been held up enough so we can try and think of something more clear sometime before code freeze as a follow-up patch.
Going through the rest of the code, I didn't spot anything else. I do think that we need tests for this though. We are introducing a big glomp of complex code here with many twisty passages and need to make all of the edge cases are working, esp. since this is going to be re-used everywhere. That means testing:
* Are checkboxes displayed when multiple is true, radios displayed when multiple is false?
* Is the empty text displayed when the table is empty?
* Is the select all checkbox rendered in the header if that property is set to true?
* Are correct selected element(s) retained when the form is submitted?
Without this, I have a feeling we're going to end up breaking this and not realizing it down the road. There is modules/simpletest/tests/form.test now which this can be added to as a new test case. If you need any help, there are lots of testing experts at all times in #drupal who would love to help you out.
So. Comments, some basic tests, and take a look at that chunk of code. If those things get done, I have no problem committing this.
#64
Webchick, thanks a lot. I'll be working on this.
#65
Hello. Webchick, Crell, et al. is too shy to mention this.
Consider using the term '#type' => 'autotable' or '#type' => 'magictable' instead of 'type' => "tableselect". Toss it around a bit in your head. Have an epiphany.
#66
I've renamed #advanced_select to #js_select, added a few comments, a number of tests, but pending the epiphany, the #type is still tableselect.
As #spawned is a D5 construct and no living soul remembers the "Why" of #processed apart from "Some optimization", both have been removed.
#67
The last submitted patch failed testing.
#68
Rerolled for whitespace change.
#69
The last submitted patch failed testing.
#70
Updated patch to include the new test module.
#71
Did another code review. Looks great. Since the extensive tests have passed, I'm comfortable with RTBC here.
#72
I don't see why all the _form_test_tableselect_multiple_true_form(), _form_test_tableselect_multiple_false_form(), _form_test_tableselect_empty_form(), and _form_test_tableselect_js_select_form() functions can't be combined into one form builder function:
<?php
function _form_test_tableselect_form($form_state, array $options) {
$form = array();
list($header, $options) = _form_test_tableselect_get_data();
$options += array(
'#type' => 'tableselect',
'#header' => $header,
'#options' => $options,
'#multiple' => FALSE,
'#js_select' => FALSE,
'#empty' => t('Empty text.'),
);
$form['submit'] = array(
'#type' => 'submit',
'#value' => t('Submit'),
);
return $form;
}
function _form_test_tableselect_multiple_true_form() {
return _form_test_tableselect_form(array('#multiple' => TRUE));
}
function _form_test_tableselect_multiple_false_form() {
return _form_test_tableselect_form();
}
function _form_test_tableselect_empty_form() {
return _form_test_tableselect_form(array('#options' => array()));
}
etc...
?>
Replace
list($header, $options) = $this->getData();with _form_test_tableselect_get_data()...don't duplicate code.#73
Review:
+ * Format a table with radiobuttons or checkboxes"radio buttons or checkboxes." [radio buttons is two words, needs a period at the end.]
+ 'operations' => array('data' => t('Operations')),put the last ) on the following line.
+ * Test the tableselect form element for expected behaviour....
+ 'description' => t('Test the tableselect element for expected behaviour'),
behavior. We use US English. Sorry. :(
+ $this->assertNoFieldByXPath('//th[@class="select-all"]', NULL, t('Do not display a "Select all" checkbox when #js_select is FALSE.'));+
Extra blank line here.
So those are all minor. The patch itself is good to go from my perspective once these are fixed.
Next, I reviewed the tests, and got a little confused in places:
1. getData() and _form_test_tableselect_get_data() look to be exactly the same function. Since the setUp() function of the test already enabled the module, could you not just call _form_test_tableselect_get_data() as needed from the test?
2. I don't understand the purpose of function formSubmitHelper(), and none of our other tests need such a helper. Why not just do a drupalPost() and check its results to make sure that they do/do not contain the text that they should/shouldn't?
#74
I will make a patch based on these comments.
I need formSubmitHelper() to simulate posting invalid values to the form, so I can test if the FAPI #options checker works. When I try to post invalid values with drupalPost, simpletest itself complains about not finding fields.
As formSubmitHelper depends on FAPI implementation details I also would like another way of testing this.
#75
Here's an updated patch. Pending #346095: Test #ahah and ajax, formSubmitHelper() is still necessary.
#76
The last submitted patch failed testing.
#77
re-roll...
#78
and CNR... Do I need to do this to get the tests to run automatically, or will code needs work statuses also get tested?
#79
Patches on issues that are Code Needs Work don't get tested automatically.
#80
I gave this one last read through and then committed it. The test coverage is outstanding, it solves a huge inconsistency problem, and since this issue pre-dates #346095: Test #ahah and ajax by many fortnights, I'm okay with formSubmitHelper() being in there as a temporary solution for now.
Leaving CNW because the upgrade docs and the form API reference need to be updated to reflect this new addition.
AWESOME WORK, Heine!! :D
#81
Oops. I thought I marked it CNW, anyway. ;)
#82
Humm... noticed that the sorting is totally screwed up on this form element right now. I would have opened a new issue but since this is still CNW i guess we could address it here.