Use route name instead of system path in user maintenance mode subscriber

1. +++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
   @@ -88,8 +88,23 @@ public function authenticate(Request $request) {
   +    // @todo: Currently it is necessary to replicate the behavior of
   +    // AuthenticationEnhancer here, and restrict allowed providers to the
   +    // default one, if none was specified on the _auth option. This restriction
   +    // will be removed by https://www.drupal.org/node/2286971
   

   @todo Duplicates AuthenticationEnhancer by reducing providers to the default one if no _auth was specified.
   @see https://www.drupal.org/node/2286971
   

2. +++ b/core/modules/user/src/EventSubscriber/ExceptionRedirectSubscriber.php
   @@ -0,0 +1,81 @@
   +      if ($route_name == 'user.login') {
   +        // If user is logged in, redirect to 'user' instead of giving 403.
   +        $event->setResponse(new RedirectResponse($this->urlGenerator->generateFromPath('user', array('absolute' => TRUE))));
   

   Shouldn't this redirect to the full /user/123 entity URI/path instead of the /user alias?

3. +++ b/core/modules/user/src/EventSubscriber/ExceptionRedirectSubscriber.php
   @@ -0,0 +1,81 @@
   +        return;
   ...
   +        return;
   

   Let's replace these early-returns with elseif

4. +++ b/core/modules/user/user.services.yml
   @@ -40,6 +40,11 @@ services:
   +  user_exception_redirect_subscriber:
   +    class: Drupal\user\EventSubscriber\ExceptionRedirectSubscriber
   

   Don't we have a better naming pattern for such services? E.g.,

   user.routing.exception_subscriber

   ?

   Also, the aspect of "redirect" is rather meaningless in the service ID + class name... Should reference the context of "account" instead.

This patch looks great. Just some nits:

1. +++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
   @@ -88,8 +88,22 @@ public function authenticate(Request $request) {
   +    // If authentication is triggered after routing, restrict the allowed
   +    // providers to the ones specified in the routes _auth option.
   +    //
   +    // @todo Duplicates AuthenticationEnhancer by reducing providers to the
   +    // default one if no _auth was specified. This restriction will be removed
   +    // by https://www.drupal.org/node/2286971
   

   Even after reading this comment, I don't get why this is necessary. What's the flow that this patch changes that results in AuthenticationEnhancer not running or not running at the right time?

2. +++ b/core/modules/user/src/EventSubscriber/AccessDeniedSubscriber.php
   @@ -0,0 +1,96 @@
   +   * @param \Drupal\Core\Session\AccountInterface $account
   +   *   The current user.
   +   */
   +  public function __construct(AccountInterface $account, URLGeneratorInterface $url_generator) {
   

   Missing @param.

3. +++ b/core/modules/user/src/EventSubscriber/MaintenanceModeSubscriber.php
   @@ -19,7 +20,7 @@
   -   * Determine whether the page is configured to be offline.
   +   * Logout users if site is in maintenance mode.
   

   The doc is an improvement, but still doesn't capture everything the function does.

4. +++ b/core/modules/user/src/EventSubscriber/MaintenanceModeSubscriber.php
   @@ -28,7 +29,7 @@ public function onKernelRequestMaintenance(GetResponseEvent $event) {
   +    $route_name = $request->attributes->get(RouteObjectInterface::ROUTE_NAME);
   

   Could use RouteMatch here?

1. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -26,18 +26,20 @@ class UserController extends ControllerBase {
   +      $response = $this->redirect('user.view', array('user' => $user->id()));
   

   This change doesn't seem like it does anything, it just means an extra call to ->id(), why?

2. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -26,18 +26,20 @@ class UserController extends ControllerBase {
   -      return $this->redirect('user.login');
   +      $form_builder = $this->formBuilder();
   +      $response = $form_builder->getForm('Drupal\user\Form\UserLoginForm');
   

   This was a very specific behavior in D7 that we were replicating

3. +++ b/core/modules/user/src/EventSubscriber/MaintenanceModeSubscriber.php
   @@ -42,8 +42,12 @@ public function onKernelRequestMaintenance(GetResponseEvent $event) {
   +            $event->setResponse(new RedirectResponse(url('user/login', array('absolute' => TRUE))));
   

   Whaaaat. Why are you using url() with a path here?

LOL. I have flashbacks to writing that user login code, and I tried about 5 ways before coming up with whatever is in there, so I was nervous we were changing it. I didn't realize it was a revert :D

1. +++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
   @@ -155,6 +155,36 @@ public function getSortedProviders() {
   +   * @param Request $request
   

   Can we make this typehint absolute? 1354 will tell you about that.

2. +++ b/core/modules/user/src/Access/LoginStatusCheck.php
   @@ -27,8 +30,10 @@ class LoginStatusCheck implements AccessInterface {
   +    $required_status = filter_var($route->getRequirement('_user_is_logged_in'), FILTER_VALIDATE_BOOLEAN);
   

   Interesting but let's be honest == 'TRUE' is way easier to read, isn't it?

3. +++ b/core/modules/user/src/EventSubscriber/AccessDeniedSubscriber.php
   @@ -0,0 +1,98 @@
   +class AccessDeniedSubscriber implements EventSubscriberInterface {
   ...
   +   * Constructs a new redirect subscriber.
   

   Let's fix the docs.

4. +++ b/core/modules/user/src/EventSubscriber/AccessDeniedSubscriber.php
   @@ -0,0 +1,98 @@
   +
   +  /**
   +   * Generates a URL or path for a specific route based on the given parameters.
   +   *
   +   * @see \Drupal\Core\Routing\UrlGeneratorInterface::generateFromRoute() for
   +   *   details on the arguments, usage, and possible exceptions.
   +   *
   +   * @return string
   +   *   The generated URL for the given route.
   +   *
   +   * @todo: Use UrlGeneratorTrait (see https://www.drupal.org/node/2282161)
   +   */
   +  protected function url($route_name, $route_parameters = array(), $options = array()) {
   +    return $this->urlGenerator->generateFromRoute($route_name, $route_parameters, $options);
   +  }
   +
   

   Let's use the generator as written in the todo

5. +++ b/core/modules/user/src/RegisterForm.php
   @@ -40,11 +40,6 @@ public function form(array $form, array &$form_state) {
    
   -    // If we aren't admin but already logged on, go to the user page instead.
   -    if (!$admin && $user->isAuthenticated()) {
   -      return new RedirectResponse(url('user/' . \Drupal::currentUser()->id(), array('absolute' => TRUE)));
   -    }
   

   It is great that we can remove that

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -314,6 +314,9 @@ public function rebuildForm($form_id, FormStateInterface &$form_state, $old_form
   +      if (\Drupal::getContainer()->initialized('session_manager')) {
   +        \Drupal::getContainer()->get('session_manager')->startLazy();
   +      }
   

   I don't get why this is needed. If we have to pollute the FormBuilder with a Drupal:: call it should be documented with why.

2. +++ b/core/modules/user/src/EventSubscriber/AccessDeniedSubscriber.php
   @@ -0,0 +1,98 @@
   +class AccessDeniedSubscriber implements EventSubscriberInterface {
   

   I like this class.

3. +++ b/core/modules/user/src/EventSubscriber/AccessDeniedSubscriber.php
   @@ -0,0 +1,98 @@
   +      switch ($route_name) {
   +        case 'user.login';
   +          // Redirect an authenticated user to the profile page.
   +          $event->setResponse(new RedirectResponse($this->url('user.view', array('user' => $this->account->id()), array('absolute' => TRUE))));
   +          break;
   +
   +        case 'user.register';
   +          // Redirect an authenticated user to the profile form.
   +          $event->setResponse(new RedirectResponse($this->url('user.edit', array('user' => $this->account->id()), array('absolute' => TRUE))));
   +          break;
   +      }
   +    }
   

   Nitpick: If we're already making a utility method for url() here, the only thing that varies at all between these two is the route name. May as well move the entire RedirectResponse creation to the method, too. (Not a commit blocker at all, just some tidying that can be done.)

I hate the idea of such a call, but I hate blocking this on 2 other non-trivial issues more. :-) If we add such a force-login then we would need it documented with a @todo.

I would normally insist on injecting the current_user service, but for a temp hack I may be persuaded that it's better to leave it. IFF we are certain the hack will be removed before 8.0.0.

This should fix the remaining test failures without #57.

+++ b/core/modules/config_translation/src/Tests/ConfigTranslationUiTest.php
@@ -67,6 +67,7 @@ protected function setUp() {
+        'link to any page',

FYI: this is needed because of #2371863: SiteInformationForm validates access to paths that aren't changed, potentially making the form unsubmittable.

Would help resolve #2372507: Remove _system_path from $request->attributes and I think at least the blocking session refactoring issues should be critical.

1. +++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
   @@ -177,18 +207,11 @@ public function cleanup(Request $request) {
   +      if ($provider->handleException($event) == TRUE) {
   

   Did you meant === TRUE? Otherwise you can just skip == TRUE entirely.

2. +++ b/core/modules/user/src/EventSubscriber/AccessDeniedSubscriber.php
   @@ -53,7 +53,20 @@ public function onException(GetResponseForExceptionEvent $event) {
   +            $event->setResponse($this->redirect('entity.user.canonical', array('user' => $this->account->id()), array('absolute' => TRUE)));
   ...
   +            $event->setResponse($this->redirect('entity.user.edit_form', array('user' => $this->account->id()), array('absolute' => TRUE)));
   

   Just curious, do we really need an absolut redirect?

3. +++ b/core/modules/user/src/EventSubscriber/MaintenanceModeSubscriber.php
   @@ -69,25 +69,13 @@ public function onKernelRequestMaintenance(GetResponseEvent $event) {
      public static function getSubscribedEvents() {
   -    $events[KernelEvents::REQUEST][] = array('onKernelRequestMaintenance', 35);
   +    $events[KernelEvents::REQUEST][] = array('onKernelRequestMaintenance', 31);
        return $events;
      }
   

   +1 to not longer run before routing!

4. +++ b/core/modules/user/src/EventSubscriber/MaintenanceModeSubscriber.php
   index be73f39..326adcf 100644
   --- a/core/modules/user/src/RegisterForm.php
   
   --- a/core/modules/user/src/RegisterForm.php
   +++ b/core/modules/user/src/RegisterForm.php
   

   +1 to not hardcode that into the form Note: At least RedirectResponse can now be removed from the use statements, Url is also not used anymore

Alright!

+++ b/core/modules/user/src/EventSubscriber/AccessDeniedSubscriber.php
@@ -17,7 +17,12 @@
- * Redirects anonymous users from user.page to user.login. Redirects authenticated users from login/register form to the profile.
+ * Redirects users when access is denied.
+ *
+ * Anonymous users are taken to the login page when attempting to access the
+ * user profile pages. Authenticated users are redirected from the login form to
+ * their profile page and from the user registration form to their profile edit
+ * form.

Ha, I was shy to mention this :)

This issue is a critical task and is allowed per https://www.drupal.org/core/beta-changes. Committed 5ddf7c2 and pushed to 8.0.x. Thanks!

diff --git a/core/lib/Drupal/Core/Routing/AccessAwareRouter.php b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
index b3033ff..93a31c3 100644
--- a/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
+++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
@@ -90,7 +90,7 @@ public function matchRequest(Request $request) {
     $request->attributes->add($parameters);
     // Trigger a session start and authentication by accessing any property of
     // the current user.
-    // @todo: This can will be removed in #2229145.
+    // @todo This will be removed in https://www.drupal.org/node/2229145.
     $this->account->id();
     $this->checkAccess($request);
     // We can not return $parameters because the access check can change the
diff --git a/core/modules/system/src/Tests/System/PageTitleTest.php b/core/modules/system/src/Tests/System/PageTitleTest.php
index b606b07..98148cd 100644
--- a/core/modules/system/src/Tests/System/PageTitleTest.php
+++ b/core/modules/system/src/Tests/System/PageTitleTest.php
@@ -92,7 +92,7 @@ function testTitleXSS() {
     $this->assertNoRaw($title, 'Check for the lack of the unfiltered version of the title.');
     // Add </title> to make sure we're checking the title tag, rather than the
     // first 'heading' on the page.
-    $this->assertRaw($title_filtered . '</title>', 'Check for the filtered version of the title in a &lt;title&gt; tag.');
+    $this->assertRaw($title_filtered . '</title>', 'Check for the filtered version of the title in a <title> tag.');
 
     // Test the slogan.
     $this->assertNoRaw($slogan, 'Check for the unfiltered version of the slogan.');
diff --git a/core/modules/user/src/EventSubscriber/MaintenanceModeSubscriber.php b/core/modules/user/src/EventSubscriber/MaintenanceModeSubscriber.php
index 070c5d4..2723074 100644
--- a/core/modules/user/src/EventSubscriber/MaintenanceModeSubscriber.php
+++ b/core/modules/user/src/EventSubscriber/MaintenanceModeSubscriber.php
@@ -58,7 +58,6 @@ public function __construct(MaintenanceModeInterface $maintenance_mode, AccountI
   public function onKernelRequestMaintenance(GetResponseEvent $event) {
     $request = $event->getRequest();
     $route_match = RouteMatch::createFromRequest($request);
-    $route_name = $route_match->getRouteName();
     if ($this->maintenanceMode->applies($route_match)) {
       // If the site is offline, log out unprivileged users.
       if ($this->account->isAuthenticated() && !$this->maintenanceMode->exempt($this->account)) {

Fixed on commit. $route_name is not used in core/modules/user/src/EventSubscriber/MaintenanceModeSubscriber.php.