Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#30 | drupal-1998228-30.patch | 17.14 KB | ParisLiakos |
#27 | drupal-1998228-27.patch | 17.13 KB | ParisLiakos |
#27 | interdiff.txt | 2.73 KB | ParisLiakos |
#19 | drupal-1998228-19.patch | 14.4 KB | ParisLiakos |
#19 | interdiff.txt | 533 bytes | ParisLiakos |
Comments
Comment #1
ParisLiakos CreditAttribution: ParisLiakos commentedor an equivalent of user's module subscriber since it contains user module specific logic
Comment #2
dawehnerTrying to work on that.
Comment #3
dawehnerAfter some research it comes clear that there is more then one place:
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?
Comment #4
ParisLiakos CreditAttribution: ParisLiakos commentedyes, adding a key would save calls to
_menu_site_is_offline
.A lot better, than subribers having to reimplement the same logic as
MaintenanceModeSubscriber
Comment #5
dawehnerStarted with a patch, but for some odd reasons the event subscribers doesn't run in the proper order
Comment #6
Crell CreditAttribution: Crell commentedRemember, 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.
Comment #7
ParisLiakos CreditAttribution: ParisLiakos commentedWe 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?Comment #8
Crell CreditAttribution: Crell commentedOh, 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.
Comment #9
dawehnerThis is a interdiff between #5 and #7
Comment #11
ParisLiakos CreditAttribution: ParisLiakos commented#9: drupal-1998228-9.patch queued for re-testing.
Comment #12
Crell CreditAttribution: Crell commentedThis looks OK to me. Thanks, Paris!
Comment #13
ParisLiakos CreditAttribution: ParisLiakos commentedfixed the @todo now that #1987894: Convert user_logout() to a new style controller is in
Comment #14
dawehnerMaybe i'm pessimistic, but isn't something like user_logout() an actual helpful api function?
Comment #15
ParisLiakos CreditAttribution: ParisLiakos commentedyou are right..but i cant find a correct place for this, esp. when session is not a service
maybe a manager-like hack?
Comment #16
Crell CreditAttribution: Crell commentedIf 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).)
Comment #17
ParisLiakos CreditAttribution: ParisLiakos commentedlets 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
Comment #19
ParisLiakos CreditAttribution: ParisLiakos commentedsigh
Comment #20
dawehnerCan't we have some kind of class for that which deals with this?
Comment #21
ParisLiakos CreditAttribution: ParisLiakos commentedis it really under the scope of this issue?
Comment #22
dawehnerI agree.
Comment #23
tim.plunkettI think @dawehner meant RTBC :)
Comment #24
ParisLiakos CreditAttribution: ParisLiakos commentedfor 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
Comment #25
Crell CreditAttribution: Crell commented#19 has a +1 from me as well.
Comment #26
alexpottSo 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.
Comment #27
ParisLiakos CreditAttribution: ParisLiakos commentedoh, i am sorry, you are totally right, i missed those
Comment #28
dawehnerGreat!
Comment #29
alexpottNeeds a reroll
Comment #30
ParisLiakos CreditAttribution: ParisLiakos commentedreroll
Comment #38
tim.plunkett#30: drupal-1998228-30.patch queued for re-testing.
Comment #39
ParisLiakos CreditAttribution: ParisLiakos commentedfinally
Comment #40
alexpottComment #41
alexpottCommitted 27bd89b and pushed to 8.x. Thanks!
Comment #42
ParisLiakos CreditAttribution: ParisLiakos commentedChange notice https://drupal.org/node/2020005
Comment #43
ParisLiakos CreditAttribution: ParisLiakos commentedChange notice https://drupal.org/node/2020005
Comment #44
Crell CreditAttribution: Crell commentedLooks good to me.
Comment #46
xjmUntagging. Please remove the tag when the change notification task is completed.
Comment #47
xjmd.o--