update.php should not process path aliases

+++ b/core/lib/Drupal/Core/Update/UpdateServiceProvider.php
@@ -32,6 +32,8 @@ public function alter(ContainerBuilder $container) {
+    $container->register('path_processor_manager', 'Drupal\Core\PathProcessor\NullPathProcessorManager');

+++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
@@ -327,6 +327,9 @@ protected function runUpdates() {
+    $this->initKernel($request, '\Drupal\Core\Update\UpdateKernel');

makes sense to use ::class to populate "use" with real usage of class

I do agree. executing less APIs on update time reduces in general the chance that stuff goes wrong. I'm wondering whether we should open up an issue to talk about other APIs we could basically disable at that time.

1. +++ b/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php
   @@ -375,12 +375,15 @@ protected function initUserSession() {
   +  protected function initKernel(Request $request, $kernel_class = NULL) {
   +    $kernel_class = $kernel_class ?: DrupalKernel::class;
   

   Couldn't you use $kernel_class = DrupalKernel::class instead here?

2. +++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
   @@ -327,6 +327,9 @@ protected function runUpdates() {
   +    $request = Request::createFromGlobals();
   +    $this->initKernel($request, '\Drupal\Core\Update\UpdateKernel');
   

   Let's add some documentation why we choose this particular kernel.

I think it makes a ton of sense that update.php does not try to resolve path aliases; it ought to depend on as little as possible.

Using NullPathProcessorManager makes sense.

AFAICT we need to change UpdatePathTestBase because it's actually already wrong in HEAD, it just happens to be that we never noticed problems caused by that bug. Correct?
I have to admit I don't understand why failures are happening or why #11 is fixing some of those failures. It's probably related to now finally using the correct kernel? In that case, wouldn't it be simpler to call \Drupal::setContainer($this->container) somewhere in UpdatePathTestBase?

There is no properer way to fix this IMHO

We are now creating two different container, which means $this->container is getting out of sync. And $this->container is the old one, so it would be the opposite direction than what #12 proposed. Which already seems to happen, or getting it again from $this->container wouldn't work.

But because specific services are kept as variables we have to update them once there is a new container.

Hah!

+++ b/core/lib/Drupal/Core/Update/UpdateServiceProvider.php
@@ -32,6 +32,8 @@ public function alter(ContainerBuilder $container) {
+    $container->register('path_processor_manager', 'Drupal\Core\PathProcessor\NullPathProcessorManager');

+++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
@@ -166,7 +166,7 @@ protected function setUp() {
-    $this->updateUrl = Url::fromRoute('system.db_update');
+    $this->updateUrl = Url::fromRoute('system.db_update', [], ['path_processing' => FALSE]);

Reading this … you'd think that either of these changes would be sufficient.

Why do we need both changes?

Alright. Damn that's tricky!

+++ b/core/lib/Drupal/Core/Update/UpdateServiceProvider.php
@@ -32,6 +32,8 @@ public function alter(ContainerBuilder $container) {
+    $container->register('path_processor_manager', 'Drupal\Core\PathProcessor\NullPathProcessorManager');

Nit: Let's use NullPathProcessorManager::class. (@andypost also said this in #3.)

Bringing back a hunk from #8 to fix the failures.

Addressed #3/#19.

np!

This looks ready to me, just some observations.

1. +++ b/core/modules/user/src/Plugin/LanguageNegotiation/LanguageNegotiationUserAdmin.php
   @@ -70,12 +70,12 @@ class LanguageNegotiationUserAdmin extends LanguageNegotiationMethodBase impleme
       *   The path processor manager.
   

   This comment is outdated now.

2. +++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
   @@ -166,7 +166,7 @@ protected function setUp() {
   +    $this->updateUrl = Url::fromRoute('system.db_update', [], ['path_processing' => FALSE]);
   

   It is worth adding comment why path processing is set to false.

Thanks, it's ready.

One question I have is what happens to URLs that are generated in update messages? As far as I know we have a few. Are we sure that we don't expect these to the processed?

#26.2 was for a comment in UpdatePathTestBase, I think we need one there too.

Discussed a bit with @amateescu in slack. I was wondering if we could add a test that asserts that update messages with urls do not use aliases and that the same url is aliases afterwards. It might not be possible but I think it is worth trying.

Discussed a bit more with @amateescu - he's going to try to use SQL to create the alias instead of using the mock. Yep we'll need to update this SQL when we move from 8.x to 9.x because we'll no longer have the path alias migration but if we don't do that then we would lose explicit test coverage of this change.

@amateescu the test looks good to me - thanks agin for adding this coverage.

Committed 39578f4 and pushed to 8.7.x. Thanks!

Only committed to 8.7.x because whilst this is a bug fix the bug is only really exposed by work on-going in 8.7.x therefore I don't see any advantage to committing to 8.6.x. I might be wrong and if so we can commit #40 to 8.6.x.

diff --git a/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBaseTest.php b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBaseTest.php
index 57ab172fc5..535d271d2e 100644
--- a/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBaseTest.php
+++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBaseTest.php
@@ -5,8 +5,6 @@
 use Drupal\Component\Utility\Html;
 use Drupal\Component\Render\FormattableMarkup;
 use Drupal\Core\Database\Database;
-use Drupal\system\Tests\Routing\MockAliasManager;
-use Drupal\user\Entity\Role;
 
 /**
  * Tests the update path base class.

Removed unused uses on commit.

This issue created a bug where menu links that link to internal paths by alias will cause those paths to 404 after database updates are run (either via update.php or drush). I provided detailed steps to reproduce in the issue: #3075675: Route cache can become poisoned with invalid data during database updates, resulting in 404s.