Broken link to 'Put your site into maintenance mode' on update.php results in WSOD

Issue tested.

Reproduced. Even when using Drupal at example.com, not just when using Drupal inside example.com/subdir.

This is definitely missing test coverage.

+++ b/core/modules/system/src/Controller/DbUpdateController.php
@@ -218,7 +219,7 @@ protected function info(Request $request) {
-      ':url' => Url::fromRoute('system.site_maintenance_mode')->toString(TRUE)->getGeneratedUrl(),
+      ':url' => $base_url . \Drupal::url('system.site_maintenance_mode', [], ['base_url' => FALSE]),

This is most definitely wrong. This is not using the URL generation system. Investigating.

1. This is a duplicate of #2548095: Add option to Url() to force the site base_url to be used.

   Because \Drupal\system\Controller\DbUpdateController::requirements does this:

       // @todo Revisit once https://www.drupal.org/node/2548095 is in. Something
       // like Url::fromRoute('system.db_update')->setOptions() should then be
       // possible.
       $try_again_url = Url::fromUri($request->getUriForPath(''))->setOptions(['query' => $options])->toString(TRUE)->getGeneratedUrl();
   

   This code should follow the same pattern until #2548095: Add option to Url() to force the site base_url to be used is fixed.

   But then again I don't see how to generate a URL for a route using that approach, because it only works for links pointing to the same path, but with a different querystring. So, scratch that.

2. \Drupal\Core\Update\UpdateKernel + \Symfony\Component\HttpFoundation\Request::prepareBaseUrl() may be to blame: the latter concludes that the base URL is /update.php. This matches the documentation of the Request::getBaseUrl() function, on which the UrlGenerator relies for relative URLs:
   

       /**
        * Returns the root URL from which this request is executed.
        *
        * The base URL never ends with a /.
        *
        * This is similar to getBasePath(), except that it also includes the
        * script filename (e.g. index.php) if one exists.
        *
        * @return string The raw URL (i.e. not urldecoded)
        */
       public function getBaseUrl()
       {
           if (null === $this->baseUrl) {
               $this->baseUrl = $this->prepareBaseUrl();
           }
   
           return $this->baseUrl;
       }
   

   So including the script name is correct!

   Which means that this code in UrlGenerator::generateFromRoute():

   $base_url = $this->context->getBaseUrl();
   

   is wrong, because it calls \Symfony\Component\Routing\RequestContext::getBaseUrl(), which is set to the value retrieved from Request::getBaseUrl(), which may include the script name. That only happens for requests made to custom front controllers that aren't hidden by clean URL rewriting.

   This means that all relative URLs are broken for update.php, but also index.php without clean URLs. Which again points to #2548095: Add option to Url() to force the site base_url to be used and #2529170: [PP-1] Remove DrupalKernel::initializeRequestGlobals and replace base_root, base_url and base_path with a service.

   I don't see an easy way to force a different base URL for custom kernels (i.e. UpdateKernel).

Clearly our URL generator mechanism is troublingly complex.

The only solution I can think of: using base:// + ... This is terrible. But it's not this issue's fault.

This took 40 minutes, and should have taken 0.4 or 4 at most.

Thanks @Wim Leers and @cebasqueira. This definitely should not be this hard. :P

I agree the kinda-hacky ../ is a better workaround than globals and concatenations; at least this way we still end up with a proper URL object. Though, good work @cebasqueira at finding something that worked at all!

Wow. Fugly test for a fugly patch. Not sure how to handle the attribution for some code adapted from a Stack Overflow answer. I couldn't find anything in core or vendor that did the same thing.

+++ b/core/modules/system/src/Tests/Update/UpdateScriptTest.php
@@ -284,6 +284,28 @@ function testSuccessfulMultilingualUpdateFunctionality() {
+    /** @var \SimpleXMLElement[] $links */
+    $links = $this->xpath('//a[normalize-space(text())=:label]', array(':label' => 'maintenance mode'));
+    $this->assertNotNull($links);
+    $href = (string) $links[0]['href'];
+
+    // Adapted from @see http://stackoverflow.com/a/4444490
+    $re = ['#(/\.?/)#', '#/(?!\.\.)[^/]+/\.\./#'];
+    for ($n = 1; $n > 0; $href = preg_replace($re, '/', $href, -1, $n)) { }
+    $href = $this->getAbsoluteUrl($href);
+    $this->assertEqual($this->getAbsoluteUrl('admin/config/development/maintenance'), $href);

Maybe instead of validating the link with this mess, we can $this->drupalGet() the $href we extract from the (string) $links[0]['href'], and then make sure we get a 200 and are on the right page by checking for the title? It's late, I may try this tomorrow.

Actually, $this->drupalGet() doesn't handle the .. in the URL well (try it locally and see). We may be stuck with the test approach in #10.

I just stumbled upon this problem while manually running the update path for #2336597: Convert path aliases to full featured entities, and it's a hard dependency for that issue. It would be nice if someone who knows the url generation / routing system could bring this one to completion :)

In the meantime, here's a reroll of #10.

All the 4 working links in DbUpdateController.php use this format:

+    // @todo Simplify with https://www.drupal.org/node/2548095
+    $base_url = str_replace('/update.php', '', $request->getBaseUrl());

+      ':url' => Url::fromRoute('system.site_maintenance_mode')->setOption('base_url', $base_url)->toString(TRUE)->getGeneratedUrl(),

Can we use the same code style here also?

Ugh. Just re-re-discovered this bug. Google search led to #2692607: Broken link to 'Put your site into maintenance mode' on update.php results in WSOD which is older than this. Wasted some time updating the summary and adding a test-only patch. Then did some more searching and found this. Marked #2692607 (and a few others floating around) as duplicates of this, which seems to have the most progress/discussion. Here's the test-only I added there. Haven't looked at the patches here, yet. Quick skim of this issue makes me despair at the state of URL handling in core. 😉🤦‍♂️😢

8.8.x version of the test-only, FWIW.

Merging the test coverage from #21 into #18.

Not sure why the interdiff is showing that whole test method, I only added some lines to it, and didn't move it in the file or anything. Weird.

Finally, trying to actually click the link reveals yet another bug (for a follow-up): we present this link to anyone on update.php, even if they don't have 'administer site configuration' permission and the link is a 403. ;)

Sorry for the broken test-only patches:
A) 8.8.x uses Update while 9.0.x uses UpdateSystem. Fun!
B) drupalci.yml is for the evil run-tests.sh, not phpunit. So --filter doesn't mean anything and the bot gets confused.

These should fail (quickly) as expected. ;)

Follow-up for the last point in #23 at #3136762: Update.php includes link to 'Put site into maintenance mode' for users without permission to use it

🤦‍♂️sorry for the noise. Last try on test-only fail patches. ;)

Fixing typos in the summary and update remaining tasks.

Some self-review:

1. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php
   @@ -580,6 +580,42 @@ public function testSuccessfulMultilingualUpdateFunctionality() {
   +    // Check for the link to the maintenance mode setting page.
   +    /** @var \Behat\Mink\Element\NodeElement[] $links */
   +    $links = $this->xpath('//a[normalize-space(text())=:label]', [':label' => 'maintenance mode']);
   +    $this->assertNotNull($links);
   +    $href = $links[0]->getAttribute('href');
   +
   +    // Adapted from @see http://stackoverflow.com/a/4444490
   +    $re = ['#(/\.?/)#', '#/(?!\.\.)[^/]+/\.\./#'];
   +    for ($n = 1; $n > 0; $href = preg_replace($re, '/', $href, -1, $n)) {
   +    }
   +    $href = $this->getAbsoluteUrl($href);
   +    $this->assertEquals($this->getAbsoluteUrl('admin/config/development/maintenance'), $href);
   

   FWIW, I don't love any of this coverage. ;) Seems like a lot of whacky magic. I believe we learn more by just clicking the link, like I added below. What do y'all think about ripping this out?

2. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php
   @@ -580,6 +580,42 @@ public function testSuccessfulMultilingualUpdateFunctionality() {
   +    $admin_user = $this->drupalCreateUser([
   +      'administer software updates',
   +      'access administration pages',
   +      'administer site configuration',
   +    ]);
   

   This comes up a few places in this test class. Would it be in scope to add this as another protected variable and create the user in setup()?

   And/or add 'administer site configuration' to the perms for the self::updateUser ?

This addresses #31.1. I left #32.2 alone for now.

Also fixed a minor indentation bug for the new @todo comment.

The 8_9_x patch applies cleanly and passes on 9.1.x, 9.0.x and 8.9.x branches.

Re: #33:

There was 1 error:

1) Drupal\Tests\system\Functional\Update\UpdateScriptTest::testUpdateLinks
Exception: TypeError: Argument 1 passed to Drupal\Core\ProxyClass\Render\BareHtmlPageRenderer::renderBarePage() must be of the type array, boolean given, called in /var/www/html/core/modules/system/src/Controller/DbUpdateController.php on line 195
Drupal\Core\ProxyClass\Render\BareHtmlPageRenderer->renderBarePage()() (Line: 73)

Which is exactly what end users see right now if they try to use that link from update.php. Seems like a reasonable test failure, even though it's more cryptic than the results from #26:

There was 1 failure:

1) Drupal\Tests\system\Functional\Update\UpdateScriptTest::testUpdateLinks
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'http://php-apache-jenkins-drupal-patches-45634/subdirectory/admin/config/development/maintenance'
+'http://php-apache-jenkins-drupal-patches-45634/subdirectory/update.php/admin/config/development/maintenance'

Thoughts?

Thanks,
-Derek

Thank you @dww!

1. +++ b/core/modules/system/src/Controller/DbUpdateController.php
   @@ -217,7 +217,9 @@ protected function info(Request $request) {
   +      // @todo Change to Url::fromRoute('system.site_maintenance_mode') once
   +      //   https://www.drupal.org/node/2548095 is in.
   

   I've tested the patch on my local (See the screenshot above). It works. Even though the URL /update.php/../admin/config/development/maintenance is ugly. But with the @todo there, and as a temp workaround, it's totally fine to me

2. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php
   @@ -580,6 +580,23 @@ public function testSuccessfulMultilingualUpdateFunctionality() {
   +   * Tests links on update.php.
   ...
   +  public function testUpdateLinks() {
   

   As the test is testing the link of maintenance mode only, so I'd suggest changing the comment and method to match it. For example: Tests maintenance mode link on update.php and testMaintenanceModeLink()

3. The links of upgrading handbook and maintenance mode on update.php (see after-1.png attached), do not have target="_blank", which I think it's nice to have. To me, I would be sticking on the update.php, and open the links on new tabs. Jumping out the update.php page is not what users want, perhaps. It's out of scope here, do it in a separate issue, if we agree on that it's necessary.
4. Raising the priority, as this is a link on a frequently viewed page, especially, Drupal 9 is coming soon. Changing the version to 9.1.x-dev as well

Setting back to NW for #2, otherwise, it's RTBC to me.

#35 said to raise the priority, but did not, sorry!

@jungle Thanks for the review!
#35.1: Yeah, it's kinda weird. Maybe #19 is better?
2: We might expand this method to test other links, so I'm inclined not to go through all the churn of a new patch and bot tests just for this.
3: Definitely out of scope. Some people say that links that open in new tabs are evil and wrong. Don't want to go there in this issue. If you want to, please open a follow-up (although I'd search for prior issues/debates on this).
4: Agreed on bumping to major. I was going to do the same but forgot. ;)

Back to NR since it was only NW for 2 which I'm not sure we need. TBD if #19 is cleaner than 35.1 / #6.

@dww, thanks for your quick response!

#37.1, #19 might be better, but it's still a temp workaround, I am good with either way :) to focus on #2548095: Add option to Url() to force the site base_url to be used next is more important.

#37.2 Makes sense to me. Then I'm good with that, personally.

#37.3, Yes, there are good reasons and bad reasons. Just like the post pointed. https://css-tricks.com/use-target_blank. Usually, I use drush updb, so it has no/less chance to bother me.

Making patches in #32 RTBC

Okay, here's with the #19 approach. I think this is better, and it's more consistent with the rest of the controller. Back to NR, although probably still RTBC. ;)

Glad you agree on #32.2, thanks.

9_y_x patch works all the way back to 8.9.x branch. Only 8.8.x needs something different.

Thanks,
-Derek

This solution looks so much better!

Thanks for the review/RTBC @daffie!

Added "Unblock #3136762: Update.php includes link to 'Put site into maintenance mode' for users without permission to use it" to the end of remaining tasks, since I just marked that as postponed on this.

Note to committers: I have gone through all the duplicate issues I found (all of which are now children of this) and other than myself, it looks like @Pasqualle is the only person to have meaningfully contributed to any of those who isn't already credited here. I'm checking that box with this comment. Also crediting @daffie for the RTBC review.

Thanks for working on this issue.

I agree with #35.2 FWIW. If we're going to test multiple links, then let's test multiple links. Otherwise, let's rename the method and make the docblock more specific.

For #35.3, @dww is correct and we should not add that here. It can cause accessibility problems -- see #2652272-50: Review (and in most cases remove) use of target="_blank" attribute outside the installer or update.php. We need to give users choice about it as well as communicate to both sighted and non-sighted users that the link will open a new window. See #2702881: [policy, no patch] Formalize how external links are handled in core for a policy discussion about it.

There are a few (< 10) target="_blank" in core, mostly in the Media and DateTime modules. Two are in the installer. There are many other update.php links that don't have it, so we want to stay consistent with that.

Thanks!

Given that the string only you to do something that you can't do currently, I think this is a case of a patch-eligible string change. (Also I think changing the value of a placeholder perhaps doesn't actually break the string?)

Re: #43: Okay, if that's what you want, we can rename the function. I know #3136762: Update.php includes link to 'Put site into maintenance mode' for users without permission to use it will expand this method, but that'll still be for testing the maintenance mode link, so fine. But I hereby want permission to rename it later if/when it makes sense to do so. I don't want to hear "out of scope" if I/we/someone wants to expand this test method to test other links and re-renames it. ;)

Re: #44: Definitely not a string change. That's (part of) the whole point of placeholders. They're stuff outside the reach of translations, so we can freely change the value of placeholders all we want and all the translations keep working fine.

Glad you agree this is worth backporting to 8.8.x (making it worth all the trouble I've been going through to keep uploading 8.8.x backports). :)

Note: interdiff is confused since I changed the name of the function. It's flagging the entire function as a change, even though the body of the function is identical in #39 vs. this. So I'm also attaching a raw diff of the 2 patch files so you can see I only changed the test method name and the phpdoc summary.

Thanks,
-Derek

The test name has changed and it's docblock.
With that all points of @xjm are addressed.
Back to RTBC.

Committed and pushed 240e303d6a to 9.1.x and 0fb80bb6bb to 9.0.x and 7a7e0aa050 to 8.9.x. Thanks!

Will backport to 8.8.x once it's out of commit freeze.

Alas, this didn't make it into the 8.8.x branch in the brief window when that was possible. At this point, 8.8.x is for security fixes only, so there's no longer anything to port. This bug is fixed as of 8.9.0.

Changing version and status based on confirmation from @xjm in Slack.

Thanks,
-Derek