Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence

I think we usually postpone follow up issues until the base issue lands, so doing so here, but correct me if I'm wrong.

Going to take a crack at this.

FWIW, I just filed #1893730: Provide an in-memory mock implementation of Path\AliasManager for tests and update.php — AFAICS, that doesn't or shouldn't conflict with anything here though.

@Crell that sounds eminently sensible.

However, we won't get very far as long as this unholy abomination is one of the path listeners that need to be converted to a futzer:

  public function onKernelRequestLanguageResolve(GetResponseEvent $event) {
    // drupal_language_initialize() combines:
    // - Determination of language from $request information (e.g., path).
    // - Determination of language from other information (e.g., site default).
    // - Population of determined language into drupal_container().
    // - Removal of language code from _current_path().
    // @todo Decouple the above, but for now, invoke it and update the path
    //   prior to front page and alias resolution. When above is decoupled, also
    //   add 'langcode' (determined from $request only) to $request->attributes.
    drupal_language_initialize();
  }

I think this can only be sorted out in conjunction with the work I started over in #1862202: Objectify the language system although we don't necessarily need the whole OOPification of the language negotiation system, just some refactoring and separating out of responsibilities. So here's what I propose: I'll break off that work into its own issue with the following goals:

• Make the language manager not dependent on request scope
• Add a request listener for the language negotiation system that runs before the path listeners (maybe with the exception of onKernelRequestDecodePath)
• This listener, not the path subscriber, is responsible for language initialization
• A side effect of extracting the language from the request will be to obtain the prefix-less path, so store this statically (a temporary measure until we actually have a path futzer object that we can set it as a property on.)
• Change the onKernelRequestLanguageResolve method to just call the function that will return the prefix-less path that was statically stored in the last step, and then call $this->setPath($request, $path); just like the other path listeners do.

Then we'll be in a position to convert all the listeners to futzers per #5 I think.

Am I over-thinking this? :-/

Edit: I had used blockquote instead of code :-P #fail

Right, that's basically where I was going - it's just that I felt that sorting out the language stuff would be an intermediary step, done in a separate issue, hence we'd still have the path listeners doing $this->setPath(). And then this issue would be where we get rid of that and convert things to futzers. Does that make sense or should we just do it all in this issue?

Hmm - it's really hard to know how to carve this work up. And I feel it ought to be carved up because the changes required here to make the language/path stuff sane are also needed over in #1862202: Objectify the language system but it doesn't seem right to block that one on this seeing as there's so much more needed for this issue that is not relevant to that issue (and vice versa).

So I had intended to stop at just fixing up the LanguageManager and rolling a patch that both issues could then depend on, but once you disentangle language initialization from path futzing, well... you need to add the futzer. So I went ahead and did the futzing bit too, although just for the incoming path. But in doing so I had to rip out the path alias caching mechanism as it's unclear to me where it fits in in this new set-up. So I can't stop here either :-/

Anyway, thought I'd put up what I have for now - Crell, maybe you could wade through it and just see if we're on the same page regarding the futzers, which I have called manipulators (though futzers would have been so much easier to type.) I use a compiler pass to register incoming path manipulators to a manipulator manager, which ended up essentially being a composite of manipulators. That's possibly not what we want once we add the outgoing stuff and caching into the mix...

I've broken the language manager changes off into a separate issue: #1899994: Disentangle language initialization from path resolution

OK I managed to get this rebased on the bus so sticking up a combined patch just so that testbot can tell us what's exploding. The path futzer stuff on its own is in the do-not-test patch and I have not yet gotten around to changing the name from manipulators to processors...

Will this solution account for the redirect contrib module (and others) that might cause a conflict to exist with the url path?

• #1817976: Updating an entity with an URL alias that matches an existing redirect causes bad data and circular redirect
• #683510: Add hook_path_alter()

Or is this an issue that should be addressed elsewhere? Like maybe:

• #1751210: Convert URL alias form element into a field and field widget
• #1553358: Figure out a new implementation for url aliases

#14: incoming_fuzters_combined_with_language_fixes.14.patch queued for re-testing.

crell: Thanks for you response. #19 totally makes sense at runtime, which wasn't the perspective I was inquiring about. What about route save time. Does Symfony2 provide a mechanism to mediate route conflicts during a save operation? I think it needs to be centralized, for all the reasons why this issue was created.

Redirect would need to listen once the inbound futzers are finished.

I have a hard time understanding what is going on since so much of this is new to me. Some other use cases that I have in my current D7 dev checkout that I'd like to check are possible are the following:

hook_url_inbound_alter

./subpathauto/subpathauto.module:22:function subpathauto_url_inbound_alter(&$path, $original_path, $language) {
// Converts paths like node-alias/foobar to node/1/foobar. Note that this needs to call inbound resolution itself (recursive), which might be a worrying case.

./fb/fb_url_rewrite.inc:148:function fb_url_inbound_alter(&$result, $path, $path_language) {
// Like language, resolves Facebook URL prefixes like fb_page/* and then calls inbound resolution again to look up the normal path. Could be benefited by futzing since it's purpose is very similar to language module.

./redirect/redirect.module:206:function redirect_url_inbound_alter(&$path, $original_path, $path_language) {
Uses url_inbound_alter to check for a redirect that matches $original_path instead of the resolved path since we lose that context in Drupal 7. Might be possible to get rid of in D8.

./domain_path/domain_path.module:51:function domain_path_url_inbound_alter(&$path, $original_path, $path_language) {
Resolves per-domain path aliases.

hook_url_outbound_alter

./subpathauto/subpathauto.module:38:function subpathauto_url_outbound_alter(&$path, $options, $original_path) {
// Converts links like node/1/foobar to node-alias/foobar. Note that this needs to call outbound resolution itself (recursive), which might be a worrying case.

./fb/fb_url_rewrite.inc:107:function fb_url_outbound_alter(&$path, &$options, $original_path) {
./fb/fb_canvas.module:320:function fb_canvas_url_outbound_alter(&$path, &$options, $original_path) {
// Adds Facebook prefixes to links.

./domain/settings_custom_url.inc:17:function domain_url_outbound_alter(&$path, &$options, $original_path) {
Alters outbound URLS that have domain-specific settings to have the different domain name set as the link's base URL.

./simple_login_destination/simple_login_destination.module:6:function simple_login_destination_url_outbound_alter(&$path, &$options, $original_path) {
Adds a 'destination' query string to every /user and /user/login link when the user is anonymous.

Let's try this one more time.

Looking at the hook_url_inbound_alter examples above, the subpathauto one definitely sounds like the trickiest (the futzer for that would have the futzer manager service as a dependency??) but I don't think there's anything in this new architecture that will pose problems for any of those examples.

Now if I could just manage to roll a patch that testbot can apply...

Just out of curiosity I made a futzer that does the subpathauto thing, by taking the futzer manager as a dependency - and it totally worked, the universe did not implode :-D But actually it sounds like you wouldn't even need to do that - you'd just need the alias futzer service as a dependency of the subpath alias futzer.

Figured I may as well keep this moving along on top of the language manager patch, which will hopefully land soon.

I renamed the manipulators to processors, added docblocks.

Should we re-title this issue to reflect the fact that this first part is just concerned with processing the incoming path?

#28: 1888424.path_processors.review.do-no-test.patch queued for re-testing.

The language manager issue went in.

Getting rid of the last remaining instances of the word "manipulator", also adding the request type as a parameter to the processors because the language-based processor needs to only act if it's the master request, as per the fix here: #1899994-41: Disentangle language initialization from path resolution.

#31: 1888424.path_processors.31.patch queued for re-testing.

Note that this requirement does impose a dependency on HttpKernel

Well, come to think of it... we are also passing the request as a parameter but in fact none of the processors even uses the request in its processIncoming() method. Maybe what we need instead of both of these parameters is a context parameter, i.e. an array containing information about the request (including the request type), and any processor that cares about a particular context element can check for it. Thoughts?

As discussed in yesterday's wscci meeting, it makes sense to break off the incoming path processing into a separate issue and keep this one for the url()/generator side of things. The new issue for the incoming side is here #1910318: Make path resolution the responsibility of a series of PathProcessor classes, rather than one PathSubscriber class and the only change to the patch from the one in #31 here is that I removed the $request_type param that I had added, after discussing this with Crell. This just means that the language-based path resolution will kick in on subrequests but won't actually do anything because there won't be a language prefix.

Here's an initial attempt at doing the outbound side, including a combined patch on top of the inbound stuff, just to see how hard testbot laughs at me.

OK, let's try this...

Why the rename?

It just seemed odd in the url() function to be asking for a 'router.generator' service when the function we're calling on it has nothing to do with the router. But then... the service itself is still in the Router namespace so I guess there's not much point in changing its name. I'll revert it on the next reroll.

Will move those other procedural functions into the class as well, although I'm wondering if we shouldn't use a separate class for those seeing as they'll still be called from procedural code in places (i.e. using drupal_container()->get('router.generator')->httpBuildQuery(...)) and I'm not sure it makes sense for those methods to be part of the PathBasedGeneratorInterface, but they ought to be part of *some* interface.

The biggest thing I'm stuck on at the moment is, what's the best way to handle the problem of hook_url_outbound_alter implementations that alter things other than the path? There's only one example in core: when domain-based language negotiation is used, urls are rewritten to be absolute and to include the language subdomain (so $options['absolute'] and $options['base_url'] are altered). Looking on drupalcontrib.org, the only implementation I find is purl module, which has a series of path processors modifying things other than the path.

I guess the options are to either a) have a futzer for each thing modules could possibly want to futz with or b) have an options futzer. I'm not particularly enthused about either of these. And maybe I'm missing some other alternative solution to this..?

#44: 1888424.outgoing_path_futzing.44.patch queued for re-testing.

This changes one of the most often called functions in core.

Derp. Thanks to chx for the tip-off about what was going wrong in the testing environment.

Oops

This should fix all the above test failures but I know that it breaks at least one more :-/ I was fixing SimpleTestTest which was having a failure related to http://drupalcode.org/project/drupal.git/blob/ca5a3ef054940fbdc7200433e1.... So of course as soon as I saw that I wanted to fix it up to not be modifying $_SERVER vars. But the way I fixed it up causes a failure in the Session HTTPS test. Also, there's an https.php as well which does something similar but my attempts to fix that up resulted in 12 failures in the Session HTTPS test.

So I'd like to just see if we are in fact down to 1 failure with this patch. Then I guess I need to decide whether to just forget about overhauling either of those files and instead identify the exact nature of the problem in SimpleTestTest and provide a minimal fix for it.

Sigh. I have no idea how I did that :-(

Oh, well for starters I had totally broken WebTestBase::assertUrl() - hopefully that was the cause of most of those fails.

I'm reverting the change to http.php because I think a change like that (which should include a corresponding change to https.php) belongs in its own issue. But I haven't been able to figure out how to get rid of the fail in SimpleTestTest :-/

There are 2 tests failures that I can't reproduce locally. Here are fixes for the other 3 and I'm crossing my fingers that the 2 that pass locally for me will also pass here this time around...

All 3 of those tests pass for me locally - both through the UI and using run-tests.sh. Also, what's up with testbot? The output from the test failures is not helpful :-/

#57: 1888424.outgoing_path_futzing.57.patch queued for re-testing.

+class PathProcessorManager implements InboundPathProcessorInterface, OutboundPathProcessorInterface {

I don't understand why we are basically reimplementing EventDispatcher here? The more typical pattern here would be to have a ProcessPathInboundEvent / ProcessPathOutboundEvent with a ->setPath() method, and use it in PathProcessorManager with something like:

$event = new ProcessPathOutboundEvent($path, $options, $request);
$this->dispatcher->dispatch(PathProcessorManager::OUTBOUND, $event);
$path = $event->getPath();

Implementing a more targeted event then the ones that already exist is fine, but what's the reasoning behind reimplementing an complete dispatcher?

The code in PathProcessorLanguage seems bogus (but it was bogus before this patch). At the minimum, we should use information from $request directly, not from $request->server (which breaks encapsulation in mostly all cases):

+        if ($request !== NULL) {
+          $https = $request->server->get('HTTPS');
+          $is_https = $https && strtolower($https) == 'on';
+          $http_host = $request->server->get('HTTP_HOST');
+        }

Coming from #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig, I'm working on implementing a 'url is active' callback that both l() and theme('link') can use to determine when they should add 'active' classes to links. I think it should be part of this refactoring. Also url_is_external() could be moved into this UrlGenerator class.

In the attached patch I've got one problem which is preventing 'active' class tests from passing:

During UrlTest::testLActiveClass, there is no $request set for UrlGenerator, even though the test is calling a drupalGet() on a valid page callback.

1. Does this look like the right way to implement a 'url is active' check?
2. Does this look like the right way to refactor the 'url is external' check?
3. What do you think the deal is with $request not being set when that drupalGet('common-test/l-active-class') is called?

didn't get tests.

Talked to kat about this, she said that this is effectively a blocker (or at least a strong dependency) for things like [@1743590] and #1830854: [meta] The ESI pipeline battle plan. Since those are critical, escalating to critical as well.

This latest patch is just some refactoring to use the request object properly (including dealing with the last point in #62). I still cannot reproduce the Session HTTPS, LanguageUrlRewrite or Access Denied fails, they will probably fail here again.

@steveoliver I am not ignoring your patch, I'd just like to get this green first before we add in more refactorings that break more tests :-P I'll roll in your changes again once I've gotten to the bottom of these outstanding fails.

Gah, stupid tests modifying global variables :-(

Heh, all those tests were passing for me locally - I finally thought of testing it with drupal running in a subdirectory and I can reproduce the fails :-) This is good.

#64 - if you're refactoring url_is_external() anyway would you consider #1900092: url_is_external() should be called url_is_absolute() and _external_url_is_local() should be called _absolute_url_is_local()?

I haven't gotten to the bottom of the fails with Session HTTPS test and Access Denied test when running in a subdirectory, I suspect it is drupal_goto()'s fault. In the meantime I'd like to find out what the net effect is of my latest attempts at fixing stuff.

#74 - it got a little bit worse :(

Wheeeeeee!

Well, I consider that a win - I was aware of all 5 of those fails locally :-)

Latest changes are just some tidying up, they *shouldn't* cause any more breakages. I think at this stage people could review it even though it's not green yet. In the meantime I'll bang my head against these last 5 fails.

Does this fix the Session HTTPS test without breaking anything else?

#81: 1888424.outgoing_path_futzing.81.patch queued for re-testing.

#81: 1888424.outgoing_path_futzing.81.patch queued for re-testing.

I... have no words

Just reverting the change I had made to WebTestBase::getAbsoluteUrl() in the last patch, to see if that was responsible for the spectacular assplosion.

Still haven't addressed all of Crell's points above but want to get a patch up for testbot. I had tried to make the generator dependent on the request but I couldn't get the installer to work (how strange!) so instead I'm just making the DIC call the setRequest() method so then we don't need a request listener. Of course this means that the request doesn't change for a subrequest, but for the purposes of the generator that doesn't actually matter - all it's interested in are the base url, base path and script path. And anyway, with a request listener, while it does get changed for each subrequest it doesn't get changed back afterwards so it's much of a muchness. Plus the setRequest method call in the DIC is what we'll need once that Symfony PR lands.

Other than that the main change here is that I've converted all url generation tests (well, almost all) to phpunit. This included the existing unit tests but also the WebTest for path-based url generation. There are more that can be converted I think but it's a start. Yay PHPUnit stubs! :-)

There's also a bit of refactoring, including moving httpBuildQuery and stripDisallowedProtocols to a PathUtils class.

Now to see how much damage I've done...

Doh! I thought that patch size seemed a little low, and sure enough I didn't diff back far enough so that's not going to apply :-/

I was noticing weirdness when enabling modules via the UI - it enabled the module but didn't reload the modules page. Making the generator service persistent does fix that strange behavior for me so I'm hoping that change will also get testbot beyond that. The other changes are just some clean-up.

Wow, I don't believe I've engaged in this much flail since the module handler patch... :-/

Stupid WebTests and run-tests.sh...

Again, stupid WebTests and run-tests.sh...
(and slightly less stupid me)

I'm still absolutely not convinced that we need to implement our own custom, one-off, limited EventDispatcher in there. This type of things is exactly what the actual EventDispatcher is for. If there are performance concerns (the Symfony components introducing a massive amount of overhead as compared to our previous micro-framework, I'm not sure I understand why we suddenly focus on performance here), let's fix them upstream.

Ugh. (Too ashamed to provide an interdiff :-P)

@Crell: can you point toward precise examples of this in Symfony or Symfony CMF?

Really the only thing I see here is a (limited) re-implementation of EventDispatcher, which has no advantage that I can see but severe drawbacks:

(1) It's capabilities are limited as compared to the actual EventDispatcher (cannot remove the listeners, introspect them, etc.)
(2) It's a *third* way of reacting to events in Drupal 8 (the first two are hooks and events)
and as a consequence, (3) it is really confusing to everyone: both people from the Drupal world, that expect hooks, and people from the Synfony world that expect events.

So, could you elaborate on why you think that reimplementing something that exists is a "perfectly legitimate design in OOP code"?

Shouldn't this extend UrlGeneratorInterface?

Why? To me it seems fine as a separate interface - it just so happens that our generator implements both because it generates router-based as well as path-based urls, but in theory you could have a class that only implements the PathBasedGeneratorInterface, right? I guess it's a bit odd for it to be lumped in with routing stuff though so not sure what we really want here...

Wowsa. Is this all for performance optimization purposes?

This code has changed slightly in that we now just have the $basePath, $baseUrl and $scriptPath properties. Setting them once seems to make sense from a performance optimization perspective, but there's another reason too: these properties now have setters and that's what gets us around problems with url() being called when there is no request. We just set these properties and then it knows how to generate urls for paths. I've left a todo about eventually getting rid of that fallback (because it uses the dreaded globals).

I don't understand why we're persisting the request object here, on both inbound and outbound

I need to look at this again and remind myself what my thinking was. I've been working on fixing tests for so long I've almost forgotten what this patch is even about :-P

Latest changes are just cleanup...

Should have pointed out that the questions I'm responding to above come from #90.

#111 very nice
Didn`t do deep review of documentation(not native speaker (( ), but the code are good for me

PS. This part are very usefull for multidomain handling such as domain contrib

Ripped out the setRequest() shenanigans in the language-based path processor as it seems that was just me over-complicating things...

One thing I noticed off the bat with this latest patch in #114 is, maybe related to #72: the link to "Visit your new site" after install didn't honor the fact that I installed in a subdirectory; I installed in http://localhost/d8; The link was to http://localhost.

Let's see how this reroll fares - the two major changes I had to deal with were the services-in-yaml patch and the changing of variable_get('https', FALSE) to settings()->get('mixed_mode_sessions', FALSE). The latter means that both the UrlGenerator and PathProcessorLanguage are dependent on the 'settings' service, but this is not reflected in the interdiff as it was done during reroll.

The interdiff only reflects the changes I made to incorporate Crell's feedback in #117. I took the easy way out regarding the @inheritdoc change, i.e. I'm ignoring it for now :-P.

Dare I ask why we need to rebuild the container 3 times in one setUp() method?

Uh, apparently we don't - I don't know what this is left over from, I guess the period of general flail I went through with this patch a while back :-/ In general, I was needing to rebuild the container any time there was a drupalPost() call that changed the enabled languages, because of services that call language_list() in their constructor. Anyway, I've removed those calls and the tests pass without them.

Also, I confirmed the bug mentioned by @steveoliver in #115, just haven't figured out the best way to deal with it yet. Will try and come up with something while testbot chews on this...

OK here's a fix for the stupid installer page - best I could come up with for now...

#119: 1888424.outgoing_path_futzing.119.patch queued for re-testing.
Edit: Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git] seems unrelated.

Minor conflict, rerolled...

Heh, I had removed that $request variable as it wasn't being used and in the meantime a patch got committed that actually uses it...

Thankfully that turned out to have been just a silly mistake I made when adding the outbound path processor to language.services.yml - hopefully that accounts for all those fails, only ran a couple of the tests locally...

Edit: fixing a typo

When I fixed the "Visit your new site link" at the end of installation when installed in a subdir, it broke when not installed in a subdir. Tiny change and now it works in both scenarios...

Had forgotten one instance of Drupal::service('request') that needed to be converted to Drupal::request()...

HttpRequestTest.php is gone.
Quick re-roll.
Attached a patch diff, not a real interdiff.

I will review on Monday 15th April... any other core committers who can do this before me and commit - go ahead - but claim the issue first as the review is going to take me sometime :)

Did anyone profile this?

Profiling it before adding caching would give us some 'before' numbers, then we'd know how slow it was on cache misses, for example. The hook_url_outbound_alter() pipeline (and etc. except for path aliases - and path alias caching is very complex) isn't cached at the moment so a straight comparison to the status quo seems useful.

I only reviewed this cursorily, so this might be incomplete, but here's what jumped out at me. Given that this issue is critical, I'd be fine with these being follow ups.

+++ b/core/lib/Drupal/Core/PathProcessor/OutboundPathProcessorInterface.php
@@ -0,0 +1,32 @@
+  public function processOutbound($path, &$options = array(), Request $request = NULL);

We need an @return doc.

+++ b/core/lib/Drupal/Core/Routing/PathBasedGeneratorInterface.php
@@ -0,0 +1,137 @@
+   * @param $path
+   *   (optional) The internal path or external URL being linked to, such as
+   *   "node/34" or "http://example.com/foo". The default value is equivalent to
+   *   passing in '<front>'. A few notes:
+   *   - If you provide a full URL, it will be considered an external URL.
+   *   - If you provide only the path (e.g. "node/34"), it will be
+   *     considered an internal link. In this case, it should be a system URL,
+   *     and it will be replaced with the alias, if one exists. Additional query
+   *     arguments for internal paths must be supplied in $options['query'], not
+   *     included in $path.
+   *   - If you provide an internal path and $options['alias'] is set to TRUE, the
+   *     path is assumed already to be the correct path alias, and the alias is
+   *     not looked up.
+   *   - The special string '<front>' generates a link to the site's base URL.
+   *   - If your external URL contains a query (e.g. http://example.com/foo?a=b),
+   *     then you can either URL encode the query keys and values yourself and
+   *     include them in $path, or use $options['query'] to let this method
+   *     URL encode them.
+   * @param $options
+   *   (optional) An associative array of additional options, with the following
+   *   elements:
+   *   - 'query': An array of query key/value-pairs (without any URL-encoding) to
+   *     append to the URL.
+   *   - 'fragment': A fragment identifier (named anchor) to append to the URL.
+   *     Do not include the leading '#' character.
+   *   - 'absolute': Defaults to FALSE. Whether to force the output to be an
+   *     absolute link (beginning with http:). Useful for links that will be
+   *     displayed outside the site, such as in an RSS feed.
+   *   - 'alias': Defaults to FALSE. Whether the given path is a URL alias
+   *     already.
+   *   - 'external': Whether the given path is an external URL.
+   *   - 'language': An optional language object. If the path being linked to is
+   *     internal to the site, $options['language'] is used to look up the alias
+   *     for the URL. If $options['language'] is omitted, the language will be
+   *     obtained from language(LANGUAGE_TYPE_URL).
+   *   - 'https': Whether this URL should point to a secure location. If not
+   *     defined, the current scheme is used, so the user stays on HTTP or HTTPS
+   *     respectively. TRUE enforces HTTPS and FALSE enforces HTTP, but HTTPS can
+   *     only be enforced when the variable 'https' is set to TRUE.
+   *   - 'base_url': Only used internally, to modify the base URL when a language
+   *     dependent URL requires so.
+   *   - 'prefix': Only used internally, to modify the path when a language
+   *     dependent URL requires so.
+   *   - 'script': Added to the URL between the base path and the path prefix.
+   *     Defaults to empty string when clean URLs are in effect, and to
+   *     'index.php/' when they are not.
+   *   - 'entity_type': The entity type of the object that called url(). Only
+   *     set if url() is invoked by Drupal\Core\Entity\Entity::uri().
+   *   - 'entity': The entity object (such as a node) for which the URL is being
+   *     generated. Only set if url() is invoked by Drupal\Core\Entity\Entity::uri().

Seems like some of this doc shouldn't be on the interface, since it's implementation specific.

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -36,23 +81,262 @@ class UrlGenerator extends ProviderBasedGenerator {
+    $path = $this->processPath($path);

Dreditor isn't showing the method context, but the above is for the generate() method (not generateFromPath()). If $absolute was passed in as TRUE, then this won't work, because path processing needs to happen on the path, not the URL.

unfortunately non drupal life means I won't be able to get to this to the end of the week

-  $url = url($path, $options);
+  $url = Drupal::service('router.generator')->generateFromPath($path, $options);

This doesn't fly for me, DX-wise.

- We should get Drupal::service('router.generator') moved to Drupal::routerGenerator() or whatever. (Or ideally just router(), but not sure if that makes sense.) Can be a separate issue that we can get in quickly, then re-roll this patch to include it.
- Unless there's a compelling Symfony reason for this (doesn't look like it, since it seems to be our own interface), generateFromPath() should be just url() or, at worst, something like generate(). I can see from the function signature that $path is an argument to the function; those sort of implementation details are weird in a method name.

Also, since this still needs profiling, it's not RTBC.

While this makes sense as a refactoring in general, can the issue summary be updated to explain why it's critical? There's something in the comments about it being needed for ESI, but why is that?

It would also be great for the issue summary to include a before/after example of how a contrib module would alter a URL before this (via a hook) vs. after this (via a tagged service). We'll need this for a change notice anyway, but doing it before commit will help clarify the DX impact.

And I discussed this with xjm, who was concerned about generate() being broken when $absolute=TRUE: is it a critical bug, or does it need docs explaining why it isn't?

@effulgentsia - nice catch on the absolute paths problem! Bad me for not spotting that - it should be easy enough to take care of so I'll add a fix and a test in my next reroll unless someone else gets to it first.

I also had a question about our expectations for url(), which is related to @webchick's question in #143. Are we planning to deprecate url()? Where and when should it be used? Let's document that on the function. We're ripping a bunch of stuff out of the docblock for url() but not adding anything back other than an @see, which is not helpful for developers deciding whether they should use it.

Augh crossposts.

Is there any reason it can't just be Drupal::url($path, $options)?

Another question -- Does @Crell's feedback in #109 sufficiently answer @Damien Tournoud's concerns about whether we should be contributing something upstream instead? I don't see any reply from DamZ after that comment.

+10909340909304 to #150.

Is there any reason it can't just be Drupal::url($path, $options)?

We don't currently add random procedural functions into the Drupal class. We only add service getters into there. So I'd understand Drupal::routerGenerator(), but not Drupal::url(). If we want to retain url() as a convenient shorthand for procedural code, why not leave it in the global namespace?

Are we planning to deprecate url()? Where and when should it be used? Let's document that on the function.

Specifically, I'm assuming that we plan on instructing people that within OO code, they should have $routerGenerator injected, and call $this->routerGenerator->generateFromPath() instead of url(). If I'm understanding correctly, would be nice for docs in url() to say that.

Side note: I'm not crazy about the name routerGenerator in the above examples, but maybe that can be bikeshed elsewhere.

We don't currently add random procedural functions into the Drupal class. We only add service getters into there.

That's only sort of true. For instance, we don't do \Drupal::configFactory->get('foo') but we directly do \Drupal::config('foo'). You're right, of course, though, that most of the static methods simply return the service directly.

Just wanted to point that out that doing \Drupal::url($path, $options) wouldn't be totally unheard of. I'm not sure whether I myself would actually be for that, however.

Almost half of the current ones return something other than the top-level service object, actually. However, they do all return some kind of object... Drupal::url() would be the first to return a string, so I guess that could be confusing.

However, it's still returning something that ultimately comes from a swappable service, which I thought was the main point.

However, they do all return some kind of object

Yeah, I think of all of the current Drupal:: methods as returning either non-parameterized or parameterized services. For the parameterized ones, some are implemented with service id concatenation (i.e., Drupal::cache()) and others with factories (i.e., Drupal::config()).

I think a Drupal::url() would establish a precedent for just moving all D7 global utility functions into that class, which I'm not necessarily opposed to, though I didn't think that's what we created the class for, but maybe I'm wrong.

I dont understand why we are talking about url() here, since this patch does not remove the url() function? non-injected code can keep calling url()...no differences..So, profiling is done, whats left are the absolute URLs right according to #147

Ah forgot to add tag

an aggregation of 20 runs puts this at a 1.3% regression, so we're not losing much

Per #1744302-10: [meta] Resolve known performance regressions in Drupal 8, if we were really losing 1.3% of total request time relative to HEAD on a front page with 10 teasers, I would actually consider that to be a lot, because 8.x is already 500% slower on that page than 7.x, so that 1.3% is relative to an unreasonably large denominator. If we're able to claw back the most egregious regressions in 8.x, then that 1.3% will balloon to over 6% just by virtue of getting the denominator back to 7.x levels, and at least when we worked on D7, we considered 6% for a single issue to be a lot.

However, I'm unable to replicate a reliable 1.3% regression. In running various ab -c1 -n100 runs of HEAD vs. #158, I see some runs where patch results in a lower median, and some runs where it results in a higher one. For some reason, today, my machine seems to be generating too much noise to get a statistically significant result.

So instead, I decided to microbenchmark url() using the attached code.

I found that in HEAD, a simple url() with no $options passed takes 82us, while with #158, it takes 91us. So, for a page with ~100 links, that adds 1ms, or 0.2% to the scenario in #158. However, I found that with the patch, if instead of calling url(), you already have a $generator object available, and then can use it for multiple $generator->generateFromPath() calls, then that only takes 77us (faster than HEAD's url()), so we can likely neutralize the performance impact by making places that frequently generate URLs use an injected $generator.

In summary, from what I can tell, this issue's performance impact (whether positive or negative) is trivial, so the issue should be exclusively evaluated on other grounds (architecture, DX, etc.). However, that's based on questioning the 1.3% result of #158, so if someone can dig into that and see how stable it is, that would be helpful, because if it's stable, it could be pointing to some code flow impact of the patch that's not entirely attributable to url() (but from reading the patch, I don't readily see what that can be).

Per #145, we still need a better issue summary. I'll work on a draft, but might need help with getting it refined.

Sorry. I tried to come up with an issue summary, but couldn't. I scanned through #1597696: Consider whether HttpCache offers any significant benefit over the existing page cache, #1945024: Remove subrequests from central controllers and use the controller resolver directly., and http://symfony.com/blog/new-in-symfony-2-2-the-new-fragment-sub-framework, but am still confused as to:

- What Drupal code will need to call $generator->generate() and why? Will it only be SCOTCH code that integrates ESI support for blocks, or will we want regular modules to switch to generating URLs from route names rather than system paths? If only SCOTCH code, then why must it use that? I don't see anything in HttpKernel that requires it. All I see is that Drupal's implementation of HttpKernel needs to extend generateFragmentUri(), but I don't see why that needs to connect to using $generator->generate().

- Related to above, do we need separate implementations of generate() and generateFromPath()? Can we just have one? I.e., either make SCOTCH use generateFromPath() or make all of Drupal use generate()?

- I think the only motivation for this issue that I understand is that Drupal's HttpKernel will need to call either generate() or generateFromPath(), and in either case, we want it staying purely OOP rather than calling out to a global url() function. And that then buys us all the standard benefits of unit testability, etc. Which I think are all good things, but I don't yet understand why that makes this a critical issue. Seems like all "nice to haves", but is there something I'm missing?

- So in sum, what work is blocked on this and why?

Because this is still needs work on fixing a bug, removing the Avoid Commit Conflicts tag.

Despite my questions in #163, after a good night's sleep, I took a stab at an issue summary. Please correct as needed.

Thanks @effulgentsia and @beejeebus for moving this forward - I have been totally snowed under at work lately and haven't been able to get back to this patch to fix the problem with absolute URLs in the generate() method and anything else that needs doing :-/

I hope to be able to spend some time on it soon, but in the meantime if anyone else wants to have at it please feel free!

Created a new issue summary based on the template

Just on this bit from the issue summary:

Drupal's HttpKernel, which is a copy of Symfony FrameworkBundle's HttpKernel, also needs to be able to generate URLs to various Drupal routes, for purposes of HTTP-based partial caching such as ESI, SSI, or hInclude. It currently does this by invoking UrlGeneratorInterface::generate() on the router service. This interface is based on generating URLs from a route name, not from a Drupal system path. In #1945024: Switch from FrameworkBundle's HttpKernel to rendering strategies, some of the details of this will change, but what will still remain is the need for some way of exposing Drupal's URL generation logic to the Symfony code that deals with HTTP caching strategies.

There's only one call to the generator, and it's only for the _internal route, so it'd be completely possible for us to override that method to hard code it to a specific route and it'd work completely fine otherwise. So this doesn't actually block HttpCache at all, much less *SI in general (there's plenty of other *SI blockers out there).

What this patch does enable is having routes decoupled from paths - so eventually it'll be possible to move 'node/%' and have everything still work, similar to what entity_uri() was after in Drupal 7 but a lot more generic, that's really the only practical reason to do this, but it'll mean converting all url() calls to have it work, as well as changing logic in system module which for example looks for all links under /admin etc. to actually enable that feature to be used reliably.

Ok, then I don't think this needs to be critical (I say as the person who moved it there originally :)). "it'll be possible to move 'node/%' and have everything still work" sounds like a straight-up nice-to-have to me, but I'll split the difference at "major."

I don't relish the idea of embarking on a "convert all uses of url() to something else" quest this late in the release cycle. We already have numerous "convert all the things" initiatives that aren't getting done quick enough to make the July 1 deadline. :\

I had promised to get a new patch up today and I have failed :-( Didn't get as much time on it as I'd hoped and the absolute URLs problem is proving less straight-forward than I'd originally thought. At first I thought we could just pass FALSE for the $absolute param to the parent method and deal with it the same way we deal with it in generateFromPath(), but that doesn't work as there is logic in symfony's doGenerate() method that will set it to TRUE regardless of what's passed in because of requirements.

So it looks like parsing the url after calling the parent generate() method is the way to go. But that gets messy too and I don't have time right now to figure it out - I'll have to get back to it at the weekend.

Instead of calling parent::generate(), can't we directly call doGenerate() and remove _scheme from the $requirements array that we pass to it? That's hacky, but I think less hacky then parsing the URL after getting it.

Alternatively, it would be cool if we could get rid of Drupal's implementation of absolute URL handling and use Symfony's, but I think that would require an upstream change to let us insert path processors into the middle of doGenerate(), so is probably out of scope for this issue.

Here's an attempt to get around the problem with absolute URLs. It's not very pretty :-/

Oops, just saw a problem with that last patch. Here's a fix.
Also, I should explain the approach here. It's basically what @effulgentsia suggests in #173, so we don't call the parent's generate method, which means therefore having to copy some code from it. And then we call the doGenerate() method directly, having modified the requirements array, and passing FALSE for the absolute param.

I have included one test for this but it does not feel like adequate coverage (for example, the mistake in my last patch would not have been picked up at all). It would be nice to test with a base path too, I'm just not sure how to do that in a unit test.

Fixing the things...

Rerolled after language constants moved to class constants...

Latest patch addresses some outstanding issues from #141, also renames the generator service from 'router.generator' to 'url_generator', and renames PathGeneratorNotInitializedException to GeneratorNotInitializedException.

Given #1888424-169: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence assigning this to catch

So, the comment from @ParisLiakos in #160 says this:

I dont understand why we are talking about url() here, since this patch does not remove the url() function? non-injected code can keep calling url()...no differences..

But the patch still does this:

@@ -722,8 +695,7 @@ function drupal_goto($path = '', array $options = array(), $http_response_code =
   // The 'Location' HTTP header must be absolute.
   $options['absolute'] = TRUE;
 
-  $url = url($path, $options);
-
+  $url = Drupal::service('url_generator')->generateFromPath($path, $options);

So is that just in there by mistake? (It does look like the only such instance in the patch.)

The fact that the patch contained changes like that was how the whole conversation about url() started in the first place...

+++ b/core/includes/common.incundefined
@@ -495,42 +496,14 @@ function drupal_get_query_array($query) {
+ * @deprecated as of Drupal 8.0. Use
+ *   Drupal::service('url_generator')->httpBuildQuery() instead.

For this and others it'd be good to put the generator as a dedicated method on Drupal. I think this was discussed much earlier in the issue but not sure what the outcome was..

+++ b/core/includes/install.core.incundefined
@@ -1916,7 +1918,12 @@ function install_finished(&$install_state) {
-  $output .= '<p>' . (isset($messages['error']) ? st('Review the messages above before visiting <a href="@url">your new site</a>.', array('@url' => url(''))) : st('<a href="@url">Visit your new site</a>.', array('@url' => url('')))) . '</p>';
+  $request = Request::createFromGlobals();
+  $generator = Drupal::service('url_generator');
+  $generator->setBasePath(str_replace('/core', '', $request->getBasePath()) . '/');
+  $generator->setScriptPath('');

This could at least use a comment as to why we're going to so much trouble.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -3223,4 +3236,50 @@ protected function verboseEmail($count = 1) {
+  /**
+   * Creates a mock request and sets is on the generator.

'sets is on the generator'.

Couldn't find much else, overall this looks great.

#180: 1888424.url_generator.180.patch queued for re-testing.

Needs a reroll plus fixes for #186. Please assign back to catch when this is RTBC again.

Reroll was not totally straight-forward because of the UrlValidator utility that got added, but hopefully this works...

Also addressed catch's points.

Looks great to me.

Re-assigning back to catch...

Catch has indicated in #186 this looks great... his concerns have been addressed. Let's get stuff done.

Committed b219439 and pushed to 8.x. Thanks!

+++ b/core/modules/language/tests/language_test/language_test.moduleundefined
@@ -119,5 +119,18 @@ function language_test_menu() {
 function language_test_subrequest() {
-  return drupal_container()->get('http_kernel')->handle(Request::create('/user'), HttpKernelInterface::SUB_REQUEST);
+  $request = Request::createFromGlobals();
+  $server = $request->server->all();
+  if (basename($server['SCRIPT_FILENAME']) != basename($server['SCRIPT_NAME'])) {
+    // We need this for when the test is executed by run-tests.sh.
+    // @todo Remove this once run-tests.sh has been converted to use a Request
+    //   object.
+    $server['SCRIPT_FILENAME'] = $server['SCRIPT_NAME'];
+    $base_path = ltrim($server['REQUEST_URI'], '/');
+  }
+  else {
+    $base_path = $request->getBasePath();
+  }
+  $subrequest = Request::create($base_path . '/user', 'GET', $request->query->all(), $request->cookies->all(), array(), $server);
+  return Drupal::service('http_kernel')->handle($subrequest, HttpKernelInterface::SUB_REQUEST);

This code change has been moved to new routing system on #1987728-8: Convert language_test_subrequest() to a new style controller and just re-rolled it. Would be great, if someone from this issue can review and set it RTBC (It is just this code change from callback to controller method).

https://drupal.org/contributor-tasks/draft-change-notification
is instructions for drafting a change notice.
Anyone can try the first draft. :)
Just make a comment saying you are going to try, and ask questions if you have any.