Ok, first try. All settings are now per content type, and the global settings page is removed. We can ad it back if we need it again.

I changed $node to $node_type in _advpoll_get_mode() to be able to call it easier from advpoll_form_alter, and as I see it the full node object is not needed. That function is a bit hackish. I once added a check to _advpoll_list_modes() to see if the mode was listed, but I removed it again for performance reasons.

Removed "default" from variable names and constants since the variables are renamed anyway, and "default" redundant. It would be nice with an upgrade path for the previous default settings, or we should at least delete the old variables. Chris, could you help me with that?

CommentFileSizeAuthor
#9 advpoll_default_2.patch12.92 KBanders.fajerson
#8 advpoll_default_1.patch12.89 KBChrisKennedy
#2 advpoll_default_0.patch11.96 KBanders.fajerson
advpoll_default.patch11.87 KBanders.fajerson

Comments

ChrisKennedy’s picture

ChrisKennedy commented

This is awesome!

A few small things:
* The algorithm description needs to be translated (as does the original, apparently).
* For me, the new settings show after the "save content type", "reset to defaults" buttons, and aren't collapsible. Is a cache clear required or something?
* Need to uppercase that "false".

And yeah, I'll help with the settings upgrade path.

anders.fajerson’s picture

anders.fajerson commented
StatusFileSize
new advpoll_default_0.patch11.96 KB

1. Fixed
2. I can't see this. Would be great if you could do some digging.
3. Coder module once told me that "false" was faster than "FALSE". But a search in HEAD reveals that FALSE is the one used. Fixed. (some other places still exists, but that's for a code style patch).

ChrisKennedy’s picture

ChrisKennedy commented

Okay I found out that the collapsible part is due to a javascript error or something unrelated.

The ordering problem continues to exist though. The code does seem to be the issue here - advpoll_form_alter() is inserting $form['advpoll'] after $form['submit'] and $form['reset'] have already been created in node/content_types.inc. I don't see how you wouldn't also be experiencing this, hopefully I'm not crazy.

For me this code fixes the ordering by moving $form['submit'] & $form['reset'] after $form['advpoll'].

if (strpos($node_type, 'advpoll_') !== FALSE) {
      $submit = $form['submit'];
      $reset = $form['reset'];
      unset($form['submit'], $form['reset']);

      $form['advpoll'] = array(
        '#type' => 'fieldset',
        '#title' => t('Poll settings'),
        '#collapsible' => TRUE,
      );

      $form['submit'] = $submit;
      $form['reset'] = $reset;

Or maybe you could use weighting.

anders.fajerson’s picture

anders.fajerson commented

That should not be needed. Are you sure you are using a clean checkout of Drupal-5? If you print the form:

...
if (strpos($node_type, 'advpoll_') !== FALSE) {
dpr($form); // If devel module is installed
...

You see that submit has a weight of 40 and reset a weight of 50:

    [submit] => Array
        (
            [#type] => submit
            [#value] => Save content type
            [#weight] => 40
        )

    [reset] => Array
        (
            [#type] => submit
            [#value] => Reset to defaults
            [#weight] => 50
        )
ChrisKennedy’s picture

ChrisKennedy commented

Ahhh ok I figured it out - I'm only partially crazy. I was accidentally running DRUPAL-5-1 rather than DRUPAL-5, and the buttons didn't get weights added until after 5.1 was released: http://drupal.org/node/111537

So the weights have only existed in a released version of Drupal since 5.2 came out two weeks ago.

anders.fajerson’s picture

anders.fajerson commented

That's a petty. But the fix is in Drupal 5.2 (I looked it up), wich is good. Can I commit this?

anders.fajerson’s picture

anders.fajerson commented

Missed your link - early morning.

ChrisKennedy’s picture

ChrisKennedy commented
StatusFileSize
new advpoll_default_1.patch12.89 KB

I implemented the settings migration - check it out and see if it looks okay. What was your reasoning behind moving the "reset vote" button out of a fieldset?

anders.fajerson’s picture

anders.fajerson commented
StatusFileSize
new advpoll_default_2.patch12.92 KB

1. I missed to rename "advpoll_default_electoral_list" to "advpoll_electoral_list" in a few places. Fixed that and updated the migration code to handle the renaming. It might have been cleaner/less code to rename it directly in the database but this works. Also fixed $type->name in the migration code, it should be $type->type.

2. Changed if (strpos($node_type, 'advpoll_') !== FALSE) to if ($form['module']['#value'] == 'advpoll'). I didn't know the module name was in there.

3. The reset button most have been some left-over from testing. Fixed that.

ChrisKennedy’s picture

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

Looks good, thanks. I don't think direct db changes would be cleaner though - it's better to use the drupal API (although Drupal could add a variable_rename() command).

anders.fajerson’s picture

anders.fajerson commented
Status: Reviewed & tested by the community » Fixed

Committed with updated documentation: http://drupal.org/cvs?commit=77113. Great job!

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)