Convert all instances of #type link/links to convert to use routes

can you push a sandbox branch?

1. +++ b/core/modules/aggregator/src/FeedViewBuilder.php
   @@ -99,7 +100,7 @@ public function buildComponents(array &$build, array $entities, array $displays,
   +            '#href' => Url::fromUri($link_href),
   
   +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -271,6 +272,7 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
            '#href' => 'admin/people/permissions',
   +        '#url' => Url::fromRoute('user.admin_permissions'),
   

   Extra #href, one missing conversion.

2. +++ b/core/modules/toolbar/src/Element/ToolbarItem.php
   @@ -33,7 +33,6 @@ public function getInfo() {
            '#type' => 'link',
            '#title' => NULL,
   -        '#href' => '',
   
   +++ b/core/modules/toolbar/tests/modules/toolbar_test/toolbar_test.module
   @@ -17,7 +17,7 @@ function toolbar_test_toolbar() {
          '#type' => 'link',
          '#title' => t('Test tab'),
   -      '#href' => '',
   +      '#url' => '',
   

   What's the difference between these?

Will we need https://www.drupal.org/node/2235495#comment-9197627 once this is in?

Working on this.

Whew. Quite the can of worms.

That's probably it for me tonight.

One more for the road.

Thanks @dawehner, those fixes should help a lot. My turn again!

#2345753: Remove url(current_path()) and url(NULL) with <current> and <none> might have gotten in the way if we did it later, so pulling parts of it in.
I'm still working on this, in a test issue.

This incorporates the first half of #1853072: Remove forum_menu_local_tasks_alter() hack and instead add links in ForumController::build

I can't deal with this issue more tonight. Good luck $whoever_is_next!

Okay a bit more before bed.

That was about 3 hours ago, and you're not online, so I'm going to get back to posting patches for this. Hopefully we don't overlap too much.

Still working on this. Getting close!

Hmmm, UnroutedUrlAssembler::buildLocalUrl needs to do more of what UrlGenerator::generateFromPath does...

Here it is without the exceptions I used to track things down.

It's the little things.

+++ b/core/includes/theme.inc
@@ -1018,16 +1019,19 @@ function template_preprocess_links(&$variables) {
+          // @todo System path is deprecated - use the route name and parameters.

We need an issue for this.

Working on this

Keep in mind this still removes CommentLinkBuilderTest, which I have NO idea how to fix.

Yay! I was able to figure out CommentLinkBuilderTest. It was easier to debug when I reduced the permutations down to 1 each, and found the exact combination that was failing.
Moral of the story: don't mock value objects.

(Also, I switched the order of the assertion since PHPUnit makes a distinction between expected/actual.

API-wise, this should have been a beta blocker. Better late than never.

Weird tag bug.

shoudl we remove toArray() and toRenderArray() all together?

Just as long as it gets back to green before long, research away.

I removed verbose calls because we don't usually force debug output into our tests, and because it blows up on Url

+++ b/core/lib/Drupal/Core/Url.php
@@ -473,6 +478,10 @@ public function toArray() {
+  public function __toString() {
+    return 'bloub';
+  }

?

Rerolled because #2326409: Annotate render element plugins removed hook_element_info() from the toolbar.module.

+++ b/core/core.services.yml
@@ -491,7 +491,7 @@ services:
-    arguments: ['@request_stack', '@config.factory' ]
+    arguments: ['@path_processor_manager', '@request_stack', '@config.factory']

I think we've agreed that path processing will not be done for unrouted url's.

Our current implementation of shortcut paths considers them unrouted, by using base://, and they need language processing. Otherwise, someone needs to rewrite shortcut module, I guess?

1. +++ b/core/lib/Drupal/Core/Url.php
   @@ -456,12 +456,26 @@ public function toString() {
   -      'url' => $this,
   ...
   +        'route_name' => $this->getRouteName(),
   +        'route_parameters' => $this->getRouteParameters(),
   +        'options' => $this->getOptions(),
   

   This needs to be reverted, we just removed support for 'route_name'.

2. +++ b/core/lib/Drupal/Core/Url.php
   @@ -456,12 +456,26 @@ public function toString() {
   +        // @todo Change 'path' to 'href': https://www.drupal.org/node/2347025.
   +        'path' => $this->getUri(),
   

   I don't know why we're bringing this back.

Forgot the interdiff, and wasn't up-to-date with HEAD.

Oh, actually this should work. Forgot that all of the routing changes were done to Shortcut, and the path is just computed.

Weird. TypedData is adding an extra 'value' => [] to the route_parameters, which breaks the comparison if ($shortcut->getRouteName() == $url->getRouteName() && $shortcut->getRouteParams() == $url->getRouteParameters()) {

Why that is happening now and not in HEAD, I do not know.

Oh! That would be why. Shortcut::setRouteParams is wrapping the array in an array for no reason.
Yay for finding bugs.

This should adequately address @alexpott's feedback.

+++ b/core/modules/block/block.module
@@ -93,10 +93,15 @@ function block_page_build(&$page) {
-      '#href' => 'admin/structure/block' . (\Drupal::config('system.theme')->get('default') == $theme ? '' : '/list/' . $theme),
...
+    if (\Drupal::config('system.theme')->get('default') == $theme) {
+      $page['page_top']['backlink']['#url'] = Url::fromRoute('block.admin_display_theme', ['theme' => $theme]);
+    }
+    else {
+      $page['page_top']['backlink']['#url'] = Url::fromRoute('block.admin_display');
+    }

This seems to be the wrong way around?

I need more time to review this patch.

Shortcut thinks should use routes - or if not, they should not get any path processing applied.

Anything using base:// is like hard-coding a link.

I'd like to hold off committing this patch until we've got solid agreement on #2351379: [meta] Define, then support exact use cases for link generation and storage.

After that while I expect we'll need to convert a lot of these cases, we might find specific examples that need to be moved to non-routes. Or if we do go ahead with the patch, we might have to go through and convert some cases a second time.

#74, I fixed shortcut, that's using routes, and I removed the path processing I added.

The only usages of base:// added in this patch are:
Drupal\views\Plugin\views\Field\Links::getLinks() where we deal with a string path
Two places we handle ?destination=, which must be a string path.

If you hold off on this patch past Thursday, you'll need to find someone else to work on it, I'll be gone until late October after this week. It'd be a shame to see this massive improvement fall stale and not get in.

@Tim can you look at #2351379: [meta] Define, then support exact use cases for link generation and storage then? It has a comprehensive issue summary and no big long discussion to wade through. Also enjoy the break :)

I think this issue makes that one easier, not harder.

Rerolled and fixed #73. Opened #2352791: Add test coverage for admin/structure/block/demo

Looking at the patch I do think this makes the other issue easier rather than harder.

Nearly all the changes are the specific use case of linking to a route very much coupled to a module, it's not the user-entered route use-cases that are proving difficult to support with routes in the other issue. Also some valid uses of base::// for destination which I don't think we'll need to change either.

So yes (unless someone has a concrete objection I missed), let's keep going here and continue in the other issue dealing with menu links/shortcuts and similar cases where the pain is.

Added a workaround for a random bug.
I tested the patch in #2352791: Add test coverage for admin/structure/block/demo with the patch from #71, and it catches the bug, and passes with the fix in #79.

Additionally, added @todos for the known places we'd want to stop using with fromUri().

None of the get*Path() methods are called during routing, just get*RouteName() and get*Route().

Opened #2353029: Book "Add child page" link query string needs test coverage to add test coverage.

Thanks for opening the issues to improve test coverage.

I agree with @catch in #81. I don't any reason to hold this up based on #2351379: [meta] Define, then support exact use cases for link generation and storage

Committed 195d499 and pushed to 8.0.x. Thanks!

+++ b/core/includes/theme.inc
@@ -902,14 +902,8 @@ function template_preprocess_status_messages(&$variables) {
+ *     - url: (optional) The url object to link to. If omitted, no a tag is
+ *       printed out.

Can we change this wording a bit, please? I have read it 3-4 times to understand that this is not a typo. Maybe something like "If omitted, no <a>-tag is printed out."?

I'm guessing #82 missed an instance of the random fail and so this introduced: #2353457: Random test failure in Drupal\views_ui\Tests\DisplayPathTest

#2353347: Random failure in DisplayPathTest

Published change notice (tracked down to this after finding all my contrib list builder operations broken).

oops, removed the deprecated toArray() from the test that was to test toArray(). see #2417445: Remove Url::toArray()