Move update.php back to a front controller

One thing we need to keep in mind that the container is fundamental, so fundamental that we even need it in order to get to upgrade.php, so
for example we should not couple the container to the cache system, as otherwise the cache table cannot ever be changed.

It is also doesn't make things necessarily easier for people writing update functions as they need to be aware of these 3 different steps. If you are honest though,
they implicit already need to know it.

I do not want, badly so, stir up controversy. Still, I believe have some expertise in update matters which might help. So take everything below with a huge grain of salt and add "I cautiously think..." everywhere.

1. Users might get confused by having two front controllers.
2. update.php in D7 had a clunky hardwired piece of code to get Drupal 7 to bootstrap. This was roughly equivalent to the 1. in the issue summary but it was for major version upgrades only
3. Having an update kernel / container makes sense -- what we want for that is an in memory version of everything. We have the know-how in kernel tests.
4. Separating the "bootstrap broken" upgrades from trivial updates is a good idea for developers.

So: start update.php with in memory services borrowed (reused?) from kernel tests, run hook_upgrade_N (indeed no batch support -- we have in memory storage only so batch would be tricky) and then run hook_update_N. I think displaying the update page / form before hook_upgrade_N is doable so users don't accidentally mess up their database. If we aren't worried about that then just run hook_upgrade_N the moment update.php is loaded. Keep a list of upgrades in sites/default/files/.ht.upgrades -- there's noone else to keep it. I am not worried about information disclosure much, tbh but still .ht should be enough. The files directory placement is a settings override isn't it? So it should be doable w/o the database.

So what we basically need to be able in a state where running rebuild.php would be doable without breaking anything.

I totally agree that we should have something like hook_update_post_N() or something like that, but it is not clear for me whether we will certainly need a hook_update_pre_N().
Should we instead better try to promise that we are not changing storage mechanisms until you come to update.php?

On the other hand now that update.php is a front controller potentially all code, due to the existence of event subscribers, could be executed before we hit the DbUpdateController
so having a upgrade.php (or how we name it), would be a great reduction of fragility.

Stage 2 we already have in HEAD. Should we do Stage 3 in #2538108: Add hook_post_update_NAME() for data value updates to reliably run after data format updates and repurpose this issue for a discussion/implementation of Stage 1?

What is the justification of this being a critical but #2538108: Add hook_post_update_NAME() for data value updates to reliably run after data format updates not?

IMHO the point is that you potentially end up in a non recoverable state, as update.php will never be able to even start.
Step 3 is an API which gives you a tool for the case your system is already in the state where it can run already again. Additional updates, like unpublishing some blocks is something which could also be done in Step 2, there is "just" the problem of not throwing some events/hooks.

According to #2538108-3: Add hook_post_update_NAME() for data value updates to reliably run after data format updates (the issue summary I cited there) that could also end up in WSODs in update and the kind AFAIS. I can see how these are different problems, so not objecting to maintain them as different issues, just trying to figure out the different priorities.

Sure.

We should take care about not using the config API/container to fetch active modules but rather use the bootstrap config factory in order to get the list of active modules.

Can you please explain this issue a bit more?

The Problem/Motivation is: There are different types of updates that can conflict with each other.
And the proposed solution is: To create a new subsystem, with several new hooks.

How does this solution help the stated problem/motivation? I am trying to be constructive, not wanting to argue at all. Maybe it's just the issue summary that is incorrect?

Right now we have /update.php as a route which is a bit weird and mostly to benefit people who read old docs, or something.

Why don't we make update.php (or redirect to core/update.php) a real front controller again and then redirect and use a regular route + path like /admin/system/update to run hook_update_N

#6 claims that #2532476: Menu links should use a TranslationWrapper to encapsulate safe translatable strings from YAML files was a potential need for this issue, but it's not, so I think we should still consider #4's option of figuring out what schema is actually called upon prior to the invocation of a routed (non-front) controller, and if we can live with not changing any of that schema in a BC-breaking way throughout 8.x.

But, if we decide that we do have a need for some very early running update functions, then I think something like #13 makes sense: update.php as a front controller that invokes hook_update_early_N() followed by a redirect to /admin/update for invoking hook_update_N().

Fix a typo in issue summary

These are the tables used by /update.php by default:

• key_value, key_value_expire
• cache_config, cache_data, cache_discovery, cache_bootstrap
• router
• cachetags

@catch
So this issue about fixing potentially broken update.php.

Would it apply to some conflict between different modules, like this Real world example? If not, is there different issue that does?

Right now, such issues should be solved with hook_update_dependencies. But there is no limit to the ways that updates can conflict, and CATCH-22 situations can happen. Updates can do anything and can depend on anything.

Thanks for the answers, and sorry if I am writing in the wrong issue, making noise.

Just gave it a try in order to be able to judge how feasible it is to do.
One problem you run into when doing that is that low level functionality (Extension discovery) is bundled with caching directly.

Re #16, what if you truncate all your cache tables before hitting /update.php? I suspect some more tables would show up in that list.

Good question! Btw. here is a query to clear all cache_ tables: SELECT concat('TRUNCATE TABLE `', TABLE_NAME, '`;') FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_NAME LIKE 'cache_%' AND TABLE_SCHEMA='dev_d8'; (just in case someone wondered whether this is possible).

One interesting sideeffect: I realized /update.php is currently page cached. Are we sure we want to do that?

• cache_config, cache_data, cache_bootstrap, cache_default, cache_discovery
• key_value
• router, url_alias
• SELECT 1 FROM system LIMIT 0, 1 // See #2542632: system.install has redundant code

Just some work in the meantime. This basically now has all the basic steps needed.

Recarding critical vs. not critical, I think the contrib point is quite strong if you think about contrib/custom code doing all kind of crazy stuff, so
they need to be able to repair, if possible.

Re #21, when I try (logged in as user 1), I also see:

• sessions
• users_field_data
• user__roles

I haven't tried #23, but I suspect that it would also need to query those tables, plus the cache and kv ones. So potentially the only difference between routed controller and front controller is router, url_alias, and whatever else contrib does in middlewares and request listeners.

I actively used $update_free_access, because I think being able to even authenticate is way too much risk to get to an update.

Let's authenticate.

I'm working on updating our sites to beta13. Started with the newest site that is currently on beta11, also have some sites that are still on beta8. No idea about those yet..

The biggest problem that I encountered so far is the rename of the plugin_selector module to plugin (thank you tim :p), which is problematic because its interfaces are used, very, very early. In plugin managers, which are included in a compiler pass.

I've managed to work around that for now by manually adjusting the classloader in autoload.php: $loader->setPsr4('Drupal\plugin\\', 'modules/plugin/src');.

I don't really know how to solve this in a generic way, how can we load anything or run update functions without a container which would be the only way to around something like this? I guess as a start, we should make compiler passes that load services/classes as error-resilient as possible, but this is a php fatal error on a missing interface. Only thing I can think of is to remove the interface check there and do it on runtime, in that case it is not in the critical path.

Everything else went pretty smooth, the move of entity manager updates to the end helps *a lot*, compared to the trouble I had especially with beta6 -> 7 and also 7 to 8.

@berdir
Thank you for sharing your "interesting" experience. If I understand correctly, you are basically argueing that given that still APIs are changed a bit, relying on a working container/routing
would not fly at all. In the current iteration of the patch you could not login as user, plugin_selector would have kept a update_early.inc file. In there it could drop the existing container instance, then manipulate the config.extension database entry to contain the new module and afterwards rebuild the container. Shouldn't this then boot up the right version of the container?

1. +++ b/core/update.php
   @@ -0,0 +1,158 @@
   +  // Scan the module list.
   +  // We don't support install profiles at that point?
   +  $extension_discovery = new ExtensionDiscovery($kernel->getAppRoot(), FALSE, []);
   +  $module_extensions = $extension_discovery->scan('module');
   

   which point exactly and support how?

   This worked fine for me for modules inside the install profile. The install profile itself AFAIK can't have update functions, though, would make sense, though. I currently have a separate module just for that.

2. +++ b/core/update.php
   @@ -0,0 +1,158 @@
   +    $kernel->boot();
   +    $container = $kernel->getContainer();
   +    /** @var \Drupal\Core\Authentication\AuthenticationManager $authentication_manager */
   +    $authentication_manager = $container->get('authentication');
   +    $account = $authentication_manager->authenticate($request);
   

   This still gives me a fatal error without my autoload.php hack. That can't be try/catch'd, unfortunately.

   What about checking for update_free_access *first* and don't even try if it that's set to TRUE?

3. +++ b/core/update.php
   @@ -0,0 +1,158 @@
   +  $response = new Response();
   +  $log_filename = PublicStream::basePath() . '/.ht.update_early';
   +
   

   .ht is a apache thing I think.

   Do we really need this? we have an example update early function that writes to database, can't we use a key_value collection and store this information there?

I commented out the auth code and tried it then:

1. run core/update.php
2. That runs and redirects to admin/update. that explodes completely because it doesn't know that route yet, so it tries to display a 404 error page and that explodes due to invalid contexts.
3. drush cr
4. drush en -y beta2beta + drush ev "beta2beta_set_beta_version(11)"
5. Then the page works, I can see the pending updates, but trying to apply gives me this error: "Recoverable fatal error: Argument 1 passed to Drupal\Core\ProxyClass\Render\BareHtmlPageRenderer::renderBarePage()"
6. drush updb to avoid that.

This still gives me a fatal error without my autoload.php hack. That can't be try/catch'd, unfortunately.

What about checking for update_free_access *first* and don't even try if it that's set to TRUE?

Good idea, this should work, too bad that PHP cannot catch this.

.ht is a apache thing I think.

Do we really need this? we have an example update early function that writes to database, can't we use a key_value collection and store this information there?

Well relying on as few as possible things would be nice, just imagine that you might want to change KV, then you cannot query KV maybe before that. Yes we could store the data in a custom DB table which just never changes, but relying on just a file feels safer.

2. That runs and redirects to admin/update. that explodes completely because it doesn't know that route yet, so it tries to display a 404 error page and that explodes due to invalid contexts.

Puh, so we need to trigger a router rebuild already? PUH. Maybe we should enter a row inter {router} manually and clear the corresponding caches.

5. Then the page works, I can see the pending updates, but trying to apply gives me this error: "Recoverable fatal error: Argument 1 passed to Drupal\Core\ProxyClass\Render\BareHtmlPageRenderer::renderBarePage()"

Which code calls BareHtmlPageRenderer with the wrong parameters?

2. Sorry, that wasn't clear. I don't think we need to do a router rebuild unless a specific early update forces that. This is a special case since this doesn't exist yet on the site that we are updating from. It is a situation that will only happen once. That said, since it could be done *after* early updates, it should be a safe thing to do since routes need to work anyway on the redirected page?

But it's possible we care about this one-time situtation too. Then maybe we could check before we redirect if the route exists and only do a router rebuild then before we redirect?

5. DbUpdateController i think, tests are failing for the same reason, from the first test fail: "Argument 1 passed to Drupal\Core\ProxyClass\Render\BareHtmlPageRenderer::renderBarePage() must be of the type array, boolean given, called in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/src/Controller/DbUpdateController.php on line 201 and defined..."

My nginx config (the default in Laravel's Homestead vagrant environment) didn't like update.php as it tried to find the file rather than pass it to index.php. Changing update.php to update will solve this for me and many more to come.

For now I just edited the nginx configuration.

But it's possible we care about this one-time situtation too. Then maybe we could check before we redirect if the route exists and only do a router rebuild then before we redirect?

I fear that doing the router rebuild will be tricky. Just imagine that a view might change the way how the router looks like, and well at that point in time, you can't really guarantee the state of config entities. I fear we need to not only provide the update_early functions as update.php but also all the other one, if we really require a router rebuild.
In general I try to understand why the router should be ever in an inconsistent state? It is not rebuild on runtime anymore.

Did some work to use an Update Kernel and extract some of the functionality in its own class. Don't look at it, unless you really want to.

The rebuild is needed because this patch is changing the routes, so you need to rebuild them to be able to access the new route.

But yes, good points on views and stuff.

What if we keep the existing route and name the new file early_update.php or something like that? Then this doesn't introduce a change and there is no need to rebuild the routes.

Also, I'm having a bit of trouble getting my early update that is supposed to enable a module working properly. Caching seems to interfere, as the caches aren't invalidated and I have no API to invalidate the cache.

I think this is moving toward avoiding to use caches during the updates, which might help but we need to make sure to clear the actual caches at the end.

+++ b/core/modules/system/system.routing.yml
@@ -449,7 +449,7 @@ system.batch_page.json:
+  path: '/admin/update/{op}'

+1 to /admin/update

You may think me weird, but I was dreaming about this, thinking if we move from update.php to update what if someone wants to add some content on /update, I panicked and woke in a sweat. Glad to see @dawehner had already thought about it.

That wouldn't work for an alternative route provider, but should let this run.

This is totally fine, they would provide an update early implementation as well.
I was thinking about rebuilding JUST the yml based ones and fire no event.

Some work on some tests and insert the entry into the router.

@catch: which parts remain critical? does the postponed part means #2538108: Add hook_post_update_NAME() for data value updates to reliably run after data format updates is critical too?

The remaining critical part is to have a dedicated front controller which ensures that you can run updates before having a working Drupal.

I'll work on this during drupalaton.

Well I don't care whether it is critical now or later. GIven how fragile a full website with all kind of subscribers I think we WILL run into the issue of not being able
to come to update.php. This will immediately be critical, so solving it now or later IMHO doesn't matter. If people, especially contrib will do stuff, we will have a critical issue sooner or later.

While I totally agree that hook_update_early_N() is critical, here is a first version of something like update.php.

Right, my question was more from a release management perspective. If the assessment of what we aim for for the 8.0.0 release changed, we should keep in check if what we wanted to originally is still critical for the initial release. (Not arguing if it is or not in fact).

Less failures, this is for sure.

Some documentation improvements.

Fixes the access parts, now we are dealing with the logo pointing to /update.php but so failing some tests.

+++ b/core/modules/system/src/Controller/DbUpdateController.php
@@ -364,7 +366,8 @@ protected function selection() {
-      $url = new Url('system.db_update', array('op' => 'run'));
+      $base_url = str_replace('/update.php', '', $request->getBaseUrl());
+      $url = (new Url('system.db_update', array('op' => 'run')))->setOption('base_url', $base_url);
       $build['link'] = array(

Made #2548095: Add option to Url() to force the site base_url to be used to avoid shenanigans like this.

There we go.

Added the @todo for #60. Thank you a lot @mpdonadio to add the issue.

Just a spelling check :)

1. +++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
   @@ -0,0 +1,164 @@
   + * to repair Drupal if its in a broken state.
   

   it is

2. +++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
   @@ -0,0 +1,164 @@
   +   * This fake routing data is needed in order to make batch API working
   +   * properly.
   

   work properly

1. --- /dev/null
   +++ b/update.php
   

   Missing @file comment

2. +++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
   @@ -0,0 +1,164 @@
   +   * Ensures that the request has a fake routing data for update.php.
   

   s/a fake routing data/fake routing data/

3. +++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
   @@ -0,0 +1,164 @@
   +   * Checks access and throws an access, in case it is not.
   

   throws an exception, in case it is not accessible

Fixing for comments in #65, probably not in the same way as suggested but I think it will suffice.

Thank you @stefan.r and @hussainweb
Will work now on making a dedicated, not compiled, container.

This is some work to let update.php use its own container.

There are still things to be figured out:

• How to swap to the correct kernel for drupal_flush_all_caches() at the end of the update. How can we leverage a DrupalKernel object for that.

Theoretically it would have been possible with #68 to limit the available APIs so you could not call out the the entity manager but use the config API/state/KV

Gabor pinged me for a review here. I've not read the full issue yet so if there's some previously discussed reason for something above, I missed it. :-) Overall I'm in favor of this shift as having Drupal updated from within itself is totally weird. A separate application, even a micro-one, is a more stable approach.

1. +++ b/core/includes/bootstrap.inc
   @@ -554,7 +554,9 @@ function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE)
        // Mark this page as being uncacheable.
   -    \Drupal::service('page_cache_kill_switch')->trigger();
   +    if (\Drupal::hasService('page_cache_kill_switch')) {
   +      \Drupal::service('page_cache_kill_switch')->trigger();
   +    }
   

   If we're using an alternate container configuration anyway, why is this needed? Include a page_cache_kill_switch service in the update container and have it be a no-op. This code should not need to know about different "modes" (which is essentially what's being done here).

   That actually applies to a bunch of other things in this patch, too, it looks like. if (service exists Foo) { use Foo } is no different conceptually than if (MAINTENANCE_MODE), which is pushing the conditional far too far down into the system. Having the container means we don't need to do that, we just stub things out in the container.

2. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -55,9 +55,9 @@ class FileSystem implements FileSystemInterface {
   -  public function __construct(StreamWrapperManagerInterface $stream_wrapper_manager, Settings $settings, LoggerInterface $logger) {
   +  public function __construct(StreamWrapperManagerInterface $stream_wrapper_manager, Settings $settings, LoggerInterface $logger = NULL) {
        $this->streamWrapperManager = $stream_wrapper_manager;
        $this->settings = $settings;
        $this->logger = $logger;
   @@ -100,7 +100,9 @@ public function chmod($uri, $mode = NULL) {
   
   @@ -100,7 +100,9 @@ public function chmod($uri, $mode = NULL) {
          return TRUE;
        }
    
   -    $this->logger->error('The file permissions could not be set on %uri.', array('%uri' => $uri));
   +    if ($this->logger) {
   +      $this->logger->error('The file permissions could not be set on %uri.', array('%uri' => $uri));
   +    }
   

   Same idea here: There's a Null logger available that is a no-op. If the constructor parameter is null, default to a new NullLogger instance and then the second conditional is unnecessary.

3. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
   @@ -100,6 +100,7 @@ public function processAttachments(AttachmentsInterface $response) {
    
        $attached = $response->getAttachments();
   +    $attached += ['html_response_placeholders' => []];
    
   

   Why is this line not just moved into getAttachments()? It seems like this is a check we'd need to add in many places potentially, which means it should be in getAttachments() so we have a more normalized pseudo-value-object.

4. +++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
   @@ -0,0 +1,236 @@
   +      $this->moduleList = isset($extensions['module']) ? $extensions['module'] : array();
   

   Total nit: []

5. +++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
   @@ -0,0 +1,236 @@
   +    // Load each module's .update_early_services files.
   +    foreach ($module_filenames as $module => $filename) {
   +      $filename = dirname($filename) . "/$module.services.update.yml";
   

   Comment filename seems out of sync with the filename that's being included.

6. +++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
   @@ -0,0 +1,236 @@
   +   * Boots up the session.
   +   *
   +   * bootSession() + shutdownSession() basically simulates what
   +   * \Drupal\Core\StackMiddleware\Session does.
   

   Is there a reason to not use it then? Or an alternate wrapping object instead of inlining? Splitting this out seems wise as it could then be used by other alternate kernels in the future. (The installer, etc.) If that's not possible for some reason, that should be documented.

7. +++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
   @@ -0,0 +1,236 @@
   +    $request->attributes->set(RouteObjectInterface::ROUTE_OBJECT, new Route('/update.php/{op}', ['op' => 'info'], ['_access' => 'TRUE']));
   

   In most other cases I think we've been breaking a new Route() call to multiple lines for each parameter, for readability.

Also, it seems like the solo route information is spread out across 3 places. There's a RouteProvider, but the _controller equivalent is hard coded in the Kernel, in 2 places. Why not just move all of the info that would normally be on the route to the RouteProvider? We don't need to do full on routing then, but we can generalize the code better and keep the relevant information in one place. At most we hard code a single route name in the kernel but all the data about it is in the RouteProvider.

I have real concerns with adding yet another file outside /core.

Like install.php should it not go in /core? or like cron should it be a route (but without the php extension)?

Even if we use the same service definitions as the runtime container does (which makes sense until one of the two issues from #75 lands), don't we still need to ensure that during update.php, that container is always rebuilt from scratch (not compiled/cached)?

However, #76 would mean that compiler passes (assuming we still want to run them, which I think we need to) must work regardless of db schema state (other than the enabled modules list), which I think is a really good idea anyway, but I'm not sure if core complies 100% with that.

I agree with catch that we should keep thing reviewable and #68 does not make that more simple.
#2550411: Let update.php use its own container in order to have more control of what is executed is a new issue for that.

I expanded the test coverage to test for broken routing, which was exactly the purpose of this issue.

Even if we use the same service definitions as the runtime container does (which makes sense until one of the two issues from #75 lands), don't we still need to ensure that during update.php, that container is always rebuilt from scratch (not compiled/cached)?

We either get rid of using any tags (as we require a compiled container) or accept compilation. I don't see actually a problem to have a different environment passed into the kernel.
This will result into a different compiled container which will not interfere with the existing one.

I have real concerns with adding yet another file outside /core.

Like install.php should it not go in /core? or like cron should it be a route (but without the php extension)?

Well, this is a good way to fix the problem without having to update existing documentation / change other internal stuff. Feel free to open up a new dedicated issue for discussing that. This does not belong IMHO into this particular issuel

Re: update.php at the root level vs. in /core, I would have concerns about it being added at the root level in this issue if its contents had anything that we'd want the ability to change in future minor releases. /core/install.php does have such contents, so I'm glad it's in /core. But the update.php in this patch is the same kind of skeleton as index.php is, so I think it's ok for non-critical followups to discuss moving one or both elsewhere, because if we end up releasing 8.0.0 with them at the root level, I'm not too concerned about us needing to deploy changes to them to sites that install on 8.0.0. And if I'm wrong about that (e.g., if there's a risk of Symfony 3.FOO requiring us to make changes to index.php and update.php), then we should probably have a critical issue to discuss what to do with both, since it's the same problem for both.

Well let's skip the frontpage test, we just care about a running update.php.

diff --git a/core/modules/system/src/Tests/Update/UpdatePathWithBrokenRoutingTest.php b/core/modules/system/src/Tests/Update/UpdatePathWithBrokenRoutingTest.php
index e9b37d1..f783608 100644
--- a/core/modules/system/src/Tests/Update/UpdatePathWithBrokenRoutingTest.php
+++ b/core/modules/system/src/Tests/Update/UpdatePathWithBrokenRoutingTest.php
@@ -32,9 +32,6 @@ public function testWithBrokenRouting() {
     \Drupal::state()->set('update_script_test_broken_inbound', TRUE);
 
     $this->runUpdates();
-    \Drupal::state()->set('update_script_test_broken_inbound', FALSE);
-    $this->drupalGet('<front>');
-    $this->assertResponse(200);
   }
 
 }

This just seems weird. We are testing that the update script can run with a busted router (simulated via an exception in an inbound path processor), but then we can't browse to the front page after we run updates? It really seems like

public function testWithBrokenRouting() {
    // Make sure we can get to the front page.
    $this->drupalGet('<front>');
    $this->assertResponse(200);

    // Simulate a broken router, and make sure the front page is
    // inaccessible.
    \Drupal::state()->set('update_script_test_broken_inbound', TRUE);
    $this->drupalGet('<front>');
    $this->assertResponse(404);

    $this->runUpdates();

    // Remove the simulation of the broken router, and make sure we can get to
    // the front page again.
    \Drupal::state()->set('update_script_test_broken_inbound', FALSE);
    $this->drupalGet('<front>');
    $this->assertResponse(200);
  }

should work, but it won't because UpdatePathTestBase::setUp() overrides WebTestBase::setUp() and the result is that the router isn't installed anyway.

I don't know if anything can be done about this, though. Maybe just a comment on the test to explain the situation?

?

Well, this is basically caused by the fact that we never had an update path for #2509300: Path alias UI allows node/1 and /node/1 as system path then fatals so the configured frontpage is "node" and not "/node" which is what we would need.

Added #2550601: Move update.php to a more acceptable location on the back of this.

Updated the IS to make it clear what we need

Moved that quickly into another issue: #2550615: Update the DB dump to have a leading slash for the frontpage.

There we go.

+++ b/core/modules/system/src/Tests/Update/UpdateScriptTest.php
--- a/core/modules/system/system.routing.yml
+++ b/core/modules/system/system.routing.yml

+++ b/core/modules/system/system.routing.yml
+++ b/core/modules/system/system.routing.yml
@@ -451,13 +451,9 @@ system.batch_page.json:

@@ -451,13 +451,9 @@ system.batch_page.json:
 system.db_update:
   path: '/update.php/{op}'
   defaults:
-    _title: 'Drupal database update'
-    _controller: '\Drupal\system\Controller\DbUpdateController::handle'
     op: 'info'
-  options:
-    _maintenance_access: TRUE
   requirements:
-    _access_system_update: 'TRUE'
+    _access: 'TRUE'

Sorry if I missed something, looked at the comments and did not find an explanation on why do we need this if we have our own front controller on update.php anyway?

The reasons are URL generated pointing to /update.php

That may be worth a comment in the routing.yml file, because that is not a route that would be served from Drupal anymore after this patch, so people looking for it would possibly be confused IMHO.

Mh, its not different to , or in that sense.

There we go.

I think this is ready.

+++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
@@ -0,0 +1,165 @@
+      // to use session API, see \batch_process().

\ can be removed.

looks ready to me too, just some minor observations

1. +++ b/core/modules/system/src/Controller/DbUpdateController.php
   @@ -207,7 +207,7 @@ public function handle($op, Request $request) {
   -  protected function info() {
   +  protected function info(Request $request) {
   

   nit:For some methods we updated the docblock params, but not for others.

2. +++ b/core/modules/system/src/Controller/DbUpdateController.php
   @@ -497,7 +507,7 @@ protected function results() {
   -    $try_again_url = Url::fromRoute('system.db_update', $options)->toString(TRUE)->getGeneratedUrl();
   +    $try_again_url = Url::fromUri(\Drupal::request()->getUriForPath(''))->setOptions(['query' => $options])->toString(TRUE)->getGeneratedUrl();
   

   any reason why we didn't pass the $request along like we did for the other methods?

3. +++ b/core/modules/system/src/Tests/Update/UpdatePathWithBrokenRoutingTest.php
   @@ -0,0 +1,62 @@
   +    $this->assertResponse(500);
   +
   +    // The exceptions are expected. Do not interpret them as a test failure.
   +    // Not using File API; a potential error must trigger a PHP warning.
   +    unlink(\Drupal::root() . '/' . $this->siteDirectory . '/error.log');
   +    foreach ($this->assertions as $key => $assertion) {
   +      if (strpos($assertion['message'], 'core/modules/system/tests/modules/update_script_test/src/PathProcessor/BrokenInboundPathProcessor.php') !== FALSE) {
   +        unset($this->assertions[$key]);
   +        $this->deleteAssert($assertion['message_id']);
   +      }
   +    }
   +
   +    $this->runUpdates();
   +
   +    // Remove the simulation of the broken router, and make sure we can get to
   +    // the front page again.
   +    \Drupal::state()->set('update_script_test_broken_inbound', FALSE);
   +    $this->drupalGet('<front>');
   +    $this->assertResponse(200);
   

   <3

Addressed the feedback.

I tried this patch againest current HEAD + #2464427-157: Replace CacheablePluginInterface with CacheableDependencyInterface. update.php ran smoothly. CacheabilityMetadataUpdateTest also passed like all the other update path tests in HEAD. We discussed this in the critical discussion call today. All the reservations are addressed and remaining are moved to followup to keep the changes minimum here. Given all that this issue is ready. Thanks @dawehner for awesome work.

This is no longer needed for batch, but still for access? Comment needs updating at least.

Yeah its authentication and general other update.php see, see interdiff.

Docs say format_string() but the function body says SafeMarkup::format().

yeah this is just a copy from some other assertion ... I'll go with suggesting strtr().

What's the functional change that actually requires this? I'd expect if there are no pending updates, there'd also be no link?

The freaking logo now links to /update.php

We could probably add a follow-up issue to use internal:// or whichever one it is for unrouted internal links, but this is fine I think for keeping the patch reviewable.

Well, we also discussed to let URLs be generated as if you would visit index.php, see #2548095: Add option to Url() to force the site base_url to be used for more discussion.

I'd be tempted to remove the new assertion, and the asserts, and just add a @todo to assert no link once that issue is fixed. There are positive tests there which are good enough. But if you disagree that's fine too.

Well, I did not wanted to remove the test coverage that all updates got run but yeah I mean we have implicit test coverage, this is for sure.

#100 & #102 got addressed still RTBC.

Quick follow-up for file permissions: #2560603: update.php has 755 file permission (unless I'm missing something stupid)