Convert most usages of EntityInterface::getSystemPath() to use routes

I guess it makes sense to wait for the #type link issue before looking at this. Many usages in views related to that I think, possibly fixed there already, then a few interesting special cases, but most usage is in tests.

So, the views path stuff is still there.

Another thing is the image_formatter path.

And, the path alias stuff, see PathWidget::formElement().

Most other calls should be removed.

Well, that was the wrong patch. Here is the patch that I wanted to upload and an additional one that includes image formatter conversion.

Cheating a bit, but allows destination to work correctly with external-local URL's

1. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
   @@ -55,16 +55,21 @@ public function checkRedirectUrl(FilterResponseEvent $event) {
          //   base path) are allowed.
   -      if ($destination && (!UrlHelper::isExternal($destination) || UrlHelper::externalIsLocal($destination, $GLOBALS['base_url']))) {
   -        $destination = UrlHelper::parse($destination);
   +      if ($destination) {
   +        if (!UrlHelper::isExternal($destination)) {
   +          $destination = UrlHelper::parse($destination);
    
   

   I can haz tests?

2. +++ b/core/modules/content_translation/src/Tests/ContentTranslationUITest.php
   @@ -68,23 +69,26 @@ protected function doTestBasicTranslation() {
   -    $path = $langcode . '/' . $content_translation_path . '/add/' . $default_langcode . '/' . $langcode;
   +    $content_translation_path = $entity->url('drupal:content-translation-overview', array('language' => $language));
   +    $path = $content_translation_path . '/add/' . $default_langcode . '/' . $langcode;
        // This does not save anything, it merely reloads the form and fills in the
   

   Filled a follow up: #2363459: Make it possible to provide 'langcode' in $options for URL generation instead of Language object

3. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
   @@ -98,15 +98,14 @@ public function settingsSummary() {
          if (!$entity->isNew()) {
   -        // @todo Remove when theme_image_formatter() has support for route name.
   -        $uri['path'] = $entity->getSystemPath();
   -        $uri['options'] = $entity->urlInfo()->getOptions();
   +        $uri = $entity->url();
          }
        }
        elseif ($image_link_setting == 'file') {
   

   Lovely!

Not sure I like all this stuff.

Fixing translation UI tests and the $path[0] notices.

Grml ;)

Fixed a few tests, the image formatter stuff is still all weird, we should extract that in a separate issue and figure out a good solution there.

Some feedback on some of the changes here would be great, if I'm on the right track or going crazy... :)

1. +++ b/core/modules/config/src/Tests/ConfigSingleImportExportTest.php
   @@ -134,7 +134,7 @@ public function testImportSimpleConfiguration() {
        $this->drupalPostForm(NULL, array(), t('Confirm'));
   -    $this->drupalGet('/');
   +    $this->drupalGet('');
        $this->assertText('Test simple import');
      }
   
   +++ b/core/modules/contact/src/Tests/ContactPersonalTest.php
   @@ -86,7 +86,7 @@ function testSendPersonalContactMessage() {
   -    $this->drupalGet('/admin/reports/dblog');
   +    $this->drupalGet('admin/reports/dblog');
        $placeholders = array(
   
   +++ b/core/modules/system/src/Tests/ParamConverter/UpcastingTest.php
   @@ -76,7 +76,7 @@ public function testEntityLanguage() {
   -    $this->drupalGet("/paramconverter_test/node/" . $node->id() . "/test_language");
   +    $this->drupalGet("paramconverter_test/node/" . $node->id() . "/test_language");
   
   +++ b/core/modules/system/src/Tests/Routing/RouterTest.php
   @@ -119,7 +119,7 @@ public function testDynamicRoutes() {
   -    $this->drupalGet('/router_test/test10');
   +    $this->drupalGet('router_test/test10');
   

   Yeah just in case ... this is kinda out of scope. We explicit add kinda support for it, or at least should. It is though worth to do that in a none critical issue.

2. +++ b/core/modules/content_translation/src/Tests/ContentTranslationUITest.php
   @@ -68,23 +69,26 @@ protected function doTestBasicTranslation() {
   +    $language = ConfigurableLanguage::load($langcode);
   ...
   +    $content_translation_path = $entity->url('drupal:content-translation-overview', array('language' => $language));
   

   ... Opened up #2363459: Make it possible to provide 'langcode' in $options for URL generation instead of Language object in order to fix that non-perfect API.

3. +++ b/core/modules/image/src/Tests/ImageThemeFunctionTest.php
   @@ -104,18 +103,6 @@ function testImageFormatterTheme() {
   -
   -    // Link the image to a fragment on the page, and not a full URL.
   -    $fragment = $this->randomMachineName();
   -    $element = $base_element;
   -    $element['#path']['path'] = '';
   -    $element['#path']['options'] = array(
   -      'external' => TRUE,
   -      'fragment' => $fragment,
   -    );
   -    $this->drupalSetContent(drupal_render($element));
   -    $elements = $this->xpath('//a[@href=:fragment]/img[@class="image-style-test" and @src=:url and @width=:width and @height=:height and @alt=""]', array(':fragment' => '#' . $fragment, ':url' => $url, ':width' => $image->getWidth(), ':height' => $image->getHeight()));
   -    $this->assertEqual(count($elements), 1, 'theme_image_formatter() correctly renders a link fragment.');
   

   I could easily imagine this to be a usecase ... are we sure we cannot support it?

4. +++ b/core/modules/rest/src/Tests/AuthTest.php
   @@ -83,18 +84,18 @@ public function testRead() {
   -  protected function basicAuthGet($path, $username, $password) {
   +  protected function basicAuthGet(Url $url, $username, $password) {
   

   This is totally okay given that this is not a base level test API

5. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1449,9 +1449,14 @@ protected function isInChildSite() {
      protected function drupalGet($path, array $options = array(), array $headers = array()) {
   ...
   +    // Assume that paths that start with a / are already fully generated
   +    // URL's that are just missing $base_root.
   

   I kinda thing we should update the documentation of drupalGet()

6. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1449,9 +1449,14 @@ protected function isInChildSite() {
   +    if (isset($path[0]) && $path[0] == '/') {
   

   I actually kinda like how php can deal with strings.

7. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1449,9 +1449,14 @@ protected function isInChildSite() {
          $url = $this->container->get('url_generator')->generateFromPath($path, $options);
   

   It is hilarious that we use a deprecated method as central point of our testing infrastructure.

8. +++ b/core/modules/system/src/Tests/Cache/PageCacheTagsTestBase.php
   @@ -7,6 +7,7 @@
    
   @@ -53,7 +54,8 @@ protected function verifyPageCache($path, $hit_or_miss, $tags = FALSE) {
   
   @@ -53,7 +54,8 @@ protected function verifyPageCache($path, $hit_or_miss, $tags = FALSE) {
   -      $cid_parts = array(_url($path, array('absolute' => TRUE)), 'html');
   +      $absolute_url = UrlHelper::isExternal($path) ? $path : _url($path, array('absolute' => TRUE));
   +      $cid_parts = array($absolute_url, 'html');
   

   Filled a follow up here: #2372899: PageCacheTagsTestBase should use Url objects

1. No it is not out of scope as it conflicts with the added leading / logic, this now assumes that it is a full url that does not need the prefix anymore, so it breaks badly. If there are better ways for that, great, but this is what we discussed and so we need it for now.
3. I removed the test because we now just have a url string that is passed through and not processed in any way. If you add 'drupal is awesome' there, it will happily print that out as a link. So testing it like that is pointless IMHO.
5. Sure we should, most of the stuff in this issue is very experimental, not going to bother with good documentation before we have a green patch and agreed on the approach. Btw, this is exactly the code that requires the changes in 1.
7. Yes :)

Also, we need to split those changes up, the image stuff needs to be a separate issue, and it's not correct yet, stuff is still breaking.

Just a re-roll for now.

5. Sure we should, most of the stuff in this issue is very experimental, not going to bother with good documentation before we have a green patch and agreed on the approach. Btw, this is exactly the code that requires the changes in 1.

Alright, not a problem at all

Re-roll.

Going to continue on the path that has been taken, will see if I can get some progress done on the NodeTranslationUiTest. This is a very slow test to run so it would be good to get this over with.

Wasn't able to make any meaningful progress. The code sprint is at its end now, unassigning as I won't have much time in the near future.

Apart from these testbot details, this looks really great!

1. +++ b/core/modules/comment/src/CommentManager.php
   @@ -263,7 +263,7 @@ public function forbiddenMessage(EntityInterface $entity, $field_name) {
   -        $destination = array('destination' => $entity->getSystemPath() . '#comment-form');
   +        $destination = array('destination' => $entity->url('canonical', array('absolute' => TRUE)) . '#comment-form');
   

   instead you could also use $entity->urlInfo()->setAbsolute()->toString() (with toString() being optional).

2. +++ b/core/modules/comment/src/Controller/CommentController.php
   @@ -128,7 +128,7 @@ public function commentPermalink(Request $request, CommentInterface $comment) {
   -      $redirect_request = Request::create($entity->getSystemPath(), 'GET', $request->query->all(), $request->cookies->all(), array(), $request->server->all());
   +      $redirect_request = Request::create($entity->url(), 'GET', $request->query->all(), $request->cookies->all(), array(), $request->server->all());
   

   I'm a little bit suprised that it used to work :)

3. +++ b/core/modules/image/image.field.inc
   @@ -79,14 +79,4 @@ function template_preprocess_image_formatter(&$variables) {
      }
   -
   -  // The link path and link options are both optional, but for the options to be
   -  // processed, the link path must at least be an empty string.
   -  // @todo Add support for route names.
   -  $variables['url'] = NULL;
   -  if (isset($variables['path']['path'])) {
   -    $path = $variables['path']['path'];
   -    $options = isset($variables['path']['options']) ? $variables['path']['options'] : array();
   -    $variables['url'] = _url($path, $options);
   -  }
   

   This is one of the remaining one in the other _url issue: yeah!!

4. +++ b/core/modules/image/src/Tests/ImageThemeFunctionTest.php
   @@ -104,18 +103,6 @@ function testImageFormatterTheme() {
   -
   -    // Link the image to a fragment on the page, and not a full URL.
   -    $fragment = $this->randomMachineName();
   -    $element = $base_element;
   -    $element['#path']['path'] = '';
   -    $element['#path']['options'] = array(
   -      'external' => TRUE,
   -      'fragment' => $fragment,
   -    );
   -    $this->drupalSetContent(drupal_render($element));
   -    $elements = $this->xpath('//a[@href=:fragment]/img[@class="image-style-test" and @src=:url and @width=:width and @height=:height and @alt=""]', array(':fragment' => '#' . $fragment, ':url' => $url, ':width' => $image->getWidth(), ':height' => $image->getHeight()));
   -    $this->assertEqual(count($elements), 1, 'theme_image_formatter() correctly renders a link fragment.');
      }
   

   Is there a reason why we removed that test coverage? I would assume that we say that we just support everything \Drupal\Core\Url anyway?

5. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
   @@ -79,14 +81,22 @@ protected function httpRequest($url, $method, $body = NULL, $mime_type = NULL) {
   +    else {
   +      $url = _url($url, array('absolute' => TRUE));
   +    }
   

   Do we still need this here?

Thanks.

1. Not sure that's really easier?
4. You asked the same a month ago in #17.3, see explanation in #18 :)
5. There some URL's where it is not easy to get a route for, like dblog/ID and 'entity/' . $entity_type, as they are dynamically defined in rest resource plugins.

Looking at the test failures...

Having trouble running those tests with a prefix locally but let's see what this fixes.

@dawehner: Can you have a look at the failing unit and views test? I don't think that what the unit test is testing still makes sense with the updated implementation and no idea what the views test is doing exactly :)

Oh, #26.2 actually does *not* work when you install drupal in a subdirectory. I have no idea how HEAD is passing with that, but I had a test fail in my project when going to comment/ID#comment-ID.

Fixed some test failures.

There we go (hopefully)

There we go.

I'm fine with the patch, it would be great if someone else coudl review my recent test fixes.

Other then this minor issue i think it is RTBC

+++ b/core/lib/Drupal/Core/Routing/RequestContext.php
@@ -27,4 +35,30 @@ public function fromRequestStack(RequestStack $request_stack) {
+    // @todo Extract the code in DrupalKerne::initializeRequestGlobals.

Can we add a issue link to this?

Note that we still have ~20 calls to getSystemPath() left.

Almost all of the them are in views link plugins. I assume we have to refactor how that works before we can get rid of it, and we will have to when/if we do that.

Another thing are path aliases, for which we have no solution but a major issue open somewhere.

Two more calls we can get rid of I think:

- EntityWithUriCacheTagsTestBase
- ConfigEntityTest

Which I think means we need an issue summary/plan what to do with getSystemPath() exactly.

My suggestion: Keep the method, use this issue to help getting rid of _url() calls (which also helps to explain why this is critical), deal with remaining calls in case we refactor the mentioned sub-components in separate issues.

Which I think means we need an issue summary/plan what to do with getSystemPath() exactly.

Well, there are usecases indeed where you want to get the system path (url aliases is a classical example).

Alright, let's start again with a reroll first.

Let's get rid of 2 uses.

And this time with the fixes of the failures. Sorry for the noise!

There are a few things in here that need a change record, probably separate ones I think

- Changed behavior with leading / for drupalGet()/Post() (must be an absolute url then)
- image formatter changes

Not sure if the base url thing also counts as that, it might make sense to deprecate it but not remove in 8.x? not sure if it needs a change record then.

Your changes look good to me in general but I won't be able to RTBC this as I've written large parts of it myself.

While writing the change record for the image formatter I realised that we should rather use the URL object, so for example a preprocess function could still preprocess it,
like adding a new query attribute.

Ah, Url objects have __toString(), right, then we could print it directly in twig? Works for me..

Yeah exactly, it will directly work.

So we need the following additional adaptions for that.

Looks good to me. I noticed that image-formatter.html.twig documentation still mentiones path, that should be removed there.

Also wondering if we want to restore the fragment test now that I removed, not sure.

For the fragment bit we need #2401395: <none> does not work as expected with fragments

Alright, let's do it!

Just a couple of naming comments...

1. +++ b/core/lib/Drupal/Core/Routing/RequestContext.php
   @@ -19,6 +20,13 @@
   +   * The scheme, host and base URL, for example "http://example.com/d8".
   

   The scheme, host and base path?

2. +++ b/core/lib/Drupal/Core/Routing/RequestContext.php
   @@ -19,6 +20,13 @@
   +  protected $completeBaseUrl;
   

   Maybe just $baseUrl?

3. +++ b/core/lib/Drupal/Core/Routing/RequestContext.php
   @@ -27,4 +35,30 @@ public function fromRequestStack(RequestStack $request_stack) {
   +   * Gets the scheme, host and base URL.
   

   again, base path?

Thank you for your review!

The scheme, host and base path?

Well, if you look at the implementation you will see

Maybe just $baseUrl?

There is already a different concept of base url in the request context implementation of symfony.

PhpStorm is reporting 17 usages of EntityInterface::getSystemPath(), and found the string '->getSystemPath' 23 times in code. The method is still in the interface definition, and there are 3 method implementations.

Should these be part of the patch, or are they excluded and being handled in followups? If the actual removal of the method is part of the patch, should there be a CR?

Can't see much to nitpick here, best I could come up with:

1. +++ b/core/lib/Drupal/Core/Routing/RequestContext.php
   @@ -27,4 +35,30 @@ public function fromRequestStack(RequestStack $request_stack) {
   +    // @todo Extract the code in DrupalKerne::initializeRequestGlobals.
   

   %s/Kerne/Kernel

2. +++ b/core/modules/config_translation/src/Tests/ConfigTranslationUiTest.php
   @@ -326,7 +326,7 @@ public function testContactConfigEntityTranslation() {
   +        $this->drupalGet(ltrim("$langcode_prefix/$translation_base_url/$langcode/edit", ' /'));
   

   Extra space?

3. +++ b/core/modules/entity_reference/src/Tests/EntityReferenceFieldTranslatedReferenceViewTest.php
   @@ -121,6 +121,7 @@ protected function setUp() {
   +    $this->rebuildContainer();
   

   Perhaps a comment as to why this is needed?

Fixes #60 and #61, points 1 and 2 - still needs addressing #61.3

Hoping didn't break stuff.

looking at fails

getSystemPath() can not be removed here. see #41.

Paths are a tricky one, hope this is moving in the right direction.
All I have time for today.

Yes, they are :)

We can *not* replace those remaining calls. url() returns the full URL, including prefix, so on testbot, it is /checkout/node/1, while systemPath() is always node/1.

There is no way that this is going to work without *somehow* refactoring the alias system first, which I don't think is going to happen in 8.x. We need to go back to #59 + other fixes that you worked on.

doing so

Backed out stuff, interdiff against #59, mostly minor stuff.

1. +++ b/core/modules/content_translation/src/Tests/ContentTranslationWorkflowsTest.php
   @@ -142,33 +143,37 @@ protected function assertWorkflows(UserInterface $user, $expected_status) {
   -    $options = array('language' => $languages[$langcode]);
   +    $options['language'] = $languages[$langcode];
   

   Are we extending an old $options array? If not can we keep the old version.

2. +++ b/core/modules/content_translation/src/Tests/ContentTranslationWorkflowsTest.php
   @@ -142,33 +143,37 @@ protected function assertWorkflows(UserInterface $user, $expected_status) {
   +    $options = ['language' => $languages[$langcode], 'absolute' => TRUE];
   

   Why do we set a new $options array? As far as I can see this is not used any more.

3. +++ b/core/modules/entity_reference/src/Tests/EntityReferenceFieldTranslatedReferenceViewTest.php
   @@ -121,6 +121,7 @@ protected function setUp() {
   +    $this->rebuildContainer();
   

   Why is this line added to the patch?

4. +++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyDefaultArgumentTest.php
   @@ -31,7 +31,10 @@ class TaxonomyDefaultArgumentTest extends TaxonomyTestBase {
   +    $request->server->set('SCRIPT_NAME', $GLOBALS['base_path'] . 'index.php');
   +    $request->server->set('SCRIPT_FILENAME', 'index.php');
   
   @@ -44,7 +47,10 @@ public function testNodePath() {
   +    $request->server->set('SCRIPT_NAME', $GLOBALS['base_path'] . 'index.php');
   +    $request->server->set('SCRIPT_FILENAME', 'index.php');
   

   Why are these lines added to the patch?

5. +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php
   @@ -60,19 +45,25 @@ public function testDestinationRedirect(Request $request, $expected) {
   -        ->expects($this->once())
   +        ->expects($this->any())
   

   Why change it from once to any?

6. +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php
   @@ -8,6 +8,7 @@
   +use Drupal\Core\Routing\RequestContext;
   ...
   @@ -60,19 +45,25 @@ public function testDestinationRedirect(Request $request, $expected) {
   +    $request_context = new RequestContext();
   +    $request_context->fromRequest($request);
   +    $request_context->setCompleteBaseUrl('http://example.com/drupal');
   

   Do not use the RequestContext class directly. Create a mock.

Thank you for your review!

Are we extending an old $options array? If not can we keep the old version.

Well, we want to include absolute everywhere now, so ... meh, we could redefine them.

Why is this line added to the patch?

Note: Previously we generated the expected URl like that:

    $path = $this->referrerEntity->getSystemPath();
    $translation_path = $this->translateToLangcode . '/' . $path;

so we bypassed the API for url generation basically.

With this patch we use the api (->url()) which then requires an up to date language manager as well as language path processors, so we rebuilt the container for that.

Why are these lines added to the patch?

Well, we need to setup a proper fake request now that we use the proper API.

Why change it from once to any?

Because the implementation itself changed ... and we no longer call the method in every case.

Do not use the RequestContext class directly. Create a mock.

Thank you for being so pragmatic :)

BTW, I haven't done any work on this patch, so I will do a proper review it later today. It pretty much looked RTBC last night.

1. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
   @@ -79,14 +81,22 @@ protected function httpRequest($url, $method, $body = NULL, $mime_type = NULL) {
   +    else {
   +      $url = _url($url, array('absolute' => TRUE));
   +    }
   +
   

   +++ b/core/modules/system/src/Tests/Cache/PageCacheTagsTestBase.php
   @@ -53,7 +54,8 @@ protected function verifyPageCache($path, $hit_or_miss, $tags = FALSE) {
   +      $absolute_url = UrlHelper::isExternal($path) ? $path : _url($path, array('absolute' => TRUE));
   +      $cid_parts = array($absolute_url, 'html');
   

   Was going to nit about introducing two new uses of _url(), but these can be handled in the issues that are getting rid of them.

Code changes look good; test changes look good. Read through comments a few times which justify decisions the got made. The patch is green. Not seeing anything to prevent RTBC.

1. +++ b/core/lib/Drupal/Core/Routing/RequestContext.php
   @@ -19,12 +20,52 @@
   +    // @todo Extract the code in DrupalKernel::initializeRequestGlobals.
   +    if (isset($GLOBALS['base_url'])) {
   +      $this->setCompleteBaseUrl($GLOBALS['base_url']);
   +    }
   

   Can we create a followup for the @todo.

2. +++ b/core/modules/content_translation/src/Tests/ContentTranslationWorkflowsTest.php
   @@ -142,33 +143,37 @@ protected function assertWorkflows(UserInterface $user, $expected_status) {
   +    $options['language'] = $languages[$langcode];
   

   We are expending the $options array. With which $options array are we testing. It is not clear to me.

   $options = ['language' => $languages[$langcode], 'absolute' => TRUE];
   OR
   $options = ['language' => $languages[$langcode]];
   

3. +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php
   @@ -8,6 +8,7 @@
   +use Drupal\Core\Routing\RequestContext;
   

   This line is not needed any more. Can you remove it.

4. +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php
   @@ -60,19 +45,28 @@ public function testDestinationRedirect(Request $request, $expected) {
   -        ->expects($this->once())
   +        ->expects($this->any())
   

   I do not see why this change is necessary. If I am wrong about this, can you explain it to me. Your previous explanation was for me not clear enough. Sorry.
   

   Because the implementation itself changed ... and we no longer call the method in every case.

Can we create a followup for the @todo.

#2403967: DrupalKernel should setup the request context

We are expending the $options array. With which $options array are we testing. It is not clear to me.

Alright, fixed it.

I do not see why this change is necessary. If I am wrong about this, can you explain it to me. Your previous explanation was for me not clear enough. Sorry.

Here is the new code:

-      if ($destination && (!UrlHelper::isExternal($destination) || UrlHelper::externalIsLocal($destination, $GLOBALS['base_url']))) {
-        $destination = UrlHelper::parse($destination);
+      if ($destination) {
+        if (!UrlHelper::isExternal($destination)) {
+          $destination = UrlHelper::parse($destination);
 
-        $path = $destination['path'];
-        $options['query'] = $destination['query'];
-        $options['fragment'] = $destination['fragment'];
-        // The 'Location' HTTP header must always be absolute.
-        $options['absolute'] = TRUE;
+          $path = $destination['path'];
+          $options['query'] = $destination['query'];
+          $options['fragment'] = $destination['fragment'];
+          // The 'Location' HTTP header must always be absolute.
+          $options['absolute'] = TRUE;
 
-        $response->setTargetUrl($this->urlGenerator->generateFromPath($path, $options));
+          $response->setTargetUrl($this->urlGenerator->generateFromPath($path, $options));
+        }
+        elseif (UrlHelper::externalIsLocal($destination, $this->requestContext->getCompleteBaseUrl())) {
+          $response->setTargetUrl($destination);
+        }
       }

Even if it is hard to see, but there is an elseif() in there ... which means that $this->urlGenerator->generateFromPath() itself is not called every time.
Note, at the same time we expanded the test coverage, so we also get into the new elseif() in the test

Only a few very silly nits

1. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
   @@ -55,16 +59,21 @@ public function checkRedirectUrl(FilterResponseEvent $event) {
   +          // The 'Location' HTTP header must always be absolute.
   

   s/be absolute/contain an absolute URL/

2. +++ b/core/lib/Drupal/Core/Routing/RequestContext.php
   @@ -19,12 +20,52 @@
   +   * For example on a subdir installation "d8" it should be
   

   For example, in an installation in a subdirectory "d8", it should be

3. +++ b/core/modules/entity_reference/src/Tests/EntityReferenceFieldTranslatedReferenceViewTest.php
   @@ -121,6 +121,7 @@ protected function setUp() {
   +    $this->rebuildContainer();
   

   Could use what you wrote in #72:

   With this patch we use the api (->url()) which then requires an up to date language manager as well as language path processors, so we rebuilt the container for that.

4. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1470,11 +1470,15 @@ protected function isInChildSite() {
   +    // Assume that paths that start with a / are already fully generated
   +    // URL's that are just missing $base_root.
   

   We probably want to see this documented in ::drupalGet()'s docblock?

5. +++ b/core/modules/system/src/Tests/Cache/PageCacheTagsTestBase.php
   @@ -53,7 +54,8 @@ protected function verifyPageCache($path, $hit_or_miss, $tags = FALSE) {
   -      $cid_parts = array(_url($path, array('absolute' => TRUE)), 'html');
   +      $absolute_url = UrlHelper::isExternal($path) ? $path : _url($path, array('absolute' => TRUE));
   +      $cid_parts = array($absolute_url, 'html');
   

   There already is an issue to clean this up, at #2372899: PageCacheTagsTestBase should use Url objects :) I'll probably take that on at a later time.

The CR looks ready: https://www.drupal.org/node/2399171

All that is missing now AFAICT is an issue to address the remaining EntityInterface::getSystemPath() calls (particularly those in Views) and remove that method — see #41.

s/be absolute/contain an absolute URL/

Fixed

For example, in an installation in a subdirectory "d8", it should be

Fixed

Could use what you wrote in #72:

Expanded that part of the documentation

We probably want to see this documented in ::drupalGet()'s docblock?

Fixed as well

There already is an issue to clean this up, at #2372899: PageCacheTagsTestBase should use url objects :) I'll probably take that on at a later time.

Can we keep that hunk in for now?

All that is missing now AFAICT is an issue to address the remaining EntityInterface::getSystemPath() calls (particularly those in Views) and remove that method — see #41.

The issue is NOT about remove all calls

Can we keep that hunk in for now?

Oh, absolutely, I didn't want you to remove it!

The issue is NOT about remove all calls

I know! I'm asking where the issue for the next step is, where we do remove all calls. But that's something to add to the issue, not to the patch. Hence: RTBC :)

I know! I'm asking where the issue for the next step is, where we do remove all calls. But that's something to add to the issue, not to the patch. Hence: RTBC :)

Well yeah, there needs to be an issue which talkes the ONE _url call inside views field plugin to support url objects properly and then at the same rewrite all those usages of paths in views (2fun").

1. +++ b/core/lib/Drupal/Core/Routing/RequestContext.php
   @@ -19,12 +20,52 @@
   +    // @todo Extract the code in DrupalKernel::initializeRequestGlobals.
   +    if (isset($GLOBALS['base_url'])) {
   +      $this->setCompleteBaseUrl($GLOBALS['base_url']);
   +    }
   

   Should we be doing this in this issue or another issue? Can we get a follow up created.

2. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
   @@ -7,7 +7,9 @@
   +use Drupal\Component\Utility\UrlHelper;
   

   Not used.

3. The issue summary seems out of date - ie it does not explain the api changes.
4. Are the changes to WebTestBase 100% necessary?
5. We should have all the necessary followups created before committing eg. #81

Should we be doing this in this issue or another issue? Can we get a follow up created.

Opened up a follow up: #2404601: Figure out how to best deal with RequestContext::setCompleteBaseUrl

Are the changes to WebTestBase 100% necessary?

Sadly yes, as we support paths, relative URLs and absolute URLs now.

We should have all the necessary followups created before committing eg. #81

#2404603: Add proper support for Url objects in FieldPluginBase::renderAsLink(), so we can remove EntityInterface::getSystemPath(), This should be it.

Here is a patch.

@alexpott's comments looks like they have all been addressed, and I think we have all of the necessary followups created. Patch is green. Setting RTBC.

I'm still not comfortable with the changes to webtestbase - I've discussed it with @dawehner in IRC and we concluded that we could remove the changes and instead expand drupaGet to accept a URL object. This means that no contrib tests will be broken, there's less change and more addition.

alexpott: so
alexpott: https://www.drupal.org/node/2350837
alexpott: what should we do?
https://www.drupal.org/node/2350837 => Convert most usages of EntityInterface::getSystemPath() to use routes #2350837: Convert most usages of EntityInterface::getSystemPath() to use routes => 86 comments, 14 IRC mentions
dawehner: say "meh" :)
alexpott: so yes we could add support for URL objects
but this doesn't help us getting rid of the "/" fix
dawehner: yeah - makes me :(
dawehner: since that is the case I guess we need to stick with the current solution

Back to RTBC based upon the IRC conversation.

Rebased; had to resolve a conflict for a newline being added by #2281645: Make entity annotations use link templates instead of route names in a changed hunk. Stupid git.

A failure in Drupal\system\Tests\Theme\EngineTwigTest… that's unrelated. Let's see if a retest makes it green again.

Nope, that's a new test apparently that is using a leading / for a path with drupalGet(). See testTwigFileUrls().

Trivial fix, hence keeping at RTBC.

+++ b/core/modules/content_translation/src/Tests/ContentTranslationUITest.php
@@ -52,9 +53,9 @@ protected function doTestBasicTranslation() {
-    $this->drupalGet($entity->getSystemPath());
+    $this->drupalGet($entity->url());
...
+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -1469,11 +1474,15 @@ protected function isInChildSite() {
+    // Assume that paths that start with a / are already fully generated
+    // URL's that are just missing $base_root.
+    if (isset($path[0]) && $path[0] == '/') {
+      $url = $GLOBALS['base_root'] . $path;
+    }

Seems like this patch can't get around changing drupalGet() in some way to support an already generated URL. However, why overload it based on the first character of the string? Why not make this patch call $this->drupalGet($entity->urlInfo()); instead, and change drupalGet() to overload based on whether the parameter is a URL object?

Not changing issue status, since there might be a good reason, and if a committer is willing to commit as-is, I don't want to stop that.

@effulgentsia: We did discuss exactly that, see #86/#87. The argument against it was that we still have cases where we append a built URL with things.

I actually can't find that many of those in the patch and we should be able to build a url object for all because there has to be a matching route for it in the end (although there are some 404 and similar cases I believe), so I'm willing to give a try, but I can't say for sure when I will find time for that, possibly tomorrow.

@effulgentsia: We did discuss exactly that, see #86/#87. The argument against it was that we still have cases where we append a built URL with things.

I actually can't find that many of those in the patch and we should be able to build a url object for all because there has to be a matching route for it in the end (although there are some 404 and similar cases I believe), so I'm willing to give a try, but I can't say for sure when I will find time for that, possibly tomorrow.

There are really tricky cases in content translation tests.

Ok, I'm working on this in #2082049: Ignore me: Berdir vs. Simpletest, CommentTranslationUITest is green locally, did just touch that as the content translation tests is probably the most complex.

So, still needs a fair amount of work, but I think it is do-able, will post back here later.

Ok, let's see how this goes. Note that I reverted all the leading / test changes that were necessary before. I still think we shouldn't do that, but there is no need anymore to clean that up here.

Patch is slightly bigger, but I renamed a bunch of variables, especially in the content translation tests, to make it more consistent.

Grml. Stupid last minute changes.

berdir++

Discussed in IRC with @Berdir (great work btw) ContentTranslationDeleteForm and TermTranslationBreadcrumbTest are still using getSystemPath. Once this patch lands we should only have the path field plugins and views field plugins using it.

Updated those.

ContentTranslationDeleteForm was actually also path alias related. However, I decided to just update the 5 calls related to path aliases to $entity->urlInfo()->getInternalPath(). That's just as deprecated, but then we're at least rid of those getSystemPath() calls, that is more likely to be removed than getInternalPath().

Which means the only remaining calls are all in views link fields. Which has a follow-up: #2404603: Add proper support for Url objects in FieldPluginBase::renderAsLink(), so we can remove EntityInterface::getSystemPath().

Also updated the issue summary a bit.

Ok, that was mean, there was a _url() call deep down in that assertBreadcrumbs() thing. Made that dynamic now to support URL's as well, can't see a different way without converting 100 other calls.

Without debug().

#109 looks good to me. Double checked usage, and outside of the Entity code, the only remaining uses look like they are in Views related code and will be handled by the followup.

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

Follow up : #2473687: Fix @todo in BasicAuthTest