Some minor code cleanup needed to pass coder

Also fixed tests to use token, we needs extended coverage:
1) block
2) API functions
3) admin UI

CommentFileSizeAuthor
#6 1835954-masquerade-6.patch15.7 KBandypost
#5 1835954-masquerade-5.patch16.19 KBandypost
#5 1835954-interdiff-5.txt9.49 KBandypost
#4 1835954-masquerade-4.patch13.69 KBandypost
#4 1835954-masquerade-4-1.patch13.53 KBandypost
#4 1835954-interdiff-4.txt4.57 KBandypost
#2 1835954-masquerade-2.patch10.55 KBandypost
#1 1835954-masquerade-1.patch9.32 KBandypost

Comments

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 1835954-masquerade-1.patch9.32 KB

Initial patch, fixed code-style and started tests (still not passes switch 404)

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 1835954-masquerade-2.patch10.55 KB

A bit extended test. I can't find a proper way to build a token

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Some good clean-ups here. Though do you think we could move forward with #1836516: Port Masquerade to D8 and rewrite + simplify it into a new 2.x series first?

+++ b/masquerade.module
@@ -252,6 +252,7 @@ function masquerade_menu_access($type, $uid = NULL) {
 function masquerade_admin_settings() {
+  drupal_set_message(user_access('masquerade as user') ? 'Allowed' : 'Disallowed');

?

+++ b/masquerade.module
@@ -359,14 +360,15 @@ function _masquerade_user_load($username) {
+    // @TODO Seems this useless.
     cache_clear_all($user->uid, 'cache_menu', TRUE);

Yep, also noticed those. Seem to date back to D5.

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 1835954-interdiff-4.txt4.57 KB
new 1835954-masquerade-4-1.patch13.53 KB
new 1835954-masquerade-4.patch13.69 KB

@sun thanx for review, this is a dubuggin staff - I'm trying to debug the session that should be fixed in #1555862: DrupalWebTestCase::drupalGetToken() does not add hash salt

patch includes additional cleanups from #1836516: Port Masquerade to D8 and rewrite + simplify it into a new 2.x series

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 1835954-interdiff-5.txt9.49 KB
new 1835954-masquerade-5.patch16.19 KB

The final patch I'm going to commit to D7

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 1835954-masquerade-6.patch15.7 KB

Another rount to pass tests

Status: Needs review » Needs work

The last submitted patch, 1835954-masquerade-6.patch, failed testing.

ressa’s picture

ressa
he/him
commented
Issue summary: View changes
Status: Needs work » Closed (outdated)

Thanks 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: https://www.drupal.org/docs/develop/issues/issue-procedures-and-etiquett....

Since Drupal 7 is EOL, we can probably close this issue, but feel free to re-open if this is still relevant, and we should consider adding this to the Drupal 11 version?

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.