Remove most remaining l() calls

Adapted the info to include the proper way for entities. Here is a patch which JUST converts 2 instances given an example for both.

Some additional changes.

Shall links to 'javascript:void()' use Drupal::l?

+++ b/core/modules/user/src/RegisterForm.php
@@ -115,7 +115,7 @@ public function save(array $form, FormStateInterface $form_state) {
-    $this->logger('user')->notice('New user: %name %email.', array('%name' => $form_state->getValue('name'), '%email' => '<' . $form_state->getValue('mail') . '>', 'type' => l($this->t('Edit'), 'user/' . $account->id() . '/edit')));
+    $this->logger('user')->notice('New user: %name %email.', array('%name' => $form_state->getValue('name'), '%email' => '<' . $form_state->getValue('mail') . '>', 'type' => \Drupal::l($this->t('Edit'), 'entity.user.edit_form', array('user' => $account->id()))));

+++ b/core/modules/user/src/Tests/UserAdminTest.php
@@ -52,7 +52,7 @@ function testUserAdmin() {
-    $link = l(t('Edit'), "user/" . $user_a->id() . "/edit", array('query' => array('destination' => 'admin/people')));
+    $link = \Drupal::l(t('Edit'), 'entity.user.edit_form', array('user' => $user_a->id()), array('query' => array('destination' => 'admin/people')));

You could use $user->urlInfo('edit-form') again (see initial patch as example

Shall links to 'javascript:void()' use Drupal::l?

Let's keep them for now.

Working on also making it possible to get a url object from views.

Some work.

Pushing this one further.

(While debugging the test failure in Drupal\user\Tests\UserAdminTest.)

Apparently, UrlGenerator is broken, and its test coverage is incomplete, because:

  $user_a = entity_load('user', 1);
  $link = l(t('Edit'), 'user/1/edit', array('query' => array('destination' => 'admin/people')));
  var_dump($link);
  $link = \Drupal::l(t('Edit'), 'entity.user.edit_form', array('user' => $user_a->id()), array('query' => array('destination' => 'admin/people')));
  var_dump($link);
  exit;

prints:

string '<a href="/user/1/edit?destination=admin/people">Edit</a>' (length=56)
string '<a href="/user/1/edit?destination=admin%2Fpeople">Edit</a>' (length=58)

l() calls url() which calls UrlGenerator::generateFromPath(), which uses UrlHelper::buildQuery() to build the query string.
\Drupal::l() calls UrlGenerate::generateFromRoute(), which calls getInternalPathFromRoute(), which calls \Symfony\Component\Routing\Generator\UrlGenerator::doGenerate(), which … of course doesn't call UrlHelper::buildQuery() and therefore explains the difference.

That's a pretty clear regression. But the only ways to fix it are: 1) fix upstream (no time to do that before beta), 2) don't use Symfony's UrlGenerator::doGenerate(), 3) allow the regression for now.

Re #15, see #2340251-31: Remove most remaining url() calls

Wim, can you explain why those are functionally different? Symfony seems to work with links, so is it an actual regression or just "different"?

I have to call it a day now, will continue tomorrow. Didn't get much done, sadly, had other obligations, and debugging these is pretty painful.

Progress report:

1. Fixed Drupal\toolbar\Tests\ToolbarHookToolbarTest failure.
2. Drupal\user\Tests\UserAdminTest root cause explained in #15. Indirectly caused by Drupal\user\Plugin\views\field\LinkEdit still rendering using l(), converting that to \Drupal::l() will fix the fail, but the way those links are rendered is … non-trivial … so I will need to step through it with a debugger to find where l() is being called :)
3. Drupal\taxonomy\Tests\TermTest fails because in TermForm::save(), $term->id() is NULL, i.e. the term didn't get created at all.

Crell: because this was done intentionally, for legibility:

// For better readability of paths in query strings, we decode slashes.
$params[] = $key . '=' . str_replace('%2F', '/', rawurlencode($value));

Have some unexpected time to push this forward further.

I did some more conversions. Would have done more but there were some external links + some more complex cases I'd be more comfortable if you took a look at the Wim.

What about the l() with current_path() in them? Can we use the request object to get the current route?

Let me know what else I can do to help.

This should bring it down to 1 fail and 1 exception.

(Cross-posted with Upchuk, so applied my interdiff to #22, the numbers above apply to #18.)

@Wim Leers
Mh actually the UrlGenerator already takes into account that. See UrlGenerator::$decodedChars. Will be worth to debug.

20:2 failures, this is nothing.

Let me work on the converting some of the view ones.

Also haven't done the views bit we talked about because this is kind of a mindfuck.

MEH! :(

Started with #23 and merged mine in.

Adding a related issue.

I'll continue with this one.

#24: no, UrlGenerator::decodedChars is used for the path segment only! :(

This should fix at least 48 of the 107 fails.

This should fix another 35 fails, bringing it down to 24 hopefully.

All the test failures in Shortcut module were related to a single wrong invocation of \Drupal::l() (which is an alias for LinkGenerator::generate()): it was being called with a Url object where a string $route_name was expected. Because PHP doesn't have a string typehint and we don't do explicit checking, this is what happened.
I haven't added a manual type check plus exception yet, because I know that's a bit controversial, but we might want to do that. It took me 25 minutes to find this. With such an exception, it would only have taken a second.

Removed 3 more l() invocations (one was in dead code). Number of failed tests should remain the same.

There are only 3 remaining l() invocations to be converted:

1. Link::preRenderLink(). That uses l(), but to fix that, we have to fix all code that uses #type => link. Grepping for #href will reveal those.
2. The "views bit" mentioned in #24: for (operation) links rendered by Views
3. ViewListBuilder::getDisplayPaths() and ViewUI::renderPreview(), which likely needs deep Views knowledge to be fixed

After that, I think we have no remaining l() calls that can be converted, they all:

1. use external URLs (most of them in tests, which we could arguably convert to not use any function at all), or
2. use file URLs, or
3. use current_path() and hence need #2344487: Provide special route names for <current> and <none>, or
4. are for listing path aliases, which are impossible to convert to routes

These represent 45 l() invocations. These would have to use the renamed _l() function IMO, as per #2343661: Rename l() to _l() and url() to _url(), and document replacements.

d.o--

I tested it and worked fine however its getting failed due to other changes.

Working on this.

See also #2277103: Switch Drupal::l() and LinkGenerator to expect a Url object

Opened #2345725: Query parameters are not decoded the same as the path portion of a URL and linked in this patch, since we can't convert anything using / in query parameters.

Here's the fix for that fail.

I think this is enough for one patch, we'll need #2344487: Provide special route names for <current> and <none> and dedicated follow-ups for more complex conversions.

#44.1 This is really only common in tests. Note that it doesn't have a fully loaded node.

4,5 The difference is some places don't have a fully loaded entity. #2277103: Switch Drupal::l() and LinkGenerator to expect a Url object would definitely help the ones that do.

17:52 timplunkett: in theory, you should never use a route name for an entity manually

Woah. That implies that we have to fix this all over core, in theory.

It makes sense, but it's a sad realization. 2 levels of abstraction to get an entity path: EntityInterface::urlInfo() to get the route name, the route name to get the path.

Hence, #2281645: Make entity annotations use link templates instead of route names. But that's certainly off-topic

Just fixing that, not addressing #55

Not sure why we're changing this test to use the front page instead of two particular nodes?

I think I might have done those. Those tests just need some links, it doesn't care links to what. In the new routing system, it's impossible to generate links to nodes without enabling the node module. This test doesn't yet enable the node module, so that'd be slowing the tests down for no reason. Hence I just switched them to other links.

Pushing this one further.

This comment is an attempt to go over ALL outstanding problems/questions/remarks and answer them, to determine what we still have to do in this issue before it can be RTBC.

In #37, I wrote:

There are only 3 remaining l() invocations to be converted:

1. Link::preRenderLink(). That uses l(), but to fix that, we have to fix all code that uses #type => link. Grepping for #href will reveal those.
2. The "views bit" mentioned in #24: for (operation) links rendered by Views
3. ViewListBuilder::getDisplayPaths() and ViewUI::renderPreview(), which likely needs deep Views knowledge to be fixed

1. The first of those really can't be converted; all (or at the very least nearly all) occurrences of #href are for external URLs, and hence we need #2343759: Provide an API function to replace url()/l() for external urls to fix those.
2. The second bit, tim.plunkett "fixed" in #42: it cannot be solved because Symfony's URL generator handles a slash in a query string differently than we did, hence we need #2345725: Query parameters are not decoded the same as the path portion of a URL to fix those.
3. The third bit can only really be done by a Views maintainer (I think), and the first is a protected function so is totally okay to change after beta, the second is public but is unlikely to ever be used outside of Views, and it only uses l() because ViewExecutable::getUrl() returns a path. So both of these are safe to do at a later point; they won't affect an important public API.

#44.1/#44.4/#44.5 are answered by #45.
#44.2: I wondered the same. And indeed, that code can be deleted altogether once they're views. But for now, we have to keep it around.
#44.3: :)

I'm comfortable with this going in as it stands, since the meh DX is not introduced by this patch, just surfaced

+1

#55.1: I thought this also at first, but… you only need to pass it through some sort of "external URL builder" or "external URL manipulator" or "external URL tweaker", if you're actually manipulating the external URL. If we're really using an external URL verbatim, then there's no point in passing it through a function.
#55.2: answered in #59.

CONCLUSION: This is RTBC.

… except that one of the blocking issues (#2344487: Provide special route names for <current> and <none>) landed in the mean time, so now we can convert those. But to do that, I opened #2346103: Convert url() and l() invocations using current_path() and '', since we have to do it not only for l(), but also url(), which has many more of those cases anyway.

Let's get this committed, just like we got #2340251: Remove most remaining url() calls, which also didn't convert everything, but represented massive progress, just like this patch does.

Attached reroll is only for chasing HEAD, no changes.

Nice work! Onward to beta. :)

Committed and pushed to 8.x. Thanks!

Found a broken $this->l() in core. I guess as they arise they can be Zapped on a case by case basis