As a follow-up to #886878: add option to show registration form rather than login on access denied pages, it would be nice if the unified login could be shown on 403.

I thought adding yet another checkbox to the admin UI was too much, and I think this is the expected behavior.
If you want another checkbox, I can do that too.

CommentFileSizeAuthor
#3 logintoboggan-unified_on_403-3.patch3.63 KBhunmonk
#2 logintoboggan-unified_on_403-2.patch1.3 KBhunmonk
logintoboggan-unified_on_403.patch537 bytestim.plunkett

Comments

jyee’s picture

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

This patch works and is what I had expected from normal functionality. If you select unified login for the normal login/register, that same setting should apply to the 403/access denied page as well.

hunmonk’s picture

hunmonk commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new logintoboggan-unified_on_403-2.patch1.3 KB
  • the patch doesn't handle the page setup for access denied in a consistent manner between the two forms
  • i'm also liking the idea of creating a helper function that gives you the right login form based on current settings, instead of littering the code with conditional checks.

so, try the attached patch. untested, but i think it will accomplish the goal and fix those issues noted above.

hunmonk’s picture

hunmonk commented
StatusFileSize
new logintoboggan-unified_on_403-3.patch3.63 KB

further refinement, broke the generation of the unified login form out into a separate function from the full page generation. this cleans up the usage of the unified login form nicely.

light testing seems to show everything working, but i would like confirmation from somebody else here before i commit.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Needs work

My only nitpick:

+++ b/logintoboggan.moduleundefined
@@ -888,11 +872,61 @@ function _logintoboggan_toggleboggan ($form) {
+function logintoboggan_get_authentication_form($active_form = 'login') {
+  $output = '';
+  if (variable_get('logintoboggan_unified_login', 0)) {
+    $output = logintoboggan_unified_login_form($active_form);
+  }
+  elseif ($active_form == 'login') {
+    $output = drupal_get_form('user_login');
+  }
+  elseif ($active_form == 'register') {
+    $output = drupal_get_form('user_register_form');
+  }
+  return $output;

Why not just return instead of instantiating a variable? And then they could be if() not elseif()

hunmonk’s picture

hunmonk commented
Status: Needs work » Needs review

the caller is expecting a string, this is a clean way to always return a string. i also find it easier to debug code when you put something in a variable first before returning it (easier to toss in a var_dump()).

hunmonk’s picture

hunmonk commented
Status: Needs review » Fixed

did a bit more testing, and i think this is good to go. committed to 7.x-1.x.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.