Updated: Comment #N

Problem/Motivation

When placing two signup forms, e.g. a rendered entity reference field and a block, only the first form can be submitted as they have the same form id.

Proposed resolution

Implement hook_forms() to have dynamic form ID's.

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

And here is an untested patch.

Berdir’s picture

Status: Active » Needs review
miro_dietiker’s picture

Issue tags: +Needs tests
+++ b/salsa_signup_page/includes/salsa_signup_page.entity.inc
@@ -28,7 +28,7 @@ class SalsaSignUpPage extends SalsaEntity {
+    $content['signup_page_form'] = drupal_get_form('salsa_signup_page_form_' . $this->identifier(), $this);

+++ b/salsa_signup_page/salsa_signup_page.module
@@ -70,6 +70,21 @@ function salsa_signup_page_block_view($delta = '') {
+  if (strpos($form_id, 'salsa_signup_page_form_') === 0) {

Functionality wise fully fine. But please add a comment why this hook_forms was required. I know it almost is be self-explaining. And please answer the question if there are other form types that have the same problem.

Salsa offers the possibility to show forms through a block. This combination could result on virtually all salsa forms to have multiple instances on the same page due to accidental confiuration. Although not really recommended, things should not break in this case.

And since it was such a funny bug, i guess we should add a specific test?

LukyLuke_ch’s picture

There is also a small change needed in the salsa_signup_page_mollom_form_info() (salsa_signup_page.module) hook:

@@ -109,13 +122,10 @@ function salsa_signup_page_mollom_form_list() {
  * Implements hook_mollom_form_info().
  */
 function salsa_signup_page_mollom_form_info($form_id) {
-  switch ($form_id) {
-    case 'salsa_signup_page_form':
-      $form_info = array(
-        'mode' => MOLLOM_MODE_CAPTCHA,
-      );
-      return $form_info;
-      break;
+  if (strpos($form_id, 'salsa_signup_page_form') === 0) {
+    return array(
+      'mode' => MOLLOM_MODE_CAPTCHA,
+    );
   }
 }
 

Additionally, to populate and being able to configure the pages differently, this can be also added in salsa_signup_page_mollom_form_list() (salsa_signup_page.module)

@@ -102,6 +115,18 @@ function salsa_signup_page_mollom_form_list() {
   $forms['salsa_signup_page_form'] = array(
     'title' => t('Salsa signup page form'),
   );
+
+  // Populate all signup pages to configure them separately.
+  $pages = entity_load('salsa_signup_page');
+  if (!empty($pages)) {
+    foreach ($pages as $page) {
+      $signup_pages[$page->signup_page_KEY] = $page->Reference_Name;
+      $forms['salsa_signup_page_form_' . $page->identifier()] = array(
+        'title' => t('Salsa signup page form: ' . $page->identifier()),
+      );
+    }
+  }
+
   return $forms;
 }
 
LukyLuke_ch’s picture

Here a complete patch for the above.
See also https://drupal.org/comment/8479019#comment-8479019 for the appropriate mollom patch which is needed here to make this properly working with mollom.

miro_dietiker’s picture

As long as this is not in at Mollom, we would kill existing sites using mollom (required reconfiguration).

Let's wait till this is in - and then commit.
For now this remains a patch for the specific project that hits this case.