Remove {masquerade} table and rely on session flag only

Agreed exactly on removal of the table. Also if someone will try to introduce anonymous functionality then D8 state() could be used for.

It seem that it works

Not sure I get how sessions could leak...
patch adds upgrade function to remove table. Suppose it's ok to commit

+++ b/masquerade.installundefined
@@ -6,42 +6,17 @@
+function masquerade_install() {
+  module_set_weight('masquerade', -10);

Yes this should be dropped
Suppose this is a remains of hook_init()

Commit collision

+++ b/lib/Drupal/masquerade/Tests/MasqueradeTest.phpundefined
@@ -107,6 +112,87 @@ class MasqueradeTest extends WebTestBase {
+        $old_session = isset($_SESSION) ? $_SESSION : NULL;
+        // Furthermore, if this test is executed on the command line, then
+        // Drupal denies to start a session. PHP throws a notice if the session
+        // is attempted to be started more than once.
+        // @see drupal_session_start()
...
+        // In any case, ensure that it is empty.
+        $_SESSION = array();

A kind of magic... but works!

This should be backported, probably we need a 7.x-2.x branch

It's so late but I created a patch to backport this to D7. The test passes in my local environment. I'd like someone to review this.

This backport needs to use php $_SESSION and the issue #2226531: Login hooks can't detect masqueraded user with $_SESSION['masquerading'] is (should be) also fixed with this patch.

As andypost and sun told, I expected this patch to be used for 7.x-2.x branch that doesn't support masquerading as Anonymous. Thank you.

I didn't fix the hook_help() message in the patch #20. I updated the patch #20 to update the hook_help() message following the andypost's comment on #2226531: Login hooks can't detect masqueraded user with $_SESSION['masquerading'].

PS: also if get rid of this property (+1 to that) we need to update hook_help() that mention about that

Re-roll

This seems to work very well and one less sql query on the page is a nice win for lack of anonymous masquerading which as the issue summary states is not needed.

Thanx a lot for this hard work, I'd like to get other maintainer opinion before starting new branch.

I bet we should remove this hook...

+++ b/masquerade.module
@@ -364,16 +315,11 @@ function _masquerade_user_load($username) {
 function masquerade_user_logout($account) {
-  if (!empty($account->masquerading)) {
+  if (!empty($_SESSION['masquerading'])) {
     global $user;
     cache_clear_all($user->uid, 'cache_menu', TRUE);
-    $real_user = user_load($user->masquerading);
+    $real_user = user_load($_SESSION['masquerading']);
     watchdog('masquerade', "User %user no longer masquerading as %masq_as.", array('%user' => $real_user->name, '%masq_as' => $user->name), WATCHDOG_INFO);
-
-    $query = db_delete('masquerade');
-    $query->condition('sid', session_id());
-    $query->condition('uid_as', $account->uid);
-    $query->execute();
   }
 }

This is only unclear hunk for me
I think there's no such cache in d7 anymore and there was issue to remove it.
Moreover I'm sure there should be unset() for session but on logout session is cleared by core.
The only reason for this hook was to clear session from table that we are removing

btw in D6 this table cache_menu has keys that starts from links:%

| links:primary-links:page-cid:admin/user/user:1                                                     |
| links:primary-links:page-cid:admin/user:1   

In D7

| admin_menu:1:z6l9VF-2m2L2MWkcZcMMf-ZgfstckxXgsKDEh6NMK2U:en                                                |
...
| links:main-menu:page:admin/config/regional/language:en:1:1                                                 |
| links:main-menu:page:admin/config/regional/language:fr:1:1                                                 |

An so on... let's remove this hunk before creating new branch

Thank you for updating and reviewing the patch!

Following andypost's feedback, I deleted the hook implementation masquerade_user_logout(). There was another call of cache_clear_all($user->uid, 'cache_menu', TRUE) and I removed it, too.

After applying this patch, I tested the function again in my environment and it looks to work well.

(Maybe, the points changed here are small and there's no need to move it from RTBC to NR again...?)

@hgoto Thanx a lot for clean-up! makes a lot of sense

Assigned to maintainer to get opinion about start new branch or keep in 7.x-1.x

Thanks, @andypost.

I'll update the issue summary a little to reflect the current status.

When saving a user form this came up. Fatal error: Call to undefined function masquerade_user_submit() inincludes/form.inc on line 1520

I'm guessing that was intended to be removed, if not LMK, but here's a patch without the callback that was deleted.

@joelpittet thanks for pointing the error. Surely it must be removed. I myself want to move this to RTBC but I'd like someone else to do it :)

Thanks @hgoto for confirming, yeah I don't see why this couldn't be RTBC. I'm using it in production.

RTBC and #28 again

This has been RTBC with remaining question from #28 whether to commit to 7.x-1.x-dev or start a new branch.

+1 to just commit to current branch.

Adding this to #3010095: Masquerade 7.x-1.0 stable release plan

I think committing to the current branch makes sense. There's no need for another branch when 7.x-1.0 hasn't been released yet. This issue is six years old, so I'd say just merge it.

@andypost Are there any remaining objections that prevent the patch from getting committed?

I see no, except proper schedule of release

I've no d7 dites anymore to test it

I've no d7 dites anymore to test it

Well, there's always simplytest.me as well as the included web tests. Are you planning to hold out on committing all planned issues until you prep the release?

All you have to do is setup a vanilla D7 instance and enable masquerade. It's not that hard.

It's not about testing functionality (there's tests for that) but about upgrade path!

There's 50k existing installs which may need to drop the table from their database and nobody knows how many custom code is written around this table, that's why I'm waiting other maintainer's opinion

I guess it's possible that someone might have created a block or something that should show a list of all masquerading users. Removing the table removes any possibility of that, right, since there would be no way to track that?

Yes, moreover right after upgrade all sessions of users which are masquerading before upgrade will be in strange state

Sounds to me like the issue should be left out of the initial 1.0 release then (given that there's already an RC), and committed as part of a 1.1 or even 2.0 release. Perhaps we should also set this issue back to Needs Review in order to evaluate which remaining cases have to be considered?

No reason to NW but give a time to use the new patch to existing sites

Breaking schema changes should be made in a separate branch in my opinion, though I doubt many sites would directly access the table like that.

I would like to see this patch committed along with other patches as described in #3010095: Masquerade 7.x-1.0 stable release plan where I identified that the order in which the patches are applied is important. Maybe others would be willing to review that list and maybe the whole bunch can be committed for the 7.x-1.0 release before the New Year. Thanks.

Here is an example query I have. Would there be a way to do this without the masquerade table?

This gets the UID of the original currently masquerading user.

global $user;
return db_query("SELECT m.uid_from FROM {masquerade} m WHERE m.sid = :sid AND m.uid_as = :uid_as ", array(
    ':sid' => session_id(),
    ':uid_as' => $user->uid,
  ))->fetchField();

For example, if you want to check for access based on the original access of the masquerading user, you need to be able to determine who the original user is.

Edit: Actually, I noticed that this query is the same as the one in masquerade_init(). I think it's the same to just use this:

return $_SESSION['masquerading'] ?? NULL;

Thanks everyone for working on this. In an attempt to help the maintainers, I am going through some of the Drupal 7 issues, and closing them, since Drupal 7 is EOL.

Maintainers should grant credit for the great work to the relevant users, even if the patch did not get committed to Drupal 7: https://www.drupal.org/docs/develop/issues/issue-procedures-and-etiquett....

Since Drupal 7 is EOL and this seems to have been committed for the current Drupal 11 version ("As for D8, the patch in #19 was committed and currently D7 is targeted in this issue.") we can probably close this issue, but feel free to re-open if this is still relevant.