Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

Status: Active » Needs review
FileSize
3.86 KB

Initial patch...

Status: Needs review » Needs work

The last submitted patch, 1987894-user-logout-route-1.patch, failed testing.

ParisLiakos’s picture

Assigned: Unassigned » ParisLiakos
Status: Needs work » Needs review
FileSize
5.78 KB
ParisLiakos’s picture

also..

ParisLiakos’s picture

meh..this wont fire hooks..something a bit uglier then

ParisLiakos’s picture

humpff, wrong namespace

Crell’s picture

+++ b/core/modules/user/user.module
@@ -906,11 +907,9 @@ function user_menu() {
@@ -1056,8 +1055,7 @@ function user_menu_site_status_alter(&$menu_site_status, $path) {

@@ -1056,8 +1055,7 @@ function user_menu_site_status_alter(&$menu_site_status, $path) {
   if ($menu_site_status == MENU_SITE_OFFLINE) {
     // If the site is offline, log out unprivileged users.
     if (user_is_logged_in() && !user_access('access site in maintenance mode')) {
-      module_load_include('pages.inc', 'user', 'user');
-      user_logout();
+      UserController::create(Drupal::getContainer())->logout()->send();
     }

This seems very wrong... You should never send() the response yourself.

ParisLiakos’s picture

i know its ugly like i said in #5 but i am open to alternatives!

hmm maybe i should keep user_logout() and fix this in another issue

ParisLiakos’s picture

there, lets keep this issue moving, cause this access check here is needed to several other conversions and fixing openid for drupal_exit removal

ParisLiakos’s picture

WebTestBase--

Status: Needs review » Needs work
Issue tags: -WSCCI-conversion

The last submitted patch, 1987894-user-logout-route-10.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI-conversion

cant reproduce locally
#10: 1987894-user-logout-route-10.patch queued for re-testing.

dawehner’s picture

+++ b/core/modules/user/lib/Drupal/user/Controller/UserController.phpundefined
@@ -0,0 +1,69 @@
+use Drupal\user\Plugin\Core\Entity\User;

Do we really need this statement? I can't see any usage.

+++ b/core/modules/user/lib/Drupal/user/Access/LoginStatusCheck.phpundefined
index 7e294f5..46745ec 100644
--- a/core/modules/user/lib/Drupal/user/UserAutocompleteController.php

--- a/core/modules/user/lib/Drupal/user/UserAutocompleteController.php
+++ b/core/modules/user/lib/Drupal/user/Controller/UserAutocompleteController.phpundefined

Renaming an existing file feels a bit unrelated though this could make sense.

ParisLiakos’s picture

thanks for the review!

this conflicted with roles commit, so no interdiff, sorry.

i removed the unneeded use statement

Renaming an existing file feels a bit unrelated though this could make sense.

yes i moved cause i just created the directory, i dont think we should have a seperate issue for moving this

Crell’s picture

Why exactly aren't we removing user_logout(), the function?

ParisLiakos’s picture

because it is needed by user_menu_site_status_alter
it does something hacky, and imo should be converted to an event listener. you cant redirect like that, but fixing this should happen in the RedirectResponse issue

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Great, thank you.

ParisLiakos’s picture

alexpott’s picture

Committed e21c1a1 and pushed to 8.x. Thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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