Problem/Motivation

Part of #2339219: [meta] Finalize URL generation API (naming, docs, deprecation). url() needs to be deprecated and removed from Drupal 8, because it circumvents the routing system for internal URLs.

Proposed resolution

Convert as many straightforward url() calls as possible.

  • For internal URLs, \Drupal::url() (which takes a route name) is usually the correct replacement in procedural code.
  • UrlGenerationTrait is available for use in classes.
  • For now, uses of url() for external URLs (if any) could be converted to directly use UrlGenerator::generateFromPath()

Remaining tasks

In a followup issue, we will rename url() to _url() to make the BC break explicit, and mark it deprecated. After that, the remaining uses of _url() will be removed before 8.0.0

User interface changes

None.

API changes

None yet (in other issues).

CommentFileSizeAuthor
#60 interdiff.txt1.53 KBtim.plunkett
#60 2340251-url-60.patch224.57 KBtim.plunkett
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,629 pass(es). View
#59 interdiff.txt10.83 KBtim.plunkett
#59 2340251-url-59.patch223.62 KBtim.plunkett
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,622 pass(es). View
#1 url-2340251-1.patch8.48 KBdawehner
#6 url-2339219-6.patch7.77 KBiMiksu
#11 url-2339219-11.patch58.26 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,324 pass(es), 528 fail(s), and 59 exception(s). View
#11 interdiff.txt49.93 KBWim Leers
#14 url-2340251-13.patch144.09 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/shortcut/shortcut.module. View
#14 interdiff.txt90.27 KBdawehner
#21 url-2340251-21.patch144.09 KBiMiksu
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,062 pass(es), 1,432 fail(s), and 141 exception(s). View
#21 interdiff.txt1018 bytesiMiksu
#24 interdiff.txt10.18 KBiMiksu
#24 url-2340251-24.patch154.27 KBiMiksu
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,066 pass(es), 1,432 fail(s), and 139 exception(s). View
#31 24-31-interdiff.txt26.79 KBalexpott
#31 2340251.31.patch157.78 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,957 pass(es), 415 fail(s), and 32 exception(s). View
#34 31-34-interdiff.txt9.78 KBalexpott
#34 2340251.34.patch157.89 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,171 pass(es), 374 fail(s), and 23 exception(s). View
#37 2340251-url-37.patch185.2 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,507 pass(es), 32 fail(s), and 6 exception(s). View
#37 interdiff.txt30.18 KBtim.plunkett
#40 url-2340251-39.patch214.34 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,448 pass(es), 40 fail(s), and 20 exception(s). View
#40 interdiff.txt30.21 KBdawehner
#43 2340251-url-42.patch219.31 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,492 pass(es), 14 fail(s), and 14 exception(s). View
#43 interdiff.txt6.16 KBtim.plunkett
#45 2340251-url-44.patch223.35 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,519 pass(es), 12 fail(s), and 1 exception(s). View
#45 interdiff.txt28.83 KBtim.plunkett
#47 interdiff.txt4.18 KBtim.plunkett
#47 2340251-url-47.patch223.32 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,507 pass(es), 8 fail(s), and 1 exception(s). View
#49 2340251-url-49.patch228.22 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,531 pass(es), 5 fail(s), and 1 exception(s). View
#49 interdiff.txt6.42 KBtim.plunkett
#51 2340251-url-51.patch228.22 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,556 pass(es), 4 fail(s), and 0 exception(s). View
#51 interdiff.txt766 bytestim.plunkett
#54 url-2340251-54.patch228.67 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,559 pass(es). View
#54 interdiff.txt27.64 KBdawehner
#57 url-2340251-57.patch233.28 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,599 pass(es), 7 fail(s), and 0 exception(s). View
#57 interdiff.txt6.05 KBWim Leers
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

dawehner’s picture

Assigned: dawehner » Unassigned
FileSize
8.48 KB

Just a start to show in which direction it could lead.

Wim Leers’s picture

Issue tags: +Novice, +php-novice
Wim Leers’s picture

Title: Replace a lot of the leftover instances of url() » Remove all remaining url() calls
Issue tags: +Pre-AMS beta sprint, +beta deadline
iMiksu’s picture

Assigned: Unassigned » iMiksu
xjm’s picture

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.

iMiksu’s picture

Assigned: iMiksu » Unassigned
FileSize
7.77 KB

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.

dawehner’s picture

I think we should convert all INTERNAL calls away from url() and convert all EXTERNAL (not necesassary external URL (see other issue)) to use the new method on the url generator
and drop the long documentation on url() saying: use this in case you want to do X and use that if you want to do Y.

xjm’s picture

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?

tim.plunkett’s picture

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

I'll push this one forward.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Active » Needs review
FileSize
58.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,324 pass(es), 528 fail(s), and 59 exception(s). View
49.93 KB

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

Wim Leers’s picture

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
144.09 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/shortcut/shortcut.module. View
90.27 KB

@Wim Leers
Maybe we should keep it, just for consistence, but yeah this would help people to find the bad calls.

xjm’s picture

Title: Remove all remaining url() calls » Remove most remaining url() calls
Priority: Normal » Critical
Issue tags: -beta deadline

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.

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
xjm’s picture

Issue tags: +beta blocker
iMiksu’s picture

Assigned: Unassigned » iMiksu

Checking invalid PHP syntax.

iMiksu’s picture

Assigned: iMiksu » Unassigned
FileSize
144.09 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,062 pass(es), 1,432 fail(s), and 141 exception(s). View
1018 bytes

There you go.

iMiksu’s picture

Assigned: Unassigned » iMiksu

Working on this slightly further.

Upchuk’s picture

@iMiksu,

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

iMiksu’s picture

Assigned: iMiksu » Unassigned
FileSize
10.18 KB
154.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,066 pass(es), 1,432 fail(s), and 139 exception(s). View

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

Upchuk’s picture

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.

xjm’s picture

Status: Needs work » Needs review

Thanks @iMiksu!

Let's get some test results. :)

larowlan’s picture

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

alexpott’s picture

Assigned: Unassigned » alexpott

Gonna take this on for a bit

alexpott’s picture

Status: Needs work » Needs review
FileSize
26.79 KB
157.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,957 pass(es), 415 fail(s), and 32 exception(s). View

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.

Wim Leers’s picture

#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().

alexpott’s picture

Assigned: alexpott » Unassigned
Status: Needs work » Needs review
FileSize
9.78 KB
157.89 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,171 pass(es), 374 fail(s), and 23 exception(s). View

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.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

My turn!

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
185.2 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,507 pass(es), 32 fail(s), and 6 exception(s). View
30.18 KB

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.

dawehner’s picture

dawehner’s picture

Status: Needs work » Needs review
FileSize
214.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,448 pass(es), 40 fail(s), and 20 exception(s). View
30.21 KB

Some more.

alexpott’s picture

Issue tags: -Novice, -php-novice

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
219.31 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,492 pass(es), 14 fail(s), and 14 exception(s). View
6.16 KB

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
223.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,519 pass(es), 12 fail(s), and 1 exception(s). View
28.83 KB

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
4.18 KB
223.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,507 pass(es), 8 fail(s), and 1 exception(s). View

Durpal!

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
228.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,531 pass(es), 5 fail(s), and 1 exception(s). View
6.42 KB

Status: Needs review » Needs work

The last submitted patch, 49: 2340251-url-49.patch, failed testing.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs work » Needs review
FileSize
228.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,556 pass(es), 4 fail(s), and 0 exception(s). View
766 bytes

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.

Status: Needs review » Needs work

The last submitted patch, 51: 2340251-url-51.patch, failed testing.

dawehner’s picture

Assigned: Unassigned » dawehner

Woorking on fixing the test and cleanups

dawehner’s picture

Assigned: dawehner » Unassigned
Status: Needs work » Needs review
FileSize
228.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,559 pass(es). View
27.64 KB

@tim.plunkett
It is incredible helpful to have a apache configuration for localhost/d8 and one for d8.dev

This should be green now.

Wim Leers’s picture

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.

tim.plunkett’s picture

#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...

Wim Leers’s picture

FileSize
6.05 KB
233.28 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,599 pass(es), 7 fail(s), and 0 exception(s). View
  • #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 submitted patch, 57: url-2340251-57.patch, failed testing.

tim.plunkett’s picture

FileSize
223.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,622 pass(es). View
10.83 KB

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.

tim.plunkett’s picture

FileSize
224.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,629 pass(es). View
1.53 KB

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

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

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

dawehner’s picture

Great teamwork!

catch’s picture

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.

dawehner’s picture

  • https://github.com/symfony/symfony/issues/12057
  • Same question here for the upstream issue, again it looks like a straight bug (or a very poor choice if not).

    Not sure here, the symfony one provides nicer URLs.

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

    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.

xjm’s picture

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? :)

tim.plunkett’s picture

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.

webchick’s picture

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

  • webchick committed 0eda196 on 8.0.x
    Issue #2340251 by iMiksu, alexpott, tim.plunkett, dawehner, Wim Leers:...
webchick’s picture

Status: Reviewed & tested by the community » Fixed
xjm’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.