Problem/Motivation
The MaintenanceModeSubscriber
uses a request attribute to specify whether the site is in maintenance mode for the currently logged in user. Also the subscriber is split up into two methods, one running early and one running later in the request. The early method determines whether the site is in maintenance mode, while the later renders the maintenance page if necessary.
The purpose of this split is to allow customization of the maintenance mode detection logic by additional request-subscribers, which are required to run within a specified range. The comment in \Drupal\Core\EventSubscribers\MaintenanceMode::getSubscribedEvents()
states:
// In order to change the maintenance status an event subscriber with a
// priority between 30 and 40 should be added.
For #2286971: Remove dependency of current_user on request and authentication manager it is necessary to straighten a bit the maintenance mode logic, such that the MaintenanceModeSubscriber
can be moved entirely after routing / authentication.
Proposed resolution
- Introduce
\Drupal\Core\Site\MaintenanceMode
(service:maintenance_mode
with the following public methods:applies($route_match)
Returns TRUE if the site is in maintenance mode for the given route match.exempt($account)
Determines whether a user has access to the site in maintenance mode.
- On routes with the
_maintenance_access
option set toTRUE
, maintenance mode will never be active. Use this option onuser.login
,user.pass
anduser.reset
and remove the code responsible for excluding those routes from maintenance mode from the user maintenance mode subscriber.
This allows us to remove the MaintenanceModeSubscriber::onKernelRequestDetermineSiteStatus()
and the _maintenance
request attribute without scarifying customization options for contrib and site builders.
Remaining tasks
Review.
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#29 | interdiff.txt | 1.59 KB | znerol |
#29 | 2288665-site-maintenance-status-negotiator-29.diff | 20.69 KB | znerol |
#25 | interdiff.txt | 13.33 KB | znerol |
#25 | 2288665-site-maintenance-status-negotiator-25.diff | 20.45 KB | znerol |
#23 | interdiff.txt | 6.7 KB | znerol |
Comments
Comment #1
znerol CreditAttribution: znerol commentedComment #3
znerol CreditAttribution: znerol commentedOops, remove the editor autosave file.
Comment #4
znerol CreditAttribution: znerol commentedComment #6
znerol CreditAttribution: znerol commentedComment #7
damiankloip CreditAttribution: damiankloip commentedNice, overall I think this patch is a really good idea.
@var
IMO we shouldn't have an account dependency or this logic in this service. Maintenance mode just uses the site's status. I think that should be in the MaintenanceModeSubscriber.
Do you think we should allow a null status to be set?
Same as last comment.
We should consider changing this to use the urlGenerator instead here.
Comment #8
znerol CreditAttribution: znerol commentedWorking on another issue I realized that probably it would be a good idea to introduce a chain of responsibility (like suggested by @Crell before in #2124749-5: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API. E.g. the user module excludes some routes from the maintenance mode (login and password reset). I think that the risk of performance regressions mentioned on the comment in the other issue is minor. Especially if we only allow exclusion of pages/users from maintenance mode (i.e. if maintenance mode is on, give contrib the chance to exclude some pages/users from it). This way production mode would be not affected at all.
Comment #9
znerol CreditAttribution: znerol commentedThis implements a simple chain-of-responsibility. Note that in this case it is not necessary to implement priorities (like e.g. in the breadcrumb implementation). This is because the response from
MaintenanceExemption
is boolean. I'm not too sure about the naming of the new classes / methods. Any opinions?Note that this implementation also removes the requirement that the user maintenance mode subscriber is run before the core-one. In fact it looks like it may become possible to remove the user maintenance mode subscriber entirely in the long run, if we manage to implement exemption from the maintenance mode in a sane way.
Comment #10
znerol CreditAttribution: znerol commentedComment #11
znerol CreditAttribution: znerol commentedUse the
RouteMatch
class inMaintenanceExemption
.Comment #12
znerol CreditAttribution: znerol commentedRestore test-case in
MenuRouterTest
using maintenance exemption instead of a request subscriber.Comment #13
dawehnerNot really sure wether this is a good name. I had to look it up, but I never saw that name somewhere else yet.
Can we please use $this->t() which should be available ... I know the original code did not had that either.
I really love the abstraction coming from the route match now. I wonder whether we really need getStatus and exempt(). getStatus could depend on route_match as well.
Comment #14
Crell CreditAttribution: Crell commentedI really like the direction this is going!
Could we even go a step farther and turn this into a MaintenanceModeException, which then gets processed by the normal exception handling routines (and thus we don't need to do this duplicate weirdness and can get closer to eliminating drupal_maintenance_theme() and DefaultHtmlPageRenderer::renderPage())?
This line is unnecessary. Just set the property to = []; above.
This needs to be part of the interface.
Should get the generator injected and use a route name here.
in_array() is a bit cleaner here. I don't know if it's any faster (probably not) but it's shorter and can be returned directly. Ie:
Comment #15
znerol CreditAttribution: znerol commentedAdressed #10.1 and #10.5 and #9.2.
Note that it is necessary to restrict the maintenance-mode check to the master request because the symfony exception listener spawns a subrequest to pass the exception into the
ExceptionController
. Throwing theServiceUnavailableHttpException
when attempting to respond to theServiceUnavailableHttpException
leads to recursion.Also note that the call to
drupal_maintenance_theme()
is gone. I do not understand how to use$this->htmlPageRenderer
such that it renders the maintenance theme. Therefore I simply copied over the relevant code fromon500Html()
toon503Html()
which usesDefaultHtmlPageRenderer
instead of the injected instance.Regarding #9.3:
I think we should remove
getStatus()
entirely and use$this->state->get('system.maintenance_mode')
(like HEAD). Otherwise we would also have to introduce asetStatus()
method and use it in theSiteMaintenanceModeForm
. Will address #10.2, #10.3 when exploring that direction.#10.4 is addressed in #2288911: Use route name instead of system path in user maintenance mode subscriber. However due to the overlaps I may just merge that one into this issue.
Comment #16
znerol CreditAttribution: znerol commentedReplace the
StatusNegotiator
withChainMaintenanceExemption
and add appropriate interfaces. Addresses #9.3, #10.2, #10.3.Comment #19
znerol CreditAttribution: znerol commentedManual testing also reveals that we now have the Operating in maintenance mode message when the login page is accessed by an anonymous user.
This means that we cannot use
exempt()
for detecting whether a route should be exempted from maintenance mode as well as whether the logged in user has the permission to bypass it. I think it might be possible to simplify the whole thing and just extract maintenance-detection logic from the event subscriber into a dedicated service. Also we could add an attribute to the login and password reset route such that the status negotiation service is able to detect whether a page should be exempted from maintenance mode without having to implement a chain of responsibility for that part.Comment #20
znerol CreditAttribution: znerol commentedTried #19, interdiff is against #15.
Comment #21
znerol CreditAttribution: znerol commentedComment #23
znerol CreditAttribution: znerol commentedTurns out that symfony ExceptionListener::logException() invokes
error_log()
if the status-code of a HTTP exception>=500
. Because of this and because of the recursion pointed out in #15 let's not throw an exception here.Comment #24
znerol CreditAttribution: znerol commentedComment #25
znerol CreditAttribution: znerol commentedPolished the interface:
SITE_OFFLINE
andSITE_ONLINE
constants.StatusNegotiatorInterface
toMaintenanceModeInterface
,alsoStatusNegotiatorInterface::getStatus()
toMaintenanceModeInterface::applies()
site_status_negotiator
service intomaintenance_mode
serviceComment #26
znerol CreditAttribution: znerol commentedComment #27
znerol CreditAttribution: znerol commentedComment #28
Crell CreditAttribution: Crell commentedCan we at least leave a todo on here, since DefaultHtmlPageRenderer::renderPage() is a crime against humanity that is slated for execution as soon as possible?
Space in the wrong place.
Comment #29
znerol CreditAttribution: znerol commentedThanks for the review. Reroll after #2239009: Remove public direct usage of the '_system_path' request attribute, also address #28. Filed #2295609: Break the dependency of MaintenanceModeSubscriber on DefaultHtmlPageRenderer.
Comment #30
xjmChange record that will need an update for this issue:
https://www.drupal.org/node/2020005
I've added a reference to this issue so that it appears listed here.
Comment #31
Crell CreditAttribution: Crell commentedI think we're good here, then. Thanks, znerol!
We update the existing change notice AFTER this gets committed, right?
Comment #32
xjm@Crell, yep, if it's only a minor change that is fine.
Comment #33
alexpottCommitted baf2bba and pushed to 8.x. Thanks!
Fixed on commit
Comment #35
xjmThe change record updates are still needed here. Let's update them and then the tag can be removed. Otherwise I will haunt you. :D