backdrop-contrib / honeypot

Backdrop port of Drupal module. Uses both the honeypot and timestamp methods of deterring spam bots from completing forms on your site.
GNU General Public License v2.0
3 stars 1 forks source link

The "unprotected_forms" variable not upgraded #1

Closed quicksketch closed 9 years ago

quicksketch commented 9 years ago

Upon upgrading my Drupal 7 site to Backdrop, I received the following error after running update.php and visiting my site:

Warning: in_array() expects parameter 2 to be array, null given in honeypot_form_alter() (line 79 of /var/www/modules/contrib/honeypot/honeypot.module).

This line is:

  $unprotected_forms = $config->get('unprotected_forms');

Looking at my honeypot.settings.json file, I can see that this key does not exist. It looks like we should add the defaults in honeypot_install_1000().

Thanks for porting this module to Backdrop! Other than this minor upgrade issue, it works great!

quicksketch commented 9 years ago

Hm, a bunch more notices when visiting the Honeypot settings form. This looks like it would happen after creating a new node type as well:

Notice: Undefined index: honeypot_form_contact_site_form in honeypot_admin_form() (line 101 of /Users/nate/Sites/backdropcms.org/www/modules/contrib/honeypot/honeypot.admin.inc). Notice: Undefined index: honeypot_form_contact_personal_form in honeypot_admin_form() (line 107 of /Users/nate/Sites/backdropcms.org/www/modules/contrib/honeypot/honeypot.admin.inc). Notice: Undefined index: honeypot_form_sa_node_form in honeypot_admin_form() (line 131 of /Users/nate/Sites/backdropcms.org/www/modules/contrib/honeypot/honeypot.admin.inc).

herbdool commented 9 years ago

Thanks for the bug report! I think they're two different bugs. In the first bug, I got eager to move things to config so I had actually moved the hard-coded array from honeypot module and put into config. I might have assumed that even on an upgrade that the json file would be used but I guess it all has to go in the update hook in order to show up. Anyway I'll fix that one and then move on to the second issue.

herbdool commented 9 years ago

I've added unprotected_forms to the update hook: 01ec5aac7df240827ca8714dcf40f97e63154e3d

herbdool commented 9 years ago

I believe I've fixed the issue brought up in your second comment c8de6869cecf43a72baa541654f3d794a1a526a9

It now keeps the keys that aren't being exposed in the $form_settings array. Plus it checks if an array key exists for node forms so won't throw an error if a new content type is created.

I didn't want to change too much from the Drupal 7 version but I find my approach a bit clunky. Couldn't think of a way to not save honeypotform along with rest of keys. Unless I didn't bother put them in an array. At any rate, it works.

This might be a good thing to add to the documentation: suggestions on how to deal with settings where we don't know beforehand what's going to be saved.