I honestly have no idea what this hook is even for, other than this implementation:

http://api.drupal.org/api/drupal/core!modules!user!user.module/function/...

But that should get ripped out and put into the MaintenanceModeSubscriber, which (I think) makes more sense than keeping it separate anyway.

The other implementations of this hook should turn into something more sensible, too.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ParisLiakos’s picture

But that should get ripped out and put into the MaintenanceModeSubscriber

or an equivalent of user's module subscriber since it contains user module specific logic

dawehner’s picture

Assigned: Unassigned » dawehner

Trying to work on that.

dawehner’s picture

After some research it comes clear that there is more then one place:

  • Menu_test: This one is just testing the alter hook
  • User: logout users, if they aren't admin and redirect properly
  • openid: Allows openid endpoints to work on offline sites
  • openid_test: Sets the site offline to ensure openid is still working offline

It feels like the approach we could do is to add some key to the response object, which is then checked by MaintenanceModeSubscriber to determine
whether it still wants to be offline and then return the rendered maintenance page. To set a site offline you would implement KernelEvents::REQUEST
with a lower weight then MaintenanceModeSubscriber ... Does this make sense?

ParisLiakos’s picture

yes, adding a key would save calls to _menu_site_is_offline.
A lot better, than subribers having to reimplement the same logic as MaintenanceModeSubscriber

dawehner’s picture

Assigned: dawehner » Unassigned
FileSize
16.12 KB

Started with a patch, but for some odd reasons the event subscribers doesn't run in the proper order

Crell’s picture

+++ b/core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php
@@ -45,13 +50,12 @@ public function onKernelRequestMaintenanceModeCheck(GetResponseEvent $event) {
-    $events[KernelEvents::REQUEST][] = array('onKernelRequestMaintenanceModeCheck', 40);
+    $events[KernelEvents::REQUEST][] = array('onKernelRequestDetermineSiteStatus', 10);
+    $events[KernelEvents::REQUEST][] = array('onKernelRequestMaintenance', 40);

Remember, Priority is High->Low; it's backward from Weight. So DetermineSiteStatus should have the larger integer.

That said, I don't think we need to bother splitting these two listeners to separate methods. There is a performance hit for it, and the code is so short there's not much gain. Adding the _maintenance attribute is fine, and can be done in the same listener.

ParisLiakos’s picture

Status: Active » Needs review
FileSize
16.7 KB

That said, I don't think we need to bother splitting these two listeners to separate methods.

We need to let other modules decide whether they want the site to be in offline status before invoking drupal_maintenance_theme and friends..i think the performance hit is not that big that not calling drupal_alter doesnt make up for it?

Crell’s picture

Oh, I get it. You want to allow other modules to slip in between those two. That makes more sense. I don't know about the performance comparison to alter, but the use case works.

dawehner’s picture

This is a interdiff between #5 and #7

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

The last submitted patch, drupal-1998228-9.patch, failed testing.

ParisLiakos’s picture

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

#9: drupal-1998228-9.patch queued for re-testing.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

This looks OK to me. Thanks, Paris!

ParisLiakos’s picture

dawehner’s picture

Maybe i'm pessimistic, but isn't something like user_logout() an actual helpful api function?

ParisLiakos’s picture

Status: Reviewed & tested by the community » Needs work

you are right..but i cant find a correct place for this, esp. when session is not a service
maybe a manager-like hack?

Crell’s picture

If we have to keep it, $user->logout() on the user object, or perhaps on the user storage controller/repository/thing. (I'm sure Mark will tell us which one is Correct(tm).)

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
14.44 KB

lets do this for now:
since it is considered an API function, moved in user.module
updated the UserController to call that instead of duplicating logic
removed the redirection..so it only kill session, calls watchdog and triggers the hook

also..no openid here, thats why the patch is smaller

Status: Needs review » Needs work

The last submitted patch, drupal-1998228-17.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
533 bytes
14.4 KB

sigh

dawehner’s picture

+++ b/core/modules/user/user.moduleundefined
@@ -2785,3 +2743,17 @@ function user_library_info() {
+/**
+ * Logs the current user out.
+ */
+function user_logout() {
+  global $user;
+
+  watchdog('user', 'Session closed for %name.', array('%name' => $user->name));
+
+  Drupal::moduleHandler()->invokeAll('user_logout', array($user));
+
+  // Destroy the current session, and reset $user to the anonymous user.
+  session_destroy();
+}

Can't we have some kind of class for that which deals with this?

ParisLiakos’s picture

is it really under the scope of this issue?

dawehner’s picture

Status: Needs review » Fixed

I agree.

tim.plunkett’s picture

Status: Fixed » Reviewed & tested by the community

I think @dawehner meant RTBC :)

ParisLiakos’s picture

for one moment i thought dawehner got commit access:P
opened #2012976: Deprecate user_logout() and user_login_finalize() and replace with a service for discussing what to do with user_logout

Crell’s picture

#19 has a +1 from me as well.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

So you're killing the hook... that means we need to remove it from system.api.php and it is also referenced in MenuRouterTest::testMaintenanceModeLoginPaths - we should be pointing people at the new event here.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
2.73 KB
17.13 KB

oh, i am sorry, you are totally right, i missed those

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Great!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll

curl https://drupal.org/files/drupal-1998228-27.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 17544  100 17544    0     0  11728      0  0:00:01  0:00:01 --:--:-- 18294
error: patch failed: core/modules/user/lib/Drupal/user/Controller/UserController.php:11
error: core/modules/user/lib/Drupal/user/Controller/UserController.php: patch does not apply
ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
17.14 KB

reroll

Status: Needs review » Needs work
Issue tags: -WSCCI, -Needs reroll

The last submitted patch, drupal-1998228-30.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI, +Needs reroll

#30: drupal-1998228-30.patch queued for re-testing.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

finally

alexpott’s picture

Title: Remove hook_menu_site_status_alter() in favor of request listeners » Change notice: Remove hook_menu_site_status_alter() in favor of request listeners
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: -Needs reroll +Needs change record
alexpott’s picture

Committed 27bd89b and pushed to 8.x. Thanks!

ParisLiakos’s picture

Status: Active » Needs review
Issue tags: -Needs change record
ParisLiakos’s picture

Crell’s picture

Title: Change notice: Remove hook_menu_site_status_alter() in favor of request listeners » Remove hook_menu_site_status_alter() in favor of request listeners
Priority: Critical » Normal
Status: Needs review » Fixed
Issue tags: +Needs change record

Looks good to me.

Status: Fixed » Closed (fixed)

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

xjm’s picture

Issue tags: -Needs change record

Untagging. Please remove the tag when the change notification task is completed.

xjm’s picture

d.o--