Remove most remaining url() calls

So if we remove all calls to url(), but don't remove url(), I don't think that makes sense. With this issue, we're basically saying that using url() is not best practice (as it sort of circumvents the routing system, I guess), but we're offering users no documented replacements. It's not simply a matter of using \Drupal::url(), because that only works for routes, whereas url() could take either a system path or an external URL. And #2339219: [meta] Finalize URL generation API (naming, docs, deprecation) seems to suggest that UrlGenerator::generateFromPath(), which is what url() currently wraps, should not be used for internal paths.

Here's an re-roll. aggregator.theme.inc failed as url() was replaced by usage of check_plain() in #2261425: Streamline aggregator's entities rendering with rest of core.

Thanks @dawehner. I read the other issue again but still confused about what an "external call" is, if it doesn't mean "a call to create a url from http://example.com". Internal or external to what code?

See #2342925: Simplify url() in order for the installer to function without it, which you'll need for the installer.

I'll push this one forward.

I did another 60. There should be enough examples now.

We'll be able to remove PathProcessorFront::processOutbound() once all internal URL generation is route-based. There already is a <front> route.

Critical based on #2339219: [meta] Finalize URL generation API (naming, docs, deprecation).

For this issue, let's start by converting as many straightforward url() calls as possible. If there are a few calls that are difficult to convert, we will leave them in for now and rename url() to _url(). I'll file issues for the next steps.

Checking invalid PHP syntax.

There you go.

Working on this slightly further.

@iMiksu,

Does your patch include all occurrences of old url()?

@Upchuk no it does not, just couple more, if you interested you may continue from here, I'll get some sleep.

Same here. will check back tomorrow and will try to help out if I can still find anything. There's also #2343651: Remove most remaining l() calls I can take a look at.

Good night.

Thanks @iMiksu!

Let's get some test results. :)

Note #2342949: Cleanup BlockContentBlock also touches one of these and is RTBC, if that helps

Gonna take this on for a bit

Interim work to see where I'm at

Some points of interest - generateFromRoute has less options than generateFromPath so things like adding a base path don't work. Also the way in which symfony's url generator encodes url's is different from Drupal's use of rawurlencode so had to override it in our generator.

#31: interesting! I found another, related, problem with using the Symfony URL generator, described over at #2343651-15: Remove most remaining l() calls: Drupal likes http://example.com?destination=admin/people, but Symfony does http://example.com?destination=admin%2Fpeople. There's no way to override that though, unless we override all of doGenerate().

So I think we should create a follow up for the non clean url tests in DownloadTest and ImageStylesPathAndUrlTest. We can't make the changes to \Drupal\user\EventSubscriber\MaintenanceModeSubscriber - this is just too hard for this issue - we need to split this subscriber up - atm it occurs before the request context that is set - moving it after has issues to - let's address this in #2294571: Redirect anonymous users to login page from an exception listener instead of in MaintenanceModeSubscriber and restrict access to the my-account link to authenticated users and #2288911: Use route name instead of system path in user maintenance mode subscriber.

Someone else's turn to take this through to green and rtbc.

My turn!

Per discussion with @dawehner, NullGenerator needs to hardcode for <front>.

Also I fixed the taxonomy changes, and made a few other conversions.

Two patterns we need to address:

url(NULL, array('absolute' => TRUE)) is used for #field_prefix in many places, just to get the http://drupal8/subdir/ part.
url(current_path()) used in various places, like Views.

Turns out that this is not so novice after all - sometimes when a replacement fails it can be really hard to work why and which one is causing the issue

A major problem I'm seeing is that the request context, when retrieved from the container, is not updated with the info from the request.
Unless someone knows how to get the request passed to fromRequest in the container, I added a Drupal\Core\Routing wrapper class with fromRequestStack.

This should fix some of the fails from #37, I haven't looked into the new fails in #40.

Still working on this.

I had some trouble with LanguageUILanguageNegotiationTest, \Drupal::url() seems to be skipping the language processing?!
I'll have to revisit that. Used generateFromPath() for now.

I didn't *need* to change all of the assertEqual($this->getUrl(), to assertUrl(, but I'm glad I did because it found a couple bugs. I only touched lines covered by this patch anyway.

Durpal!

I'll be back in 8-9 hours to work on this more, but I can't get Drupal\aggregator\Tests\AggregatorRenderingTest to fail locally, help needed there.

Fixing of remaining url() calls, analysis of the ones that we can't convert

I'm still working on this.

I'm not working on anything in the current patch, so rerolling to address the review below can happen in parallel while I'm fixing remaining calls.

Review

(I reviewed the entire patch, looked at every hunk.)

1. +++ b/core/lib/Drupal/Core/Routing/NullGenerator.php
   @@ -34,10 +35,26 @@ public function __construct(RequestStack $request_stack) {
   +    if ($name === '<front>') {
   +      return new Route('/');
   +    }
   

   What if somebody alters the route to not point at /, but at /llamas/are/fun? Won't this then be incorrect?

   It's not clear to me when NullGenerator is used; if this is only used in tests, then this would be okay.

2. +++ b/core/lib/Drupal/Core/Routing/RequestContext.php
   @@ -0,0 +1,27 @@
   +  public function fromRequestStack(RequestStack $request_stack) {
   

   Wow, that seems like a pretty major oversight on Symfony's side?

3. +++ b/core/modules/aggregator/src/Entity/Feed.php
   @@ -172,7 +172,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   -      ->setDescription(t('The length of time between feed updates. Requires a correctly configured <a href="@cron">cron maintenance task</a>.', array('@cron' => url('admin/reports/status'))))
   +      ->setDescription(t('The length of time between feed updates. Requires a correctly configured cron maintenance task.'))
   

   Looks like a regression. I'm guessing this is because the values in baseFieldDefinitions() must be static, and therefore the URL could be incorrect in a multisite setup?

4. +++ b/core/modules/file/src/Tests/DownloadTest.php
   @@ -117,14 +117,15 @@ function testFileCreateUrl() {
   +      // @todo Reinstate non-clean URL testing.
   +      // 'unclean' => 'index.php/',
   

   We need to fix this before RTBC I think?

5. +++ b/core/modules/image/src/Tests/ImageStylesPathAndUrlTest.php
   @@ -148,9 +148,10 @@ function doImageStyleUrlAndPathTests($scheme, $clean_url = TRUE, $extra_slash =
   -    if (!$clean_url) {
   -      $this->assertTrue(strpos($generate_url, 'index.php/') !== FALSE, 'When using non-clean URLS, the system path contains the script name.');
   -    }
   +    // @todo Reinstate non-clean URL testing.
   +    // if (!$clean_url) {
   +    //  $this->assertTrue(strpos($generate_url, 'index.php/') !== FALSE, 'When using non-clean URLS, the system path contains the script name.');
   +    // }
   

   Same.

6. +++ b/core/modules/language/src/Tests/LanguageUILanguageNegotiationTest.php
   @@ -417,7 +417,10 @@ function testLanguageDomain() {
   +    // @todo Using \Drupal::url() doesn't use the language prefixing.
   +    // $italian_url = \Drupal::url('system.admin', [], ['language' => $languages['it'], 'script' => '']);
   
   @@ -426,7 +429,8 @@ function testLanguageDomain() {
   +    // $italian_url = \Drupal::url('system.admin', ['https' => TRUE, 'language' => $languages['it'], 'script' => '']);
   
   @@ -434,8 +438,10 @@ function testLanguageDomain() {
   +    // $italian_url = \Drupal::url('system.admin', [], ['language' => $languages['it'], 'script' => '']);
   

   Huh? Why doesn't it? \Drupal::url() is an alias for UrlGenerator::generateFromRoute(), which calls UrlGenerator::processPath(); the language prefixing is done by language.module's OutboundPathProcessors.
   This is the rewriting of the *domain*, which indeed only happens in UrlGenerator::generateFromPath(). Also applies to Drupal\language\Tests\LanguageUrlRewritingTest

7. +++ b/core/modules/system/system.routing.yml
   @@ -304,6 +304,16 @@ system.files:
   +system.files_url_generator:
   +  path: '/system/files/{filepath}'
   

   I think something like system.private_files_url_generator or preferably even system.private_file_download would be much better. The current route name suggests this can be used for all file downloads, which is not the case.

8. +++ b/core/modules/views/src/Tests/Handler/FieldWebTest.php
   @@ -199,38 +204,37 @@ public function testAlterUrl() {
   -      $expected_result = url('node/123', array('query' => array('foo' => NULL), 'fragment' => 'bar', 'absolute' => $absolute));
   +      // @todo The route-based URL generator strips out NULL attributes.
   +      // $expected_result = \Drupal::url('entity.node.canonical', ['node' => '123'], ['query' => ['foo' => NULL], 'fragment' => 'bar', 'absolute' => $absolute]);
   +      $expected_result = \Drupal::urlGenerator()->generateFromPath('node/123', array('query' => array('foo' => NULL), 'fragment' => 'bar', 'absolute' => $absolute));
   

   Unfortunate, but this is at least pretty harmless. This @todo we could just commit as-is, IMO.

As you can tell: very, very little to complain about for a >200K patch.

#55.1 That is only ever used by the early installer. By the time system.module is installed, the NullGenerator is swapped out.
#55.2 RequestContext is in symfony/routing, Request and RequestStack is in symfony/http-foundation. So that does seem like an oversight, maybe @dawehner can open an issue upstream?
#55.3 I didn't remove this. I don't know if its a problem, or just that it seemed like overkill

#55.4 and #55.5 These I didn't comment out either, but unless there is a good reason from @dawehner, I agree we should fix here

#55.6 This seems like a pre-existing bug, and it seems to me that it deserves its own issue.

#55.7 Either name is fine by me.

This patch is big, it removes more than half of the url() calls. #2344487: Provide special route names for <current> and <none> will help us remove even more.
I don't know if we should continue to expand this patch to include more conversions...

• #55.1: ok, thanks!
• #55.6: agreed. But that probably means we want to revert to url() so we can keep (not comment) the test coverage.
• #55.7: ok, I went with system.private_file_download.
• RE: patch is big, let's not expand further: agreed. At the very least, this is massive progress.

I've stopped converting additional ones, this reroll includes the ones I already did. From my POV, #54 is RTBC as long as we revert #55.4, #55.5 and #55.6. If you can review and approve the attached interdiff, and attach a reroll with the revert, then we can get this big step forward committed.

There will be 131 calls to url() left.

The last hunk of the interdiff is what fixes #55.4 and #55.5. That may need a follow-up to always update the request context when requests are pushed on or off the stack.

Otherwise the rest are reverts. I'd consider this to be RTBC if it passes.

Oh now I feel extra good about that change, see the interdiff. They were called together before, now its always done.

Looks great. RTBC per #56, #57 and #59.

Haven't gone through the whole patch yet, mostly repeating previous reviews here:

1. +++ b/core/lib/Drupal/Core/Routing/RequestContext.php
   @@ -0,0 +1,27 @@
   +class RequestContext extends SymfonyRequestContext {
   

   I do think we need an upstream issue, that looks like a straight bug to me.

2. +++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
   @@ -56,6 +56,18 @@ class UrlGenerator extends ProviderBasedGenerator implements UrlGeneratorInterfa
   +    '%2F' => '/',
   

   Same question here for the upstream issue, again it looks like a straight bug (or a very poor choice if not).

3. +++ b/core/modules/aggregator/src/Tests/AggregatorTestBase.php
   @@ -87,7 +87,7 @@ function deleteFeed(FeedInterface $feed) {
   +      $feed_url = \Drupal::url('view.frontpage.feed_1', array(), array(
   

   Might be worth a novice follow-up to rename that display.

We should probably create a followup issue for removing our code if/when the upstream issue is resolved, and put a @todo referencing it in the codebase. Doesn't need to block commit.

I am probably the only person on earth who hates renaming displays. Once you do that, especially when people don't use at least proper prefixes, it can be pretty annoying to figure out which kind of display this is.

But since this would be a core VDC patch we get the final say in what the name would be? :)

Regarding the + '%2F' => '/', hunk:
Upstream UrlGenerator includes decoding:

'%2F' => '/',
'%40' => '@',
'%3A' => ':',
'%3B' => ';',
'%2C' => ',',
'%3D' => '=',
'%2B' => '+',
'%21' => '!',
'%2A' => '*',
'%7C' => '|',

We only need the / one, so we're overriding. It's not that they need to change something upstream, just that our URLs are "uglier" because they expect less decoding.

And to clarify the naming thing.
Core's policy is to have the displays named $display_type . '_' . $i, starting with 1. Asking for more descriptive names means no easy rule, more bikeshedding, and risking the naming getting stale if the purpose of something changes. So let's stick with feed_1 here.

Ok, to keep pre-beta things moving along, since timezones suck, and it looks like most of catch's review is addressed, committed and pushed to 8.x. I did add the following @todo to core/lib/Drupal/Core/Routing/RequestContext.php in the class definition PHPDoc:

 * @todo: Remove once the upstream RequestContext provides fromRequestStack():
 * https://github.com/symfony/symfony/issues/12057