Replace most existing _url calls with Url objects

small-step forward.

less brackets.

Going to try to fix as many of the exceptions and syntax errors as possible, and then triage the fails into common groups.

Common error #1:

Argument 2 passed to Drupal\rest\LinkManager\TypeLinkManager::__construct() must be an instance of Drupal\Core\Utility\UnroutedUrlAssemblerInterface, none given, called in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/hal/src/Tests/NormalizerTestBase.php on line 119 and definedDrupal\rest\LinkManager\TypeLinkManager->__construct(Object) Drupal\hal\Tests\NormalizerTestBase->setUp() Drupal\hal\Tests\FileNormalizeTest->setUp() Drupal\simpletest\TestBase->run() simpletest_script_run_one_test('439', 'Drupal\hal\Tests\FileNormalizeTest')

No idea what this means.

Common error #2:

Uncaught PHP Exception Symfony\Component\Routing\Exception\RouteNotFoundException: "Route "view.frontpage.page_1" does not exist." at /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Routing/RouteProvider.php line 147

There are similar problems with bad route names. Drupal\Tests\Core\UrlTest passes, so I suspect all of these fails need test mocking for urlGenerator.

Fixed a few syntax errors. Mainly want to see what testbot says for SimpleTestBrowserTest, as I there this may be a PHP 5.4 vs 5.5 complication for me to diagnose.

My comment above about mocking is wrong, as these are all webtests (ie, simpletest) and not unit tests. I assume something is wrong with all of these in the setup to make the routes available.

It looks like the view.frontpage.page_1route problem is solved by simply adding views to the list of modules installed by the test. Is this bad?

Patch needed reroll, No conflicts, just auto-merging

I think this fixed all of the HAL failures, but need to see what TestBot says about FileNormalizeTest.

Now I think all of the HAL stuff is fixed.

Reattaching w/ proper status for testbot to run this.

Returning to #12

It looks like the view.frontpage.page_1route problem is solved by simply adding views to the list of modules installed by the test. Is this bad?

Not bad. Using the routing is needed.. and so enabling the module is a must..

In any event - the dependencies of the test, are now apparent for the next developer to see and maybe object to..

All of the Search fails are because the route is used in the test module, so you need to add the dependency to the search_extra_type.info.yml and not the test.

The fail in GlossaryTest was a simple type in the route name.

I am going to see if I can fix TokenReplaceTest before I post my next patch, but the error isn't obvious.

Few more fixes that were explained in #23. I expect these failures:

Drupal\language\Tests\LanguageUILanguageNegotiationTest, Drupal\language\Tests\LanguageUrlRewritingTest - The test looks correct, but it looks like Url::fromRoute is ignoring the language when it is making the link.

PageCacheTagsIntegrationTest - Something is wrong with the URL generation for the nodes; the page that gets generated is a 404.

TokenReplaceTest - I don't get what this error really means.

Unassigning myself, as I won't be able to look at this for a little while.

Regarding Drupal\language\Tests\LanguageUILanguageNegotiationTest
I am posting a couple of observations, I don't regard this as anywhere near a fix....

Url::fromRoute() is used in a few places to create strings which will be compared to a reference string starting 'https:://'
well if you want that then in addition to 'https' => TRUE, another key - value pair is needed. 'absolute' => TRUE.

looking at Url::fromRoute() 'script' is not a valid option, so I have removed it..

Reroll, No conflicts just merging.

Like #27 this is a nudge is the correct direction not a fix, for anything...

I have correctly annotated the new function

ViewExecutable::getUrlInfo() - which highlighted to me that it returns a \Drupal\Core\Url object

Which in turn highlights that the line from TokenReplaceTest should be changed :-

-      '[view:url]' => $view->getUrlInfo('page_1')->setAbsolute(TRUE),
+      '[view:url]' => $view->getUrlInfo('page_1')->setAbsolute(TRUE)->toString(),

I am still debugging this but as the code then flows into PathPluginBase::getUrlInfo the new error results from the fact that $route_names is an empty array.

I am posting this partial update in the hope the solution will be obvious to others.

Or Maybe tomorrow night, for me.

Few quick comments, but I think we need @dawehner's input here.

The absolute option for the language tests is not present it 8.0.x, so I don't see why it is needed here.

PageCacheTagsIntegrationTest is failing b/c the base URL is getting tacked on, and then drupalGet is also prepending it. So, the URL is eg

http://localhost:8888/drupal-8.0.x/drupal-8.0.x/node/1

If you change the test to

$this->drupalGet($url->setAbsolute()->toString());

then the test passes. I don't want to just change the test to make it pass w/o confirming that this is correct.

TokenReplaceTest is still a mystery. I think @martin107 is right about needing toString(), but the exception suggests a bigger problem that it can't find the route or display for 'test_tokens.page_1', which doesn't make sense.

@timplunkett set me down the right path with TokenReplaceTest; I have that fixed locally.

Looking an the language problems again to see if I can fix them before I post another patch.

This fixes PathPluginBase, which makes TokenReplaceTest pass.

Nothing looks obviously wrong with the language tests. I went through the UI, enabled language, added Italian, configured it for URL (like the tests that are failing) and the URLs, and then added the switcher block. The link for italian is wrong, so I think something is wrong with the url generator itself. Trying to raise someone on IRC to help me confirm this.

Thanks, @mpdonadio and @timplunkett...

Ah its obvious when you see it... I hope the mantra of post early and post often ... is not seen as too much noise on a critical issue :)

So I am looking at the test results and the first arrays returned in error labelled PageCacheTagsIntegrationTest.php line 147
I have pretty printed the 2 arrays out to get some perspective as a.txt and b.txt with the difference file diff-a-b.txt

In short what this reveals is that the following 4 elements from the PageCacheTag are missing causing the comparison to fail

  15 => 'filter_format:basic_html',
...
  20 => 'node:1',
  21 => 'node_view',
..
  25 => 'user:2',

I can't explain the filter_fomr:basic_html thing ....But my hunch is that ....it looks like something that was working is now unexpectedly returns an empty string.
As always I hope digging into the details joggs someone's brain into providing the solution.

On a minor note I have reintroduced the annotations associated with ViewExecutable::getUrlInfo() as they were the not part of the error I made as #31 was skipped over.

@martin107, are you running tests locally? If so, do you see the same behavior I describe in #32 with the wrong fetched and resulting in a 404 (and hence different headers), and having ->setAbsolute() fix it?

Regarding PageCacheTagsIntegrationTest

When I try locally ( Nov 4th 16:46 UTC+0 ) it passes locally on my machine ( with 20 passes )

In the time between last known bad [ November 4, 2014 at 10:01am ] and now there have been 4 commits.

So I am retesting ... just to make sure.

My local testing is based on php5.5.10 using run-test.sh
browser testing fails, but I think that maybe a memory allocation thing at my end...

Did not look at the patch at all yet, but I suspect this overlaps quite a bit with #2350837: Convert most usages of EntityInterface::getSystemPath() to use routes ?

@dawehner, will do. I started a bash script to identify touched files in both patches, which will make this a fairly easy task. Comments on my observations in #32 and #35 would be appreciated, too, to keep this moving.

grep -- '--- a' replace_all_existing-2364157-37.patch | sort > replace_all_existing-2364157-37.files
grep -- '--- a' entity-system-path-2350837-15.patch | sort > entity-system-path-2350837-15.files
comm replace_all_existing-2364157-37.files entity-system-path-2350837-15.files > comm.txt

Based on that, the patches only overlap on two files:

core/modules/rest/src/Tests/AuthTest.php
core/modules/node/src/Tests/NodeTranslationUITest.php

The hunks for AuthTest are similar, but different. The change in entity-system-path-2350837-15 looks more better. I will yank that out of this patch.

The hunks for NodeTranslationUITest are in different portions. That will stay, though there may be clashes later on.

I made an issue related to the NodeTranslationUITest failure, #2369225: Add $options['base_url'] to UrlGenerator::generateFromRoute(), as I see the same behavior in HEAD with the Language Switcher block. As I mention in that issue, I thin the core problem has to do with routes mishandling the language option, but I don't know enough about this to confirm or deny it.

I will dig into the cause of the LanguageUrlRewritingTest fail to see if it is a test problem, or a code problem. But, my first look seems to indicate that some of the tests may not be valid with proper routes / URL, so we need to decide how to handle this. I will triage this.

On #44: No, the other issue is not poorly named. It's just that getSystemPath() return values eventually have to be passed through url(), so to be able to get rid of them, we also have to remove the _url() calls down the line. I posted it because I suspected that I've already solved some problem here over there.

If #2369225: Add $options['base_url'] to UrlGenerator::generateFromRoute() gets committed, then I think this is good, and worthy of a proper review.

I removed the absolute=TRUE from LanguageUILanguageNegotiationTest, as that is not correct, and LanguageNegotiationUrl::processOutbound() will generate an absolute URL anyway for domain based negotiation. There is a slight quirk in this test with regards to running the test on something other than port 80/443, but I don't think that is worth addressing in the test per some of the other comments about "Base path gives problems on the testbot, so $correct_link is hard-coded." in that file. If desired, we could hard code the port that the test is running on into the $correct_link.

The fix to PageCacheTagsIntegrationTest() is a bit of a hack, but it is a side effect of taking a URL, converting it into a string (which prepends the base path), then using that in WebTestBase::drupalGet(), which will then call UrlGenerator::generateFromPath(), which tacks on the base path again. So, the ->setAbsolute() in the patch gets around this. I am going to experiment with this and file a new issue if warranted (pretty sure this is a patchable bug in UrlGenerator::generateFromPath() wrt the ltrim logic in the middle).

adding blocker tag, since this blocks #2343669: Remove _l() and _url(), so that the d8 blockers is accurate. Remove the blocker tag when this issue is fixed, and unpostpone 2343669 if this was the last blocker of that.

Re-roll wasn't bad. Setting Needs Review to see where we stand on fails.

Back to Postponed. The fail in AddFeedTest is from a missing `use`. The other two are expected, and should be fixed when #2369225: Add $options['base_url'] to UrlGenerator::generateFromRoute() gets committed (I'll verify this later).

Also did a quick search, and we will need to do another pass for new uses of _url() that have crept in.

Blocking issue was committed; will have patch soon.

This should come up green. However, PhpStorm is telling me there are 55 usages left in code, and 3 in comments. I'll work on the ones outside of tests first, and get that to green. Then I will update the tests.

Changed my mind and did all of the tests first.

I'm wondering if we should add a static method to Url that more closely resembles _url(), that would be more generic that fromUri and fromRoute, something like

public static function fromPath($path, $options = array()) {
  $url = \Drupal::service('path.validator')->getUrlIfValidWithoutAccessCheck($path);
  if ($url === FALSE) {
    if (strpos($path, 'http://') === FALSE && strpos($path, 'https://') === FALSE) {
      $path = 'base://' . $path;
    }
    return static::fromUri($path, $options);
  }
  else {
    return $url->setOptions($options);
  }
}

From looking at the remaining usages, there seem to be a decent amount of cases where there is a string that can be a path from a route, a non-routable path, or an external path. This would make the switch easier.

Well that was embarrassing. I'm surprised there weren't more fails with what I just fixed...

Small manual merge when I did the rebase this morning (just added a use to a test).

Interdiffs are before the rebase, just to show what I changed.

Tests use the `Url::fromUri('base://' . $path)->toString()` pattern, as this is what was done in earlier work.

The functions that actually fetch data use the url generator service.

1. +++ b/core/modules/aggregator/src/Tests/FeedParserTest.php
   @@ -80,7 +81,7 @@ function testHtmlEntitiesSample() {
   +    $invalid_url = Url::fromUri('base://' . 'aggregator/redirect', array('absolute' => TRUE))->toString();
   

   This is a routed URL, I think it's aggregator_test.redirect

2. +++ b/core/modules/search/tests/modules/search_extra_type/search_extra_type.info.yml
   @@ -4,3 +4,5 @@ description: 'Support module for Search module testing.'
   +  - views
   \ No newline at end of file
   

   Just hit enter

Yeah, we need to do a good pass through this to see where routes are available and use them when possible.

#74-4, #74-7, WebTestBase::drupalGet() uses the URL generator, so I changed those two to also use it. So, everything that does actual fetching uses the URL generator, but assertions use the URL class.

#74-9: $view->display_handler->getUrlInfo() was throwing an exception about the route for the display not being available. Head scratcher.

@dawehner, if you are going to fix some of this, here is my work in progress from my repo. You should probably take a look at the new method I added to URL anyway to see if it is a good idea.

Simple reroll b/c a new behavior in core/modules/field_ui/field_ui.js

Mostly easy reroll. Few manual merges.

Fixed a bunch of the easy things.

I expect two groups of fails:

ImageStylesPathAndUrlTest is failing on the non-clean URL test, because a clean URL is being generated. Didn't poke much into it.

The rest seem to be related to recursive router rebuilds. Tried a few things, but didn't fire up a debugger to really dive into where the recursion is coming from.

Another reroll was needed while I was debugging this... Manual merge in EntityReferenceItem; chose what was in HEAD.

This patch doubled in size in comment #84, there's a ton of changes in field_ui that doesn't seem related to this. Are you sure this isn't a bad reroll in there? My guess would be the add field patch that landed around then.

I'm wondering if we should focus on #2350837: Convert most usages of EntityInterface::getSystemPath() to use routes first, I think that's going to get rid of quite a few _url() calls in way cleaner ways than this issue, especially in the page cache tags and rest tests. Also the WebTestBase changes I think are either very similar or actually copied from there.

Just so we have it, I took the patch from #82, applied to 9663c55, applied the interdiff from #84, and rebased against HEAD. There were a few manual merges. The patch is now about the same size.

I did not try to add in any of my changes from #90.

I'll defer to those above my pay grade about whether to postpone on #2350837: Convert most usages of EntityInterface::getSystemPath() to use routes, but a quick read of the patch does look like it would be a good idea. When I first looked at it, there was little intersection between the patches, but I think that has changed now.

However, between putting together Ikea shelves today, I picked away at the current patch and had some notes prepared that I don't want to lose...

ImageStylesPathAndUrlTest is failing on the non-clean URL test, because a clean URL is being generated. I think the problem is that ImageStyle::buildUrl() is using Url::fromUri('base://' ...) and is generating a clean URL because it is detecting that the system support clean URLs (could also be from the stream wrapper manager). Not sure what the proper way to override this is for testing purposes. I also didn't see any other similar cases in existing tests.

The REST tests are failing from a recursive router rebuild problem. As far as I can tell it is from when entity_test is enabled in the setUp. No clue why this happens. This fixes the problem, but I doubt it is a proper solution (or would be in scope of this issue):

--- a/core/lib/Drupal/Core/Routing/RouteBuilder.php
+++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
@@ -214,7 +214,7 @@ public function getCollectionDuringRebuild() {
    * {@inheritdoc}
    */
   public function rebuildIfNeeded() {
-    if ($this->routeBuilderIndicator->isRebuildNeeded()) {
+    if (!$this->building && $this->routeBuilderIndicator->isRebuildNeeded()) {
       return $this->rebuild();
     }
     return FALSE;

Sounds like a plan. Added tag because #97 doesn't apply any more...

Not sure if this will buy us much.

lsdiff 2350837-57.patch | sort > 2350837-57.files
lsdiff 2364157-WIP.patch | sort > 2364157-WIP.files
comm -12 2350837-57.files 2364157-WIP.files > both.txt

And my notes in both.txt

a/core/modules/config/src/Tests/ConfigSingleImportExportTest.php - same fix to test; removed hunks
a/core/modules/config_translation/src/Tests/ConfigTranslationUiTest.php - same fix to test; removed hunks
a/core/modules/contact/src/Tests/ContactPersonalTest.php - same fix to test; removed hunks
a/core/modules/forum/src/Tests/ForumTest.php - same fix to test; removed hunks
a/core/modules/node/src/Tests/NodeTranslationUITest.php - no overlap; kept hunks
a/core/modules/node/src/Tests/NodeViewTest.php - same fix to test, 2350837 has change; removed hunks
a/core/modules/rest/src/Tests/AuthTest.php - 2350837 has change; removed hunks
a/core/modules/rest/src/Tests/RESTTestBase.php - 2350837 has change; removed hunks
a/core/modules/simpletest/src/WebTestBase.php - one intersections on actual functions; remove second hunk
a/core/modules/system/src/Tests/Cache/PageCacheTagsTestBase.php - 2350837 has change; removed hunks
a/core/modules/system/src/Tests/ParamConverter/UpcastingTest.php - same fix to test; removed hunks
a/core/modules/system/src/Tests/Routing/RouterTest.php - same fix to test; removed hunks
a/core/modules/toolbar/src/Tests/ToolbarAdminMenuTest.php - same fix to test, 2350837 has change; removed hunks

That resulted in the attached as 2364157-101.patch.

Also attached a diff when I apply 2350837-57.files and 2364157-101.patch to see what the test results are.

Backed out change to ImagePath so that we can test unclean URLs.

Backed out change to BreadcrumbTest b/c I think #1799820: Breadcrumb doesn't get localized when displaying parent terms broke it.

I think all of the rest are recursive router rebuilds. I can't find what part of the patch is causing this, or how to fix it without the major hack from above.

I have a disconnect between my patch names and my git repo, so the last few patches above aren't ready to look at. Working on straightening all of this out.

This should come up green, and be free of intersections with #2350837: Convert most usages of EntityInterface::getSystemPath() to use routes.

However, please see the attached trace.txt and look at the change to Drupal\rest\Plugin\Derivative\EntityDerivative::getDerivativeDefinitions. Without that hunk, there are several test failures due to recursive router rebuild detections.

Essentially, WebTestBase::setUp() eventually leads to a drupal_flush_all_caches() after the required modules have been installed. The end of that function has a router rebuild. While rebuilding the router, Drupal\rest\Plugin\Derivative\EntityDerivative tries to get a route (entity.entity_test_mulrev.canonical) from the RouteProvider. That has a RouteBuilder::rebuildIfNeeded() call in it, so we end up with a rebuild in a rebuild.

The attached patch is a quick hack to just show that preventing the recursive rebuild makes this patch pass. Questions:

Fix this in this issue, or a new issue?

Should the fix catch the exception? If I do this, I would make a RouteBuilderException which extends \RuntimeException so we have something proper to catch.

Or, should the fix just do

--- a/core/lib/Drupal/Core/Routing/RouteBuilder.php
+++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
@@ -214,7 +214,7 @@ public function getCollectionDuringRebuild() {
    * {@inheritdoc}
    */
   public function rebuildIfNeeded() {
-    if ($this->routeBuilderIndicator->isRebuildNeeded()) {
+    if (!$this->building && $this->routeBuilderIndicator->isRebuildNeeded()) {
       return $this->rebuild();
     }
     return FALSE;

Or, and I missing something?

That looks like it works. I expect this to be green.

#111-1,2,3 should be easy.

#111-4 has been in the patch since the 2364157-2.patch version. Do you recall why you added it? :) I'll yank it out.

#111:1-4 should be fixed. Wish I had remembered about the route debug info in the Console project earlier...

SearchPageOverrideTest and EntityResolverTest are failing because the try/catch that we removed were masking a problem.

The other failures were from #111-4: 'Some mandatory parameters are missing ("project_name") to generate a URL for route'

EntityResolverTest is still failing:

Drupal\Core\Routing\RouteProvider->getRouteByName('entity.entity_test_mulrev.canonical')
Drupal\Core\Routing\UrlGenerator->getRoute('entity.entity_test_mulrev.canonical')
Drupal\Core\Routing\UrlGenerator->generateFromRoute('entity.entity_test_mulrev.canonical', Array, Array)
Drupal\Core\Url->toString()
Drupal\Core\Entity\Entity->url()
Drupal\serialization\Tests\EntityResolverTest->testUuidEntityResolver()
Drupal\simpletest\TestBase->run()
_simpletest_batch_operation(Array, '7', Array)
call_user_func_array('_simpletest_batch_operation', Array)
_batch_process()
_batch_do()
_batch_page(Object)
Drupal\system\Controller\BatchController->batchPage(Object)
call_user_func_array(Array, Array)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\PageCache->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1)
Stack\StackedHttpKernel->handle(Object, 1, 1)
Drupal\Core\DrupalKernel->handle(Object)

I don't see how the router is installed, is fully rebuilt, and the canonical route for entity_test_mulrev doesn't exist despite being in the entity definition.

Actually, it looks like EntityTestRoutes::routes() doesn't set up canonical routes.

This should come up green.

Added Needs issue summary update since we need to document what we aren't converting right now, and get followup issues made.

Quoting myself from IRC

aspilicious_home: Argh...
[5:19pm] aspilicious: I still think "Url::fromUri('base://' ." is terrible DX
[5:20pm] aspilicious: It's like Url::fromPath() but not with path in the function name
[5:21pm] aspilicious: And this issue https://www.drupal.org/node/2364157#new tells us that we actually abuse that base// hack to much in core, (not even mentioning contrib)
[5:21pm] Druplicon: https://www.drupal.org/node/2364157 => Replace all existing _url calls #2364157: Replace most existing _url calls with Url objects => 123 comments, 14 IRC mentions
[5:21pm] aspilicious: or client projects

I tried before... But I'll try again... But Yeah not strictly related to this issue.

The rebase against 8.0.x did an automatic three-way merge, so technically this is a reroll + newline for #122. The interdiff is between my git branches after the rebase.

Lets try this with an actual patch.

This will need a reroll after #2281645: Make entity annotations use link templates instead of route names, I suggest you wait until #2406439: Cleanup EntityDerivative and RouteBuilderInterface is committed as well, that should get rid of that recursive router rebuild business.

Yeah, I was expecting that, and waiting on #2406439: Cleanup EntityDerivative and RouteBuilderInterface is probably a good idea to see if we can back out that workaround. I'm also hoping #2350837: Convert most usages of EntityInterface::getSystemPath() to use routes can get in, too, before I post another patch, just in case it introduces any problems.

Found a short time break to test this...

My reroll + a small adjustment for 2406439 is coming up green.

The reroll has two manual merges. Both keep HEAD. One essentially backs out our router rebuild workaround. The other is to use the path-style link annotation for entity_test_mulrev.

That + 2350837-95.patch (ie, the RBTC one) applies cleanly and also comes up green.

I'll unpostpone and post the proper patch tonight so people can begin reviewing again.

Looks like a lot of the base:// usages shouldn't be there - you don't know the route name?

1. +++ b/core/modules/aggregator/src/Tests/FeedParserTest.php
   @@ -80,7 +81,7 @@ function testHtmlEntitiesSample() {
   +    $invalid_url = Url::fromUri('base://' . 'aggregator/redirect', array('absolute' => TRUE))->toString();
   

   This should use a route name, as per #73

2. +++ b/core/modules/basic_auth/src/Tests/Authentication/BasicAuthTest.php
   @@ -150,7 +151,7 @@ protected function basicAuthGet($path, $username, $password) {
   +        CURLOPT_URL => Url::fromUri('base://' . $path, array('absolute' => TRUE))->toString(),
   

   This obviously can't.

3. +++ b/core/modules/hal/src/Tests/DenormalizeTest.php
   @@ -24,7 +25,7 @@ public function testTypeHandling() {
   +          'href' => Url::fromUri('base://rest/type/entity_test/entity_test', array('absolute' => TRUE))->toString(),
   

   There's a chicken and egg problem here, and I don't think you can use a route name for any of these rest paths.

OK, latest patch based.

Rebased 130. Two conflicts. Kept HEAD for both. One essentially backs out our router rebuild workaround. The other is to use the path-style link annotation for entity_test_mulrev.

Interdiff is against the rebase. The change to NodeTranslationUITest brings it back to HEAD b/c #2350837: Convert most usages of EntityInterface::getSystemPath() to use routes now overlaps here. The other change is to add real canonical routes for the entity_test entity types (the test types invoke ->url() in a few tests, which were the cause of exceptions).

This should come up green. This + entity-get-system-path-2350837-102.patch also came up green in my test.

All uses of base:// seem to be necessary. Once this, and similar path/route related issues, get in, I will open a followup issue to audit core for base://, and either convert to route or document why base:// is needed in each case.

1. +++ b/core/modules/basic_auth/src/Tests/Authentication/BasicAuthTest.php
   @@ -7,6 +7,7 @@
   @@ -150,7 +151,7 @@ protected function basicAuthGet($path, $username, $password) {
   
   @@ -150,7 +151,7 @@ protected function basicAuthGet($path, $username, $password) {
        $out = $this->curlExec(
          array(
            CURLOPT_HTTPGET => TRUE,
   -        CURLOPT_URL => _url($path, array('absolute' => TRUE)),
   +        CURLOPT_URL => Url::fromUri('base://' . $path, array('absolute' => TRUE))->toString(),
            CURLOPT_NOBODY => FALSE,
   

   All the calls that are made to this method are valid route names, I believe this could be refactored to accept an Url object instead of a $path string and then just do $url->setAbsolute()->toString(), see my last patch in the getSystemPath() issue, a few similar refactorings.

2. +++ b/core/modules/hal/src/Tests/FileNormalizeTest.php
   @@ -39,7 +39,8 @@ protected function setUp() {
        $entity_manager = \Drupal::entityManager();
   -    $link_manager = new LinkManager(new TypeLinkManager(new MemoryBackend('default')), new RelationLinkManager(new MemoryBackend('default'), $entity_manager));
   +    $url_assembler = \Drupal::service('unrouted_url_assembler');
   +    $link_manager = new LinkManager(new TypeLinkManager(new MemoryBackend('default'), $url_assembler), new RelationLinkManager(new MemoryBackend('default'), $entity_manager, $url_assembler));
   

   I hate that we create those objects here by hand in a kernel test, but I guess that is not for this issue to change.

3. +++ b/core/modules/hal/src/Tests/NormalizerTestBase.php
   index a87554c..4be27ee 100644
   --- a/core/modules/image/src/Entity/ImageStyle.php
   
   --- a/core/modules/image/src/Entity/ImageStyle.php
   +++ b/core/modules/image/src/Entity/ImageStyle.php
   
   +++ b/core/modules/image/src/Entity/ImageStyle.php
   +++ b/core/modules/image/src/Entity/ImageStyle.php
   @@ -14,6 +14,7 @@
   
   @@ -14,6 +14,7 @@
    use Drupal\Core\Entity\EntityWithPluginCollectionInterface;
    use Drupal\Core\Routing\RequestHelper;
    use Drupal\Core\Site\Settings;
   +use Drupal\Core\Url;
    use Drupal\image\ImageEffectPluginCollection;
    use Drupal\image\ImageEffectInterface;
    use Drupal\image\ImageStyleInterface;
   

   Looks like an unecessary change, nothing else in this file is changed, so there can't be anything using Url?

4. +++ b/core/modules/language/src/Tests/LanguageUILanguageNegotiationTest.php
   @@ -462,23 +463,22 @@ function testLanguageDomain() {
   -    $this->assertEqual($italian_url, $correct_link, format_string('The _url() function returns the right URL (@url) in accordance with the chosen language', array('@url' => $italian_url)));
   +    $this->assertEqual($italian_url, $correct_link, format_string('The Url::fromRoute() returns the right URL (@url) in accordance with the chosen language', array('@url' => $italian_url)));
   

   No "The " then. And even that doesn't really make sense. (that method does not return a URL string)

   Maybe just "The right URL ..."?

5. +++ b/core/modules/rest/src/LinkManager/RelationLinkManager.php
   @@ -27,24 +28,33 @@ class RelationLinkManager implements RelationLinkManagerInterface {
      public function getRelationUri($entity_type, $bundle, $field_name) {
   -    // @todo Make the base path configurable.
   -    return _url("rest/relation/$entity_type/$bundle/$field_name", array('absolute' => TRUE));
   +    return $this->urlAssembler->assemble("base://rest/relation/$entity_type/$bundle/$field_name", array('absolute' => TRUE));
   

   There's an issue for this @todo that @larowlan is working on, you might want to check that affects this.

6. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -99,7 +100,7 @@ public function post(EntityInterface $entity = NULL) {
   -      $url = _url(strtr($this->pluginId, ':', '/') . '/' . $entity->id(), array('absolute' => TRUE));
   +      $url = Url::fromUri('base://' . strtr($this->pluginId, ':', '/') . '/' . $entity->id(), ['absolute' => TRUE])->toString();
          // 201 Created responses have an empty body.
          return new ResourceResponse(NULL, 201, array('Location' => $url));
   

   This is strange. I don't see why this shouldn't just call $entity->url() ?

7. +++ b/core/modules/system/src/Tests/Cache/PageCacheTagsIntegrationTest.php
   @@ -132,24 +133,24 @@ function testPageCacheTags() {
   -    $this->drupalGet($path);
   +    $this->drupalGet($url->setAbsolute()->toString());
   

   Once getSystemPath() is in, you can just pass $url through here.

8. +++ b/core/modules/system/system.module
   @@ -457,7 +457,7 @@ function system_authorized_run($callback, $file, $arguments = array(), $page_tit
   -  return batch_process($finish_url->toString(), $process_url->toString());
   +  return batch_process($finish_url->toString(), $process_url);
   

   Noticed that another call called just 'update.php/results' here, toString() would be '/update.php/results', Which one is correct?

9. +++ b/core/modules/system/tests/modules/entity_test/src/Routing/EntityTestRoutes.php
   @@ -27,6 +27,15 @@ public function routes() {
   +      $routes["entity.$entity_type_id.canonical"] = new Route(
   +        $entity_type_id . '/manage/{' . $entity_type_id . '}',
   +        array('_controller' => '\Drupal\entity_test\Controller\EntityTestController::testEdit', 'entity_type_id' => $entity_type_id),
   +        array('_permission' => 'administer entity_test content'),
   +        array('parameters' => array(
   +          $entity_type_id => array('type' => 'entity:' . $entity_type_id),
   +        ))
   +      );
   

   canonical pointing to the edit form? why do we need this, and why like this and not an _entity_view?

10. +++ b/core/modules/update/src/Tests/UpdateCoreTest.php
    @@ -190,7 +190,7 @@ function testDatestampMismatch() {
    -      ->set('fetch.url', _url('update-test', array('absolute' => TRUE)))
    +      ->set('fetch.url', Url::fromUri('base://update-test', array('absolute' => TRUE))->toString())
    

    This is a route I think, in a test module.

11. +++ b/core/modules/views/src/Plugin/views/display/DisplayRouterInterface.php
    @@ -38,4 +38,11 @@ public function collectRoutes(RouteCollection $collection);
    +  /**
    +   * Generates an URL to this display.
    +   *
    +   * @return \Drupal\Core\Url
    +   */
    +  public function getUrlInfo();
    

    Missing description on the @return, AFAIK, only $this does not need one.

12. +++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
    @@ -486,5 +486,15 @@ public function validate() {
    +  public function getUrlInfo() {
    +    if (strpos($this->getOption('path'), '%') !== FALSE) {
    +      throw new \InvalidArgumentException('No placeholders supported yet.');
    +    }
    

    What happens if this is called if the view has arguments?

13. +++ b/core/modules/views/src/ViewExecutable.php
    @@ -1758,6 +1759,26 @@ public function getUrl($args = NULL, $path = NULL) {
    +    if (!$this->display_handler instanceof DisplayRouterInterface) {
    +      throw new \InvalidArgumentException(String::format('You cannot get generate a URL for the display @display_id', ['@display_id' => $display_id]));
    

    This exception message not english is :)

I'll look at these, but a few quick comments.

8. The second argument to batch_process() is supposed to be a Url object.

9. This was to match the change in #2281645: Make entity annotations use link templates instead of route names where the annotation has the same path for canonical and edit form. The canonical route was needed to prevent an exception in a test, but I don't think it is used anywhere b/c it was just in a data structure.

8. I meant the first argument, not the second :)

9. Strange. Have a look at entity.routing.yml, it has the canonical route, but only for entity_test, not the others. I suggest you move that into the class and make it generic but consistent with that.

Also, keep an eye on #2350509: Implement auto-route generation for all core entities and convert all of the core entities., that is doing the same now I think.

OK, this should come up green

#139-1,4,6,10,11,13, done

#139-3, reverted file to HEAD, done

#139-5, checked with @larowlan on #2336247: Make Relation and Type domain configurable based on context and he said to leave this in this patch.

#139-7, added @todo

#138-8, the $finish_url in system_authorized_batch_process() comes from system_authorized_get_url(), which is authorize.php not update.php, so left this as-is

#138-9, I realized that I had duplicated that route (not sure when it came in). I reverted that file to HEAD.

#139-12, per #143, left as is

Cleared up some confusion on my end w/ FeedParserTest::testRedirectFeed(). There was a small manual merge needed when I rebased, so the interdiff is a `git diff` just against the file that I changed.

@mpdonadio,
Are we going to inroduce the Url::fromPath() that's why we are not going to remove the all calls ?

Updated the issue summary #121.
Added the beta changes section as well.
Not removing the tag right now as it need to create another follow up issues to convert remaining _url() calls and also need to update the follow up issues as well.

1. +++ b/core/includes/form.inc
   @@ -815,13 +823,16 @@ function batch_process($redirect = NULL, $url = 'batch', $redirect_callback = NU
   -        $function($batch['url'], $options);
   +        $function($batch_url, ['query' => $query_options]);
   

   Unless we want a CR here then we should do $batch_url->toString() here. Unfortunately, as far as I can see, this redirect_callback is completely unused, untested and undocumented so whilst it might be nice to just pass the Url object alone I think this is the best option.

2. +++ b/core/modules/system/tests/modules/session_test/session_test.module
   @@ -1,9 +1,10 @@
   +use Drupal\user\UserInterface;
   ...
   -function session_test_user_login($account) {
   +function session_test_user_login(UserInterface $account) {
   
   +++ b/core/modules/update/tests/modules/update_test/update_test.routing.yml
   @@ -11,5 +11,6 @@ update_test.update_test:
   +    project_name: NULL
   
   +++ b/core/modules/user/user.api.php
   @@ -1,6 +1,7 @@
   +use Drupal\user\UserInterface;
   
   @@ -132,7 +133,7 @@ function hook_user_format_name_alter(&$name, $account) {
   -function hook_user_login($account) {
   +function hook_user_login(UserInterface $account) {
   
   +++ b/core/modules/user/user.module
   @@ -616,7 +616,7 @@ function user_login_finalize(UserInterface $account) {
   -function user_user_login($account) {
   +function user_user_login(UserInterface $account) {
   
   

   This all looks unrelated to me

3. +++ b/core/modules/views/src/ViewExecutable.php
   @@ -1758,6 +1759,26 @@ public function getUrl($args = NULL, $path = NULL) {
      /**
   +   * Gets the Url object associated with the display handler.
   +   *
   +   * @param string $display_id
   +   *   (Optional) The display id. ( Used only to detail an exception. )
   +   *
   +   * @throws \InvalidArgumentException
   +   *   Thrown when the display plugin does not have a URL to return.
   +   *
   +   * @return \Drupal\Core\Url
   +   *   The display handlers URL object.
   +   */
   +  public function getUrlInfo($display_id = '') {
   +    $this->initDisplay();
   +    if (!$this->display_handler instanceof DisplayRouterInterface) {
   +      throw new \InvalidArgumentException(String::format('You cannot generate a URL for the display @display_id', ['@display_id' => $display_id]));
   +    }
   +    return $this->display_handler->getUrlInfo();
   +  }
   

   Only for one test? (TokenReplaceTest) Can't we just to this in the test?

4. The issue summary and title need fixing - we still have _url calls after applying this patch - 33 of them.

Removed the special characters from the html.
Yeah #149-2 will address with that because these are not related to this patch.

OK, I am postponing again on #2350837: Convert most usages of EntityInterface::getSystemPath() to use routes so we can get an accurate list of remaining _url() uses, and get the followups made.

#149:1,2 should be easy.

I am deferring to @dawehner on #149-3.

1)Addressed the #149-2 that makes more sense and not related to this stuff.
The interdiff is not same as specified https://www.drupal.org/documentation/git/interdiff I accidently committed the changes in the same new local branch.
So I created a new branch and apply the #149 patch to it.and then commit the changes to it and then create the patch by diff oldbranch new branch
Instructions followed :

1. git checkout -b 2364157-149
2. git apply -v replace_all_existing-2364157-146.patch
3. git commit -m "Applied changes."
4. Did the changes as specified in the #149-2
5. git commit -am "Applied the #149-2 suggested changes"
6. git checkout 8.0.x
7. git checkout -b 2364157-149-old
8. git apply -v replace_all_existing-2364157-146.patch
9. git commit -m "Applied changes."
10. Interdiff : diff replace_all_existing-2364157-146-old.patch replace_all_existing-2364157-146.patch > intediff.txt
11. Patch : git diff 8.0.x > replace_all_existing-2364157-146.patch

Is there any better option to do if accidently commit the changes in the same issue old branch ?

2)#149-4
Updated the issue summary with the function usages in call and as well in comments.
Will need to create the child issues.how should the follow ups be created ?
Should be 7 followups for different components ?
1)For core/inlcudes/common.inc
2)core/module/image
3)core/module/node
4)core/module/rest
5)core/module/simpletest
6)core/module/system
7)core/module/views

changing to needs review so the bot will run.
will change it back.

about to update the beta evaluation.

---
you could have made the interdiff by git diff'ing the two branches. :) Next time.

updated beta evaluation (and put it in the motivation, where Contributor task: https://www.drupal.org/contributor-tasks/update-allowed-beta says to put it, and so it doesn't get lost after the long list of usages)

The change to update_test.routing.yml is needed.

This patch includes #149-1, #149-2, and will come up green.

Will postpone when TestBot is done.

Back to Postponed on #2350837: Convert most usages of EntityInterface::getSystemPath() to use routes, as that removes some _url() calls and (unfortunately) introduces a few more (three last time I counted). When we get the final list, I will triage them into groups, and have a followup for each to keep things manageable. I anticipate one for

• Views
• File/Image
• Everything else

However, I think the first two in this list may need to be postponed on #2405551: Add a method to support UIs where users enter paths instead of route names and other valid use cases.

I am deferring to @dawehner on #149-3 to weigh in before we make the (hopefully) final version of this patch, but I think we will want this method in ViewExecutable in the long term, as I think we will probably want/need it for some of the remaining routing/menu issues.

#2350837: Convert most usages of EntityInterface::getSystemPath() to use routes got in!

Which means that needs needs a reroll for sure and then we need to check if we really got all _url() calls.