Provide better redirect support. [#1613462]

I know that redirects were recently implemented in 7.x-4.x, however this module was recently implemented in a site which took the functionality of login redirects away from existing logic. At first I thought I could implement either some hook overrides or even try to specify a path that I could then do additional logic. I realized though, that this really wasn't the best approach. Here is a patch that I feel should be implemented in this module or something of the likes. It helps standardize redirect options for each of the three forms.

Because this changes the UI and the functionality of redirects so drastically, I also included an update to rename existing variables to something that was more suited for their purpose.

I feel that this is a good direct to start with, please let me know if I need to refactor my patch as I am ultimately unfamiliar with your module and may have missed something! Great module btw!

Thanks,
Mark

Comment	File	Size	Author
#4	ajax_register-redirect-support.patch	13.04 KB	markhalliwell
#2	ajax_register-redirect-support.patch	15.95 KB	markhalliwell
	ajax_register-redirect-support.patch	11.56 KB	markhalliwell

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Spleshka

he/him

English

UAE

CreditAttribution: Spleshka commented 2 June 2012 at 10:39

Status:

Active

» Needs work

Hi! Thanks a lot for your patch! So appreciate! Here is some comments:

+++ b/ajax_register.admin.inc
@@ -76,20 +76,101 @@ function ajax_register_admin_form($form, $form_state) {
+  $redirect_behaviors = array(
+    'default' => t('Default'),
+    'none' => t('No Redirect'),
+    'refresh' => t('Refresh'),
+    'custom' => t('Custom'),
+  );

You can't provide "No redirect" for user login. Page should be reloaded or redirected anyway.

+++ b/ajax_register.admin.inc
@@ -76,20 +76,101 @@ function ajax_register_admin_form($form, $form_state) {
+  ajax_register_add_redirect_settings($form['ajax_register_forms']['login'], $form_state, 'login');
+  ¶
+  // Register Form Settings

A lot of empty spaces in patch.

+++ b/ajax_register.admin.inc
@@ -76,20 +76,101 @@ function ajax_register_admin_form($form, $form_state) {
+  // Provide descriptions for redirect behaviors.
+  $redirect_description = '<ul>';
+  $redirect_descriptions = array(
+    t('Default') => t('After successful submission of the form, the redirect path is handled by Drupal and will redirect the user to whatever the !form_state_redirect variable is set to. NOTE: This option may be required if there are additional modules or features that handle the redirect logic.', array(
+      '!form_state_redirect' => '$form_state[\'redirect\']',
+    )),
+    t('No Redirect') => t('After successful submission of the form, the modal window will simply close and not redirect the user to any location.'),
+    t('Refresh') => t('After successful submission of the form, the page will be refreshed.'),
+    t('Custom') => t('After successful submission of the form, users will be redirected to this manually set custom URL.'),
+  );
+  ¶
+  foreach ($redirect_descriptions as $title => $description) {
+    $redirect_description .= '<li><strong>' . $title . '</strong> - ' . $description . '</li>';
+  }
+  $redirect_description .= '</ul>';

1. It is better to use theme('item_list') instead of manual <ul><li>

2. Redirect path may be stored not only in $form_state['redirect'] but in $form['#redirect']

3. Same as before: a lot of empty spaces.

+++ b/ajax_register.admin.inc
@@ -76,20 +76,101 @@ function ajax_register_admin_form($form, $form_state) {
+  // Default value

After comment should be added dot (ccording to Drupal coding standarts).

+++ b/ajax_register.admin.inc
@@ -76,20 +76,101 @@ function ajax_register_admin_form($form, $form_state) {
+  $form_type = isset($form_state['triggering_element']['#form_type']) ? $form_state['triggering_element']['#form_type'] : FALSE;
+  if ($form_type) {
+    return $form['ajax_register_forms'][$form_type]['redirect'];
+  }
+}
\ No newline at end of file

Please, provide new line at the end of file.

+++ b/ajax_register.install
@@ -33,3 +33,40 @@ function ajax_register_update_7406() {
+  $login_behavior = $register_behavior = 'refresh';

This line should be separated into two lines.

+++ b/ajax_register.install
@@ -33,3 +33,40 @@ function ajax_register_update_7406() {
+    $register_redirect_url = '';

This line in unnecessary at all.

+++ b/ajax_register.pages.inc
@@ -44,45 +44,57 @@ function ajax_register_page_callback($form_type, $js) {
-
+      ¶
       // Include additinal ajax commands.
       ctools_include('ajax');
-
-      if ($form_type == 'password') {
-        $commands[] = ctools_modal_command_display(t('Successful password request'), theme('status_messages'));
+      ¶

Empty spaces should be removed from the patch.

+++ b/ajax_register.pages.inc
@@ -44,45 +44,57 @@ function ajax_register_page_callback($form_type, $js) {
+      if ($redirect_behavior == 'default' && isset($form_state['redirect']) && !empty($form_state['redirect'])) {
+        $redirect_url = $form_state['redirect'];
       }

isset($form_state['redirect']) is unnecessary here.

+++ b/ajax_register.pages.inc
@@ -44,45 +44,57 @@ function ajax_register_page_callback($form_type, $js) {
+      $title = t('Successful submission');

This line is unnecessary.

P.s. Please, set status Needs review when providing a patch.

Comment #2

markhalliwell

English

CreditAttribution: markhalliwell commented 2 June 2012 at 17:50

Status:

Needs work

» Needs review

File	Size
ajax_register-redirect-support.patch	15.95 KB

I could have sworn I put it as Needs review, guess not, my apologies.

A lot of empty spaces in patch.

To be honest, I usually don't include any spaces at all (as you can see in the update I provided). But in the admin.inc, I was following existing format. I've been scolded about stuff like this before because I didn't follow existing format, so I just used existing line spacings in the previous patch. I completely agree with you though, they should be taken out.... so I went ahead and removed the ones above in the code I didn't touch to keep conformity.

One thing I didn't do was run the patch through coder. I did this time and fixed a few things that it found as well. I recently came across another issue where I did something similar and they told me to create a new issue because it wasn't related. So I am willing to do what ever you want as you help maintain the module :)

Redirect path may be stored not only in $form_state['redirect'] but in $form['#redirect']

$form['#redirect'] was used in D6, it was taken out of D7. See: 7: drupal_redirect_form() and 6: drupal_redirect_form()

+++ b/ajax_register.install
@@ -33,3 +33,40 @@ function ajax_register_update_7406() {
+  elseif ($register_redirect_url == '<noredirect>') {
+    $register_behavior = 'none';
+    $register_redirect_url = '';
+  }

This line in unnecessary at all.

Actually this line IS necessary. It matches the <noredirect> in the URL variable, sets the behavior variable to 'none' and then removes <noredirect> from the URL variable because it is no longer needed in that variable. Below, you'll see that I only set the new variable if the URL is not empty. This way, it cleans up which variables are actually set.

Summary:

I made the rest of the suggested changes :)

Comment #3

Spleshka

he/him

English

UAE

CreditAttribution: Spleshka commented 4 June 2012 at 09:04

Status:

Needs review

» Needs work

Thanks a lot, now it is much more better! I am very pleased that you are interested in this module. But patch still needs to be processed more carefully:

+++ b/ajax_register.admin.inc
@@ -9,12 +9,10 @@
 function ajax_register_admin_form($form, $form_state) {
-
   $form['ajax_register_modal'] = array(
     '#type' => 'fieldset',
     '#title' => t('Modal window settings'),
   );
-
   $form['ajax_register_modal']['ajax_register_modal_width'] = array(
     '#type' => 'textfield',
     '#title' => t('Modal window width'),
@@ -22,7 +20,6 @@ function ajax_register_admin_form($form, $form_state) {

@@ -22,7 +20,6 @@ function ajax_register_admin_form($form, $form_state) {
     '#field_suffix' => 'px',
     '#default_value' => variable_get('ajax_register_modal_width', 550),
   );
-

I guess you shouldn't remove empty lines between form and menu elements. It makes code more difficult to read.

+++ b/ajax_register.admin.inc
@@ -58,38 +53,118 @@ function ajax_register_admin_form($form, $form_state) {
+  $form['redirect'] = array(
+    '#prefix' => '<div id="ajax-register-forms-' . $form_type . '-redirect">', ¶
+    '#suffix' => '</div>',
+  );
+  // Determine the default value for the dropdown.
+  $selected = variable_get('ajax_register_' . $form_type . '_redirect_behavior', 'default');
+  if (isset($form_state['values']['ajax_register_' . $form_type . '_redirect_behavior'])) {
+    $selected = $form_state['values']['ajax_register_' . $form_type . '_redirect_behavior'];
+  }
+  $form['redirect']['ajax_register_' . $form_type . '_redirect_behavior'] = array(
+    '#form_type' => $form_type,
+    '#type' => 'select',
+    '#title' => 'Redirect Behavior',
+    '#description' => $redirect_description,
+    '#options' => $redirect_behaviors, ¶
+    '#default_value' => $selected,
+    '#ajax' => array(
+      'callback' => 'ajax_register_admin_form_redirect_callback', ¶
+      'wrapper' => 'ajax-register-forms-' . $form_type . '-redirect',
+    ),
+  );

I see empty space here :) Actualy, I found three extra spaces in the code. I know that it is not so important, but if you will make a new patch, please remove them.

+++ b/ajax_register.pages.inc
@@ -41,51 +36,55 @@ function ajax_register_page_callback($form_type, $js) {
+      if ($redirect_behavior == 'default' && !empty($form_state['redirect'])) {
+        $redirect_url = $form_state['redirect'];
       }

Form could be redirected by $form['#action'] as well. $form_state['redirect'] will be empty in this case.

+++ b/ajax_register.pages.inc
@@ -41,51 +36,55 @@ function ajax_register_page_callback($form_type, $js) {
+          $message = 'Login was successful.';
+          if ($redirect_behavior == 'refresh') {
+            $message .= ' Page will now be reloaded.';
+          }
+          elseif ($redirect_behavior == 'default' || $redirect_behavior == 'custom') {
+            $message .= ' Page will now be redirected.';
+          }
+          drupal_set_message(t('%message', array('%message' => $message)));

Message is not translatable. In your case message will be pasted "As Is". Here is correct code:
drupal_set_message(t($message));

Comment #4

markhalliwell

English

CreditAttribution: markhalliwell commented 4 June 2012 at 14:58

Status:

Needs work

» Needs review

File	Size
ajax_register-redirect-support.patch	13.04 KB

I guess you shouldn't remove empty lines between form and menu elements. It makes code more difficult to read.

Please see my above comment about why I did what I did. I'll put them back, but this is the last patch I'll submit about spacing issues. The patch is solid and works beautifully, cosmetic changes can be made by you after if you so desire.

Form could be redirected by $form['#action'] as well. $form_state['redirect'] will be empty in this case.

You're using ctools_modal_form_wrapper() to alter the $form_state via reference. There isn't even a $form to check for $form['#action']. What you are asking for isn't possible given the current implementation.

drupal_set_message(t('%message', array('%message' => $message)));

I had changed this from the original patch because Coder gave me a strict error about this line. I was tweaking it until it went away, in all honesty, I didn't even check to see if it was that functional. Changed it back and added a check_plain() call to satisfy coder.

So just to re-mention it, this is the last patch I'm willing to make. Aside from the small bugs I just fixed, there seems to be just nit picking going on now. If you feel there should be line spacing changes here or there, please make them yourself.