The patch makes poll.module 13 LoC shorter. That's already something in itself. But also, there is much more to that: the current code is ugly. The new code is nice and absolutely good example. We do not carry $_POST to a new element -- that's Drupal 5 thinking. This code violated the "all form start equal" principle.

Edit: The D6 version is tested, the D7 is not but we have unit tests for that.

CommentFileSizeAuthor
#12 poll_choice_js-331708-d6.patch3.19 KBchx
#10 poll_ahah_test.patch6.4 KBcatch
#9 poll_ahah_test.patch6.06 KBchx
#2 poll_choice_js-331708-d6.patch2.72 KBkatbailey
#2 poll_choice_js-331708.patch3.46 KBkatbailey
poll_choice_js-d6.patch2.77 KBchx
poll_choice_js.patch3.29 KBchx

Comments

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Hm, interesting. Chx tells me in chat that drupal_rebuild_form() does throw away _POST so that it does not affect the current form building (via validation or submission), so that is why it needs to be added back in.

katbailey’s picture

katbailey commented
StatusFileSize
new poll_choice_js-331708.patch3.46 KB
new poll_choice_js-331708-d6.patch2.72 KB

There was one redundant line left in in the last patch. I have removed it and re-rolled the patch for the two versions. The line in questions was 

$choice_form[$delta]['#attributes']['class'] = empty($choice_form[$delta]['#attributes']['class']) ? 'ahah-new-content' : $choice_form[$delta]['#attributes']['class'] .' ahah-new-content';

in D6, and 

$choice_form[$key]['#attributes']['class'] = empty($choice_form[$key]['#attributes']['class']) ? 'ahah-new-content' : $choice_form[$key]['#attributes']['class'] .' ahah-new-content';

in D7.
The $key/$delta variable does not exist in the new code. There is no difference between the new ahah content and the previous content because all the choices are being re-rendered and they are all coming from $form_state, nothing from $_POST.

robloach’s picture

robloach
he/him
commented

Putting on my list of patches to have a look at.... I love patches that have less net code.

dries’s picture

dries commented

Looks like a nice clean-up -- are the changes in this patch covered by tests?

catch’s picture

catch
he/him
Primary language English
commented

We don't have coverage for poll_choice_js() according to http://acquia.com/files/test-results/modules_poll_poll.module.html and applying the patch then doing stupid things in there confirmed this.

Status: Needs review » Needs work

The last submitted patch failed testing.

chx’s picture

chx commented
Status: Needs work » Reviewed & tested by the community

And yes tests are coming in http://drupal.org/node/295992

dries’s picture

dries commented
Status: Reviewed & tested by the community » Needs work

Please add some code comments to the form API related code. It would be good to explain why we do things this way, and what happens in the background. Thanks!

I would also recommend (but not mandate) that we merge #295992: TestingParty08: poll_choice_js() into this patch.

chx’s picture

chx commented
Status: Needs work » Needs review
StatusFileSize
new poll_ahah_test.patch6.06 KB

There we go: this is just the Drupal 7 version, with a very very through test. We need a followup issue, changing the testing framework to better support AHAH callback tests. But, even without that, this is quite ready.

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new poll_ahah_test.patch6.4 KB

The class and test function itself needs phpdoc, setUp() and getInfo() don't. Didn't have the heart to mark CNW since poll module gets enough abuse from me as it is, so re-rolled with this, and a couple extra line breaks between comments. Otherwise the new tests pass and it looks like a big improvement.

dries’s picture

dries commented
Status: Needs review » Reviewed & tested by the community

Committed to CVS HEAD. Thanks.

chx’s picture

chx commented
Version: 7.x-dev » 6.x-dev
StatusFileSize
new poll_choice_js-331708-d6.patch3.19 KB

D6.

dries’s picture

dries commented
Version: 6.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.