Menu and Block Quick Switch User Settings Don't Work for 'Anonymous'
callison - October 21, 2009 - 04:23
| Project: | Masquerade |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
This is related to #568750: Anonymous switching doesn't work, but I think it should be considered a separate issue.
I found a bug where the anonymous users could be entered into the fields on the masquerade settings page (admin/settings/masquerade) but would not cause the menu or quick switch links to be activated. I have attached a patch fixing this bug. I would love to get some feedback and review on this patch and anything that could be done to improve it. Thanks.
| Attachment | Size |
|---|---|
| masquerade_quick_switch_anon.patch | 5.73 KB |
#1
Oops. Accidentally renamed a function. Updated patch.
#2
Thanks for the patch. It looks like the patch needs to be re-rolled against the latest -dev release. For the most part, the method looks good. Comments follow:
+++ masquerade.module 21 Oct 2009 04:14:56 -0000@@ -187,6 +187,9 @@
+ $u->name = variable_get('anonymous', t('Anonymous'));
+ }
The indentation needs to be fixed here.
+++ masquerade.module 21 Oct 2009 04:14:56 -0000@@ -241,12 +245,29 @@
+ * Load users to accomodate for the anonymous 'user'
This line needs to end with a period. As well, parameters and return values need to be documented.
+++ masquerade.module 21 Oct 2009 04:14:56 -0000@@ -241,12 +245,29 @@
+function _masquerade_user_load($username) {
The indentation needs to be fixed in this function - see http://drupal.org/coding-standards
+++ masquerade.module 21 Oct 2009 04:14:56 -0000@@ -241,12 +245,29 @@
+ if ($username == '') {
+ return true;
+ }
Why do we return true here?
+++ masquerade.module 21 Oct 2009 04:14:56 -0000@@ -241,12 +245,29 @@
+ $user = user_load(array('name' => ''));
Would it make more sense to do a user_load on uid=0?
+++ masquerade.module 21 Oct 2009 04:14:56 -0000@@ -387,9 +414,10 @@
+ if( $switch_user == 0 ) {
+ $user_name->name = variable_get('anonymous', t('Anonymous'));
+ }
Fix indentation.
+++ masquerade.module 21 Oct 2009 04:14:56 -0000@@ -502,6 +530,10 @@
+ //This will add anonymous to the list, but not sorted
Fix comment style.
+++ masquerade.module 21 Oct 2009 04:14:56 -0000@@ -594,4 +626,4 @@
-}
+}
Not sure why there is a change here.
I'm on crack. Are you, too?
#3
Thanks for your comments, deviantintegral.
Sorry for all the indentation issues - I used an editor with auto-indentation. Should be fixed now.
The one that returns true does so because in masquerade_admin_settings_validate it will set a form error if the function returns false. This didn't seem like a very intuitive way to go about this, but it works. I'll put a comment there to try and avoid confusion.
#4
Couple of things changed from my previous post:
Firstly, I had made an error with the autocomplete. This patch is correct and autocomplete works great.
Secondly, I completely removed the function _masquerade_test_user() because it is virtually useless with the masquerade_user_load() function. To mimic the functionality, you simply call _masquerade_user_load(variable_get('masquerade_test_user', '')) so it was being unnecessarily repetitive.
#5
Sorry to keep doing this. I need to test these better before posting them. This patch fixes a small problem in the last one.
#6
For some reason I had to manually apply the changes in masquerade_menu(). Also, on applying the patch, I got:
I've fixed both of these errors as well as fixing a similar issue in masquerade_block_0(). As well, I've modified _masquerade_user_load() to return FALSE, just like user_load(). When wrapping a core function, it's best to keep the return values as similar as possible. Let me know how it works / reviews for you.
#7
Works great for me! Thanks for your fixes.
#8
Updated status.
#9
FYI, the proper status would be Reviewed and Tested by the Community. "to be ported" is if the patch was to be backported to a previous Drupal version.
I've committed this patch.
#10
Automatically closed -- issue fixed for 2 weeks with no activity.