Closed (fixed)
Project:
Webform
Version:
7.x-3.x-dev
Component:
Code
Priority:
Minor
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
2 May 2011 at 20:18 UTC
Updated:
17 Jul 2011 at 21:21 UTC
Jump to comment: Most recent file
On saving the settings of the webform admin settings form, the webform module creates the variables 'webform_disabled_components' and 'webform_node_types' via its own submit handling. However it doesn't unset these values in $form_state, so subsequently the system_settings_form_submit() function will happily save these values again under their form name, being 'components' and 'node_types'.
Better would be to rename the form elements to webform_... and have webform_admin_settings_submit() do something like;
$form_state['values']['webform_node_types'] = array_keys(array_filter($form_state['values']['webform_node_types']));
The variable_set is then no longer necessary as it will be done by the system_settings_form_submit() function.
Notes:
- there is an array_filter setting for this kind of operation, but it will operate on ALL array values in the form,so I'm not sure that can be used here.
- the inversion on components (storing disabled instead of enabled components) wil need some extra work
- It would be nice to consider a hook_update_N to remove the 'node_types' and 'components' variables.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | webform-unused-variables-d7-followup.patch | 354 bytes | David_Rothstein |
| #1 | webform_unused_variables-d6.patch | 2.2 KB | quicksketch |
| #1 | webform_unused_variables-d7.patch | 2.07 KB | quicksketch |
Comments
Comment #1
quicksketchThanks for this nice catch. I've fixed all these issues with the attached patch, update function included.
Comment #2
quicksketchComment #4
David_Rothstein commentedPretty sure this update function will never run in Drupal 7, since it uses the wrong numbering.
Fixed in the attached patch.
Comment #5
fietserwinDrupal only checks if the number is higher than what has been run so far and does not check for main version numbers or whatsoever (unlike what the documentation suggests).
So, if webform numbers all updates in its D6 version 63xx and the same updates in the D7 version 73xx than you're right this being a problem. But then there is a fa larger problem: updates being run again when converting from D6 to D7. Example 6318 and 7301 are the same, thus when converting from 6.x-3.11 to 7.x-3.11, the code in 7301 will be run again as it has already been run in 6318. Probably not a problem for this specific example (idempotent and non-failing), but it may lead to failures and half completed updates, etc.
So better to decouple the numbering form themain versions and revert to a schema version number specific for your schema.
Comment #6
quicksketchDavid's patch still fixes the situation where a user had been running a Drupal 7 version of Webform for a long time and then were to update to the next version of Webform (what will be 3.12). The schema version of existing D7 installations will be 7311, so the next update needs to be higher in order to update existing D7 sites.
In any case, it sounds like we all agree we need to fix the number, it was an accident on my part. Committed #4. Thanks!