Remove all remaining usage of deprecated UrlGeneratorInterface::generateFromPath() and delete it

This should fix a lot of the failing tests.

@Wim Leers
Why should we not be able to replace more of those usages?

#11: IIRC from the chat with xjm and pwolanin: because otherwise it'll be deprecated to D9, instead of deprecated sooner.

This fixes the first failure.

In reviewing, I only found nits, that I quickly fixed myself:

1. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
   @@ -25,22 +26,22 @@
   +   * The unrouted url assembler service.
   ...
   +   *   The unrouted url assembler service.
   

   s/url/URL/

2. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
   @@ -25,22 +26,22 @@
   +   * @param \Drupal\Core\Routing\UnroutedUrlAssemblerInterface $url_assembler
   

   Wrong FQCN.

3. +++ b/core/modules/action/src/Plugin/Action/GotoAction.php
   @@ -75,8 +77,17 @@ public static function create(ContainerInterface $container, array $configuratio
   +    // Check for links internal to the site and make them into a URI.
   

   This sounds a bit strange.

1. +++ b/core/modules/action/src/Plugin/Action/GotoAction.php
   @@ -75,8 +77,17 @@ public static function create(ContainerInterface $container, array $configuratio
   +    $uri = $this->configuration['url'];
   +    // Leave external URLs unchanged, convert internal URLsto internal: URIs.
   +    if (!UrlHelper::isExternal($uri)) {
   +      // @todo '<front>' is valid input for BC reasons, may be removed by
   +      //   https://www.drupal.org/node/2421941
   +      if ($uri === '<front>') {
   +        $uri = '';
   +      }
   +      $uri = 'internal:/' . ltrim($uri, '/');
   +    }
   +    $url = Url::fromUri($uri, array('absolute' => TRUE));
   

   Mh, its crazy that we have to do that. Can we at least add a follow up to maybe that such kind of logic accessible via the Url class?

2. +++ b/core/modules/simpletest/src/BrowserTestBase.php
   @@ -617,7 +624,7 @@ protected function drupalLogin(AccountInterface $account) {
   -    $this->drupalGet('user');
   +    $this->drupalGet('/user');
        $this->assertSession()->statusCodeEquals(200);
        $this->submitForm(array(
          'name' => $account->getUsername(),
   @@ -642,7 +649,7 @@ protected function drupalLogout() {
   
   @@ -642,7 +649,7 @@ protected function drupalLogout() {
        // idea being if you were properly logged out you should be seeing a login
        // screen.
        $assert_session = $this->assertSession();
   -    $this->drupalGet('user/logout', array('query' => array('destination' => 'user')));
   +    $this->drupalGet('/user/logout', array('query' => array('destination' => Url::fromUri('base:user')->toString())));
        $assert_session->statusCodeEquals(200);
        $assert_session->fieldExists('name');
   

   Do we really need those changes?

1. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
   @@ -120,16 +120,16 @@ protected function getDestinationAsAbsoluteUrl($destination, $scheme_and_host) {
            // Legacy destination query parameters can be relative paths that have
            // not yet been converted to URLs (outbound path processors and other
            // URL handling still needs to be performed).
   

   Note this.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
   @@ -120,16 +120,16 @@ protected function getDestinationAsAbsoluteUrl($destination, $scheme_and_host) {
   +        $uri = 'base:' . $destination['path'];
   ...
   +        $destination = $this->unroutedUrlAssembler->assemble($uri, $options);
   

   It is converted to using base:, which is converted to an URL by the code in UnroutedUrlAssembler::buildLocalUrl().

   But that code does this WRT outbound path processing:

       // Allow (outbound) path processing, if needed. A valid use case is the path
       // alias overview form:
       // @see \Drupal\path\Controller\PathController::adminOverview().
       if (!empty($options['path_processing'])) {
         // Do not pass the request, since this is a special case and we do not
         // want to include e.g. the request language in the processing.
         $uri = $this->pathProcessor->processOutbound($uri, $options, NULL, $generated_url);
       }
   

   And that option today is only set in … the unit test for UnroutedUrlAssembler::buildLocalUrl().

   Therefore either the code changes or these (unchanged) docs are wrong when this patch is applied.

   (And to confirm that these docs used to be correct: UrlGenerator::generateFromPath(), which was the code used here before, used to always apply path processing, as long as $options['external'] wasn't TRUE.)

3. +++ b/core/modules/contextual/src/Tests/ContextualDynamicContextTest.php
   @@ -136,7 +136,8 @@ function testDifferentPermissions() {
   -    $this->drupalGet('node', ['language' => ConfigurableLanguage::createFromLangcode('it')]);
   +    $url = Url::fromRoute('view.frontpage.page_1', [], ['language' => ConfigurableLanguage::createFromLangcode('it')]);
   +    $this->drupalGet($url);
   

   I think the fact that this had to be changed is further indication/confirmation of my previous point (because WebTestBase::buildUrl() was changed in a similar way); if path processing were enabled this would not have been necessary?

4. +++ b/core/modules/node/src/Tests/NodeTranslationUITest.php
   @@ -307,7 +307,7 @@ function testTranslationRendering() {
   -      $this->drupalGet('node', array('language' => \Drupal::languageManager()->getLanguage($langcode)));
   +      $this->drupalGet(Url::fromRoute('view.frontpage.page_1', array(), array('language' => \Drupal::languageManager()->getLanguage($langcode))));
   

   And again here, and in other places.

5. +++ b/core/modules/system/src/Tests/Routing/DestinationTest.php
   @@ -75,7 +75,7 @@ public function testDestination() {
   -    $this->drupalGet('http://example.com', ['external' => FALSE]);
   +    $this->drupalGet(Url::fromUri('base:/http://example.com'));
   

   Eh… this looks very wrong. :P

Yay! No more integration test changes! This patch looks much better, and is basically only removing generateFromPath() now. Which makes the disruption effectively zero.

IMO this is RTBC if it comes back green.

Watching the test runner at https://dispatcher.drupalci.org/job/default/21233/consoleFull, after about 65% of all tests have run, still not a single failure. RTBC'ing per #63.

since @Wim Leers was ok with rtbc'ing if it was green, and it is... I feel ok with an rtbc.

I talked to @pwolanin about why the fails happened and looked at the interdiff.

... because the path processor OutboundPathProcessorInterface requires a leading slash in the path.

[edit:
I also asked about

+++ b/core/modules/action/src/Plugin/Action/GotoAction.php
@@ -101,7 +120,7 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
-      '#description' => t('The URL to which the user should be redirected. This can be an internal URL like node/1234 or an external URL like @url.', array('@url' => 'http://example.com')),
+      '#description' => t('The URL to which the user should be redirected. This can be an internal URL like /node/1234 or an external URL like @url.', array('@url' => 'http://example.com')),

which seems like the opposite of what the interdiff with the test was doing.

But it is because in the test it is dealing with drupalGet
and in GotoAction, we are dealing with user input (where paths start with /)

this patch is not making everything consistent in that regard, because that was an existing thing, and it is trying to make as few changes as possible.

Notes from my code review:

1. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
   @@ -117,19 +117,18 @@ protected function getDestinationAsAbsoluteUrl($destination, $scheme_and_host) {
   +        // Legacy destination query parameters can be internal paths that have
   ...
   -        $path = $destination['path'];
   +        $uri = 'base:' . $destination['path'];
            $options = [
              'query' => $destination['query'],
              'fragment' => $destination['fragment'],
              'absolute' => TRUE,
            ];
   -        $destination = $this->urlGenerator->generateFromPath($path, $options);
   +        // Treat this as if it's user input of a path relative to the site's
   +        // base URL.
   +        $destination = $this->unroutedUrlAssembler->assemble($uri, $options);
   

   This looks correct and safe to me. UnroutedUrlAssembler::asseble() requires either an external (for external links) or base:, and we are enforcing the latter.

2. +++ b/core/lib/Drupal/Core/Menu/menu.api.php
   @@ -452,7 +452,7 @@ function hook_system_breadcrumb_alter(\Drupal\Core\Breadcrumb\Breadcrumb &$bread
   - *     to either \Drupal\Core\Routing\UrlGenerator::generateFromPath() or
   + *     to either \Drupal\Core\Utility\UnroutedUrlAssembler::assemble() or
   
   @@ -469,7 +469,7 @@ function hook_system_breadcrumb_alter(\Drupal\Core\Breadcrumb\Breadcrumb &$bread
   - * @see \Drupal\Core\Routing\UrlGenerator::generateFromPath()
   + * @see \Drupal\Core\Utility\UnroutedUrlAssembler::assemble()
   

   These changes are because the hook is invoked from LinkGenerator::generate(). Which is still doing some weird stuff with paths, and also using getInternalPath() which was "wishfully deprecated". @pwolanin found #2413701: Url::getInternalPath() is marked deprecated but won't be removed for 8.0.x, so update the docs. Anyway, that method uses Url::toString() which uses the unrouted URL assembler which doesn't use generateFromPath() in HEAD anyway, so these docs were just wrong to begin with.

3. +++ b/core/lib/Drupal/Core/Utility/UnroutedUrlAssembler.php
   @@ -115,11 +115,6 @@ protected function buildLocalUrl($uri, array $options = [], $collect_bubbleable_
   -    // Strip leading slashes from internal paths to prevent them becoming
   -    // external URLs without protocol. /example.com should not be turned into
   -    // //example.com.
   -    $uri = ltrim($uri, '/');
   
   @@ -128,6 +123,10 @@ protected function buildLocalUrl($uri, array $options = [], $collect_bubbleable_
   +    // Strip leading slashes from internal paths to prevent them becoming
   +    // external URLs without protocol. /example.com should not be turned into
   +    // //example.com.
   +    $uri = ltrim($uri, '/');
   

   I didn't know why this hunk was being moved. @pwolanin said it's because the path processor requires the leading slash, and so this is fixing a bug. Normally I'd suggest fixing said bug first in a separate issue, but given the special circumstances, let's file a followup for adding test coverage for said bug.

4. +++ b/core/modules/action/src/Plugin/Action/GotoAction.php
   @@ -54,29 +55,47 @@ class GotoAction extends ConfigurableActionBase implements ContainerFactoryPlugi
   +    $url = $this->configuration['url'];
   +    // Leave external URLs unchanged, and assemble others as absolute URLs
   +    // relative to the site's base URL.
   +    if (!UrlHelper::isExternal($url)) {
   +      $parts = UrlHelper::parse($url);
   +      // @todo '<front>' is valid input for BC reasons, may be removed by
   +      //   https://www.drupal.org/node/2421941
   +      if ($parts['path'] === '<front>') {
   +        $parts['path'] = '';
   +      }
   +      $uri = 'base:' . $parts['path'];
   +      $options = [
   +        'query' => $parts['query'],
   +        'fragment' => $parts['fragment'],
   +        'absolute' => TRUE,
   +      ];
   +      // Treat this as if it's user input of a path relative to the site's
   +      // base URL.
   +      $url = $this->unroutedUrlAssembler->assemble($uri, $options);
   +    }
   

   This is kind of hideous, but not as hideous as leaving the method around, I think. @pwolanin also suggested that we should add a followup for test coverage for actions.

5. +++ b/core/modules/simpletest/src/BrowserTestBase.php
   @@ -404,7 +406,14 @@ protected function drupalGet($path, array $options = array()) {
   +        $options['path_processing'] = TRUE;
   
   +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -2684,12 +2684,18 @@ protected function drupalGetMails($filter = array()) {
   +      $options['path_processing'] = TRUE;
   
   @@ -2950,8 +2956,19 @@ protected function buildUrl($path, array $options = array()) {
   +        $options['path_processing'] = !$force_internal;
   

   @alexpott expressed some concern about needing to opt in for path processing, but I don't think this patch is the cause of that, and it does appear to be all for supporting the web test functionality.

6. +++ b/core/modules/simpletest/tests/src/Functional/BrowserTestBaseTest.php
   @@ -34,7 +34,7 @@ public function testGoTo() {
   -    $this->drupalGet('/test-page');
   +    $this->drupalGet('test-page');
   
   @@ -46,7 +46,7 @@ public function testGoTo() {
   -    $this->drupalGet('/form-test/object-builder');
   +    $this->drupalGet('form-test/object-builder');
   

   er what? shouldn't these be the other way around? because we require a leading slash now?

   I asked @pwolanin why this was and he said it's because BrowserTestBase seems to be agnostic about whether it gets a leading slash; some assertions had them and others didn't. The changes above make it consistent with WebTestBase.

   I'd suggest a followup (it may already exist) to expect a leading slash instead, since we expect that in D8 in general, but that would require massive test changes so not needed now.

Looking over the CRs and such next.

+++ b/core/modules/simpletest/src/BrowserTestBase.php
@@ -412,7 +412,7 @@ protected function drupalGet($path, array $options = array()) {
+        $url = Url::fromUri('base:/' . $path, $options)->toString();

+++ b/core/modules/simpletest/tests/src/Functional/BrowserTestBaseTest.php
@@ -34,7 +34,7 @@ public function testGoTo() {
-    $this->drupalGet('/test-page');

@@ -46,7 +46,7 @@ public function testGoTo() {
-    $this->drupalGet('/form-test/object-builder');

Instead of changing the ->drupalGet calls, could we just use ltrim($path, '/') in drupalGet method

Let's also address #70 in a quick followup. The disruption should be low for now and I think it's much more important to get this in. We also should have followups for #69 points 3, 4, and six, and maybe there is an existing issue for point 5 but I don't know anything about it.

@YesCT and @pwolanin looked at the existing CRs and updated them for this change, and @pwolanin also posted https://www.drupal.org/node/2581455, which I have now published.

Committed and pushed to 8.0.x. Thanks everyone!

Created follow up for #70: #2581557: Add ltrim($path, '/') in drupalGet method

Thanks, Jeroen!

Created the following issues:

• Follow up for #69.3: #2581795: Add test for UnroutedUrlAssembler::buildLocalUrl()
• Follow up for #69.4: #2581799: Add test coverage for GotoAction
• Follow up for #69.5: #2581943: Is it necessary to opt in for path processing?
• Follow up for #69.6: #2581803: BrowserTestBase::drupalGet() not always expects an URL that starts with a slash.

All concerns mentioned by @xjm in #71 now have a separate issue, so removing the needs followup tag.