Add tableselect form element

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?

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

I tested and it works as designed. Gets rid of a nasty theme function as well.

I wonder whether tablesort_sql could be also fixed... using current (the array pointer is reset afer assigment)...

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

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.

thanks for the new doxygen.

I agree with the RTBC. This is one nice patch.

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

Did a quick check and this still applies and works. Can we please get some feedback here? Thanks.

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.

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.

Heine has written excellent documentation at http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/elements/RE.... Now, where tp put this in Drupal?

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.

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.

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.

Re-roll to CVS HEAD.

Thanks for the reroll, lilou.

Note that we finally have the above mentioned test written and RTBC. so this form element should proceed soon.

I have code reviewed the patch, and run the new simpletest for User Administration. 35 passes, 0 fails, 0 exceptions. RTBC.

Hm. Is there a reason we don't convert admin/content/node and admin/content/comment to use this same construct?

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.

Yes, an #empty property would be required. Subscribing...

subscribe

Say, Heine? While you're fixing things up, +function theme_tableselect($element) { needs some PHPDoc.

as moshe pointed out this would be really helpful for #322287: Add file.module to provide UI for managing files

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.

(Forgot to mention: Keep in mind that the user admin page still prints debugging output instead of saving.)

code looks great. comment and user admin tests pass. rtbc.

failure was due to HEAD not installing properly

See: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476

Sorry, I am confused. This was RTBC before. Why CNR now?

You're right.

Code looks good (still). The admin/user/user tests still pass - lets commit this.

Very very nice!

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.

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.

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.

re-roll to keep up with HEAD...

Issue was already RTBC, and the changes in #58 were only to sync with HEAD.

Chasing HEAD.

test bot still doesn't like the file naming test.

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.

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.

Did another code review. Looks great. Since the extensive tests have passed, I'm comfortable with RTBC here.

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:

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.

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?

re-roll...

and CNR... Do I need to do this to get the tests to run automatically, or will code needs work statuses also get tested?

Patches on issues that are Code Needs Work don't get tested automatically.

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

Oops. I thought I marked it CNW, anyway. ;)

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.

#70: core-242962-tselect_test.patch queued for re-testing.

I suppose this will be postponed to 8.x :(

@drewish, please open a new issue if the sorting issue is valid, given this has been in for more than half a year, I'm closing this issue.

We already have sep. issues for tablesort sorting issues:
#700380: Regression: Tableselect sorting is altered if '#weight' is not set
#664042: TableSort order error when no sort set in header