Part of #1971384: [META] Convert page callbacks to controllers

For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.

Files: 
CommentFileSizeAuthor
#14 1987894-user-logout-route-14.patch6.75 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 56,507 pass(es).
[ View ]
#10 1987894-user-logout-route-10.patch6.88 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 55,571 pass(es).
[ View ]
#10 interdiff.txt2.34 KBParisLiakos
#9 1987894-user-logout-route-9.patch6.4 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 55,757 pass(es), 35 fail(s), and 4 exception(s).
[ View ]
#9 interdiff.txt1.71 KBParisLiakos
#6 1987894-user-logout-route-6.patch7.33 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 55,572 pass(es), 39 fail(s), and 1 exception(s).
[ View ]
#6 interdiff.txt1.26 KBParisLiakos
#5 1987894-user-logout-route-5.patch7.33 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 55,960 pass(es), 39 fail(s), and 1 exception(s).
[ View ]
#5 interdiff.txt1001 bytesParisLiakos
#4 1987894-user-logout-route-4.patch7.04 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 55,844 pass(es), 39 fail(s), and 1 exception(s).
[ View ]
#4 interdiff.txt1.4 KBParisLiakos
#3 1987894-user-logout-route-3.patch5.78 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 56,196 pass(es), 39 fail(s), and 1 exception(s).
[ View ]
#1 1987894-user-logout-route-1.patch3.86 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 55,532 pass(es), 0 fail(s), and 418 exception(s).
[ View ]

Comments

vijaycs85’s picture

Status:Active» Needs review
StatusFileSize
new3.86 KB
FAILED: [[SimpleTest]]: [MySQL] 55,532 pass(es), 0 fail(s), and 418 exception(s).
[ View ]

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
StatusFileSize
new5.78 KB
FAILED: [[SimpleTest]]: [MySQL] 56,196 pass(es), 39 fail(s), and 1 exception(s).
[ View ]
ParisLiakos’s picture

StatusFileSize
new1.4 KB
new7.04 KB
FAILED: [[SimpleTest]]: [MySQL] 55,844 pass(es), 39 fail(s), and 1 exception(s).
[ View ]

also..

ParisLiakos’s picture

StatusFileSize
new1001 bytes
new7.33 KB
FAILED: [[SimpleTest]]: [MySQL] 55,960 pass(es), 39 fail(s), and 1 exception(s).
[ View ]

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

ParisLiakos’s picture

StatusFileSize
new1.26 KB
new7.33 KB
FAILED: [[SimpleTest]]: [MySQL] 55,572 pass(es), 39 fail(s), and 1 exception(s).
[ View ]

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

StatusFileSize
new1.71 KB
new6.4 KB
FAILED: [[SimpleTest]]: [MySQL] 55,757 pass(es), 35 fail(s), and 4 exception(s).
[ View ]

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

StatusFileSize
new2.34 KB
new6.88 KB
PASSED: [[SimpleTest]]: [MySQL] 55,571 pass(es).
[ View ]

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

StatusFileSize
new6.75 KB
PASSED: [[SimpleTest]]: [MySQL] 56,507 pass(es).
[ View ]

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.