Problem/Motivation

  • Drupal's url() function converts a system path (like 'node/1') into a relative or absolute URL. In doing this, it has logic to apply path aliasing, allow modules to alter the outbound URL (e.g., add a language prefix), prefix an explicit script path for web servers that don't support clean URLs, and generate an absolute URL based on various global variables and settings.php settings.
  • 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: Remove subrequests from central controllers and use the controller resolver directly., 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.
  • New-router routes are defined not by path, but by machine name. That allows, among other things, the actual path to be decoupled from references to it, allowing for a more flexible and self-documenting way to generate links. However, prior to this issue such links do not go through the full set of link manipulations that url() does.
  • Currently, Drupal's UrlGenerator::generate() method used by HttpKernel and the url() function used by the rest of Drupal diverge widely. For example, the former does not invoke the hook_url_outbound_alter() hooks that the latter does. And they follow completely different code paths for generating an absolute URL.

Proposed resolution

  • Move the contents of url() into a new method of UrlGenerator: generateFromPath(). That way, its logic is available via the router service used by HttpKernel, and all other code that works with dependency injected services.
  • In the process, refactor the dependencies on global variables and settings into injected ones.
  • Convert hook_url_outbound_alter() implementations into services tagged path_processor_outbound. This complements a similar hook_url_inbound_alter() to path_processor_inbound conversion that has already been done in HEAD. This avoids a low-level component such as UrlGenerator having a dependency on Drupal's module system, while still enabling modules to affect the URL generation process.
  • Move the application of path aliases from being inlined in generate() and url() to being done by the path_processor_alias service that's already in HEAD. This puts outbound aliasing code in the same place as inbound de-aliasing code.
  • With generate() and generateFromPath() being two methods in the same class, make all of their common functionality handled by a common helper method.
  • Leave url() as a (non-deprecated) wrapper function for now. Punt the question of whether to deprecate it to a follow up.

Remaining tasks

  • Fix generate() to invoke path processors prior to generating an absolute URL.

User interface changes

None.

API changes

See "Proposed resolution" above.

Original report by Crell

As a follow-up from #1874500: CMF-based Routing system, we now have a generator (which works on routes) and url() (which works on arbitrary paths). Only the latter uses hook_url_outbound_alter(), but both hard-code and special-case path aliasing.

This needs to all fold down to one clean system. Quoting from the linked issue:

That includes merging the caching of path lookups, generator included, and unifying hook_url_outbound_alter() into an event that we can cache *after* lookup, not before. That in turn means that path aliases become just another hook_url_outbound_alter()-equivalent listener, which is fine because we're caching it. That would also parallel the inbound-alter logic, which has already been converted to the request listener.

That does lose us the ability to do uncached runtime outbound url manipulation, but I think that's a fairly niche case and an acceptable trade off to a unified outbound URL cache.

This will involve moving url() into the generator, removing its external dependencies, and then unifying the caching logic from path aliasing into a generator-aliasing strategy.

Files: 
CommentFileSizeAuthor
#190 1888424.url_generator.190.patch111.16 KBkatbailey
PASSED: [[SimpleTest]]: [MySQL] 55,305 pass(es).
[ View ]
#190 interdiff.txt3.83 KBkatbailey
#180 1888424.url_generator.180.patch113.54 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1888424.url_generator.180.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#180 interdiff.txt20.59 KBkatbailey
#179 1888424.url_generator.179.patch113.02 KBkatbailey
PASSED: [[SimpleTest]]: [MySQL] 55,755 pass(es).
[ View ]
#177 1888424.url_generator.177.patch112.98 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1888424.url_generator.177.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#177 interdiff.txt4.07 KBkatbailey
#175 1888424.url_generator.175.patch111.26 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] 55,816 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
#175 interdiff.txt585 byteskatbailey
#174 1888424.url_generator.174.patch111.25 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] 57,005 pass(es), 25 fail(s), and 1 exception(s).
[ View ]
#174 interdiff.txt4.61 KBkatbailey
#162 bench.txt1.75 KBeffulgentsia
#159 1.urls_.xhprof.gz70.91 KBbeejeebus
#159 2.urls_.xhprof.gz71.66 KBbeejeebus
#158 1888424.url_generator.158.patch109.02 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 54,724 pass(es).
[ View ]
#133 1888424.url_generator.133.patch109.21 KBtwistor
PASSED: [[SimpleTest]]: [MySQL] 54,320 pass(es).
[ View ]
#133 interdiff.txt3.22 KBtwistor
#130 1888424.url_generator.130.patch109.88 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] 54,319 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#130 interdiff.txt466 byteskatbailey
#129 1888424.url_generator.129.patch109.89 KBkatbailey
PASSED: [[SimpleTest]]: [MySQL] 54,373 pass(es).
[ View ]
#129 interdiff.txt748 byteskatbailey
#127 1888424.url_generator.127.patch109.86 KBkatbailey
PASSED: [[SimpleTest]]: [MySQL] 54,349 pass(es).
[ View ]
#127 interdiff.txt1.14 KBkatbailey
#125 1888424.url_generator.125.patch109.86 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] 54,212 pass(es), 31 fail(s), and 2 exception(s).
[ View ]
#125 interdiff.txt517 byteskatbailey
#123 1888424.url_generator.123.patch110.19 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] 54,308 pass(es), 33 fail(s), and 1,362 exception(s).
[ View ]
#119 1888424.outgoing_path_futzing.119.patch110.29 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1888424.outgoing_path_futzing.119.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#119 interdiff.txt1.59 KBkatbailey
#118 1888424.outgoing_path_futzing.118.patch108.88 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1888424.outgoing_path_futzing.118.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#118 interdiff.txt7.01 KBkatbailey
#114 1888424.outgoing_path_futzing.114.patch110.31 KBkatbailey
PASSED: [[SimpleTest]]: [MySQL] 54,152 pass(es).
[ View ]
#114 interdiff.txt3.38 KBkatbailey
#111 1888424.outgoing_path_futzing.111.patch111.08 KBkatbailey
PASSED: [[SimpleTest]]: [MySQL] 53,271 pass(es).
[ View ]
#111 interdiff.txt5.56 KBkatbailey
#110 1888424.outgoing_path_futzing.110.patch111.73 KBkatbailey
PASSED: [[SimpleTest]]: [MySQL] 53,236 pass(es).
[ View ]
#110 interdiff.txt8.91 KBkatbailey
#106 1888424.outgoing_path_futzing.106.patch106.49 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] 52,145 pass(es), 1 fail(s), and 26 exception(s).
[ View ]
#102 1888424.outgoing_path_futzing.102.patch106.49 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#102 interdiff.txt1.96 KBkatbailey
#100 1888424.outgoing_path_futzing.100.patch106.44 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] 48,566 pass(es), 794 fail(s), and 458 exception(s).
[ View ]
#100 interdiff.txt750 byteskatbailey
#98 1888424.outgoing_path_futzing.98.patch106.49 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] 25,953 pass(es), 16,323 fail(s), and 7,186 exception(s).
[ View ]
#98 interdiff.txt14.16 KBkatbailey
#96 1888424.outgoing_path_futzing.96.patch105.52 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] 25,960 pass(es), 16,321 fail(s), and 7,184 exception(s).
[ View ]
#96 interdiff.txt1.89 KBkatbailey
#94 1888424.outgoing_path_futzing.93.patch105.63 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]
#91 1888424.outgoing_path_futzing.91.patch86.77 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1888424.outgoing_path_futzing.91.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#91 interdiff.txt34.32 KBkatbailey
#88 1888424.outgoing_path_futzing.88.patch100.95 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] 53,076 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#81 1888424.outgoing_path_futzing.81.patch101.42 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] 30,673 pass(es), 12,443 fail(s), and 1,579 exception(s).
[ View ]
#81 interdiff.txt1.91 KBkatbailey
#79 1888424.outgoing_path_futzing.79.patch101.08 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] 53,121 pass(es), 5 fail(s), and 1 exception(s).
[ View ]
#79 interdiff.txt6.7 KBkatbailey
#77 1888424.outgoing_path_futzing.77.patch100.94 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] 53,098 pass(es), 5 fail(s), and 1 exception(s).
[ View ]
#77 interdiff.txt11.98 KBkatbailey
#74 1888424.outgoing_path_futzing.74.patch99.46 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] 52,991 pass(es), 43 fail(s), and 3 exception(s).
[ View ]
#74 interdiff.txt11.1 KBkatbailey
#70 1888424.outgoing_path_futzing.70.patch95.28 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] 53,101 pass(es), 40 fail(s), and 3 exception(s).
[ View ]
#70 interdiff.txt11.13 KBkatbailey
#68 1888424.outgoing_path_futzing.68.patch87.19 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] 52,834 pass(es), 20 fail(s), and 1 exception(s).
[ View ]
#68 interdiff.txt10.55 KBkatbailey
#64 drupal-urlgenerator--1888424-64.patch88.58 KBsteveoliver
FAILED: [[SimpleTest]]: [MySQL] 52,439 pass(es), 14 fail(s), and 2 exception(s).
[ View ]
#64 drupal-urlgenerator--1888424-57-64--interdiff--do-no-test.patch5.37 KBsteveoliver
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-urlgenerator--1888424-57-64--interdiff--do-no-test.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#57 1888424.outgoing_path_futzing.57.patch84.7 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] 52,200 pass(es), 4 fail(s), and 2 exception(s).
[ View ]
#57 interdiff.txt2.86 KBkatbailey
#55 1888424.outgoing_path_futzing.55.patch83.16 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] 52,385 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#55 interdiff.txt5.1 KBkatbailey
#52 1888424.outgoing_path_futzing.52.patch85.24 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] 47,194 pass(es), 150 fail(s), and 176 exception(s).
[ View ]
#52 interdiff.txt11.67 KBkatbailey
#49 1888424.outgoing_path_futzing.49.patch74.77 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] 52,060 pass(es), 15 fail(s), and 28 exception(s).
[ View ]
#49 interdiff.txt839 byteskatbailey
#44 1888424.outgoing_path_futzing.44.patch74.89 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#44 interdiff.txt43.09 KBkatbailey
#39 1888424.outbound_path_futzing.combined_with_inbound.39.patch60.92 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] 50,407 pass(es), 41 fail(s), and 17 exception(s).
[ View ]
#39 1888424.outbound_path_futzing.39.do-not-test.patch40.85 KBkatbailey
#37 1888424.outbound_path_futzing.combined_with_inbound.patch58.63 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#37 1888424.outbound_path_futzing-do-not-test.patch38.56 KBkatbailey
#31 1888424.path_processors.31.patch20.63 KBkatbailey
PASSED: [[SimpleTest]]: [MySQL] 49,193 pass(es).
[ View ]
#31 interdiff.txt9.8 KBkatbailey
#28 1888424.path_processors.combined.28.patch54.22 KBkatbailey
PASSED: [[SimpleTest]]: [MySQL] 48,718 pass(es).
[ View ]
#28 1888424.path_processors.review.do-no-test.patch20.19 KBkatbailey
PASSED: [[SimpleTest]]: [MySQL] 49,102 pass(es).
[ View ]
#24 incoming_fuzters_combined_with_language_fixes.durr_.patch50.44 KBkatbailey
PASSED: [[SimpleTest]]: [MySQL] 49,972 pass(es).
[ View ]
#20 incoming_fuzters_combined_with_language_fixes.patch50.67 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch incoming_fuzters_combined_with_language_fixes.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#14 incoming_fuzters_combined_with_language_fixes.14.patch50.67 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch incoming_fuzters_combined_with_language_fixes.14.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#14 path_futzers-do-not-test.patch18.8 KBkatbailey
#10 incoming-futzers-do-not-test.patch38.86 KBkatbailey

Comments

effulgentsia’s picture

Status:Active» Postponed

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

Crell’s picture

Status:Postponed» Active

Base issue is in, this is coming up next.

katbailey’s picture

Assigned:Unassigned» katbailey

Going to take a crack at this.

sun’s picture

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’s picture

Kat: One idea I had (which may or may not be good) is to replace the multiple path listeners on kernel.request with a single listener that proxies to a series of path fuzting objects. Each PathManipulator (or whatever) has an inbound and outbound method. On an incoming request, one event listener attaches to kernel.request and passes the path (and the request separately, for context) to the path manipulator manager class, which calls each manipulator in turn, then assigns the path to system_path at the end. On an outgoing request, the path manipulator is called directly from the generator and does the same thing, but on the outbound methods.

That makes it easy to provide parallel incoming/outgoing processing in a single object (although maybe that should be two separate interfaces?), and also reduces the event overhead on incoming requests from N listeners to 1 listener.

katbailey’s picture

@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

Crell’s picture

Well, I'm actually thinking that $this->setPath() goes away. Moving the language negotiator out to an earlier listener makes complete sense. However, I don't think it should do anything to the path. It should just set the language on $request->attributes somewhere.

Then, each Path Futzer has an interface something like: public function futz($path, $request);

And the FutzerManager has a loop like so:

<?php
$path
= $request->getPathInfo();
foreach (
$this->futzers as $futzer) {
 
$path = $futzer->futz($path, $request);
}
$request->attributes->set('system_path', $path);
?>

That minimizes the surface area where the system_path variable gets hard coded, and makes all futzers naturally stateless. That may result in a little duplication or overlap between the language initializing system and the language-path futzer, but I think that's fine until and unless benchmarks show it's actually a problem. I suspect that will be more than made up for by the reduction in the number of listeners we actually fire.

katbailey’s picture

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?

Crell’s picture

Eh, whatever gets it committed faster gets my vote. :-)

katbailey’s picture

StatusFileSize
new38.86 KB

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

Crell’s picture

Status:Active» Needs work

Yep, this is what I was thinking of on the incoming side. I can't speak to the language code specifically.

I actually really dig the way you made the managing class implement the manipulator interface itself. That's classy. (Pun intended.) When we add outgoing, I think the same model should work fine. Even if a given service implements both incoming and outgoing (as I expect aliasing will, for instance), there's still only a single instance of that object so it will show up in both internal lists in the manager, and all will be well.

If we ignore caching for the moment (which I recommend we do until the real code is figured out), the outgoing side should be reasonably similar. The manager object becomes a dependency of the generator, which in turn calls that on any URL/path that it generates, regardless of whether it comes from generate() or url(). That *may* be a little problematic, though, with generate() as we may get back a path that includes query parameters and fragments and such, which we would have to then reparse to alias. All the more reason to cache the results...

I'd actually be OK with just doing this refactoring on the incoming side first (leaving the caching exactly where it is), commiting that, and then doing outgoing in a later patch in the same issue. The patch is already up to nearly 40 KB, so incremental sounds like a good idea.

As far as the name, I agree "futzer" isn't a good name. :-) Manipulator sounds, eh, malevolent. Maybe just "PathProcessor"?

Also setting to needs work just to see what happens.

+++ b/core/lib/Drupal/Core/EventSubscriber/LanguageRequestSubscriber.php
@@ -0,0 +1,50 @@
+  public function onKernelRequestLanguage(GetResponseEvent $event) {
+    if ($event->getRequestType() == HttpKernelInterface::MASTER_REQUEST) {
+      $this->languageManager->setRequest($event->getRequest());
+    }

This could be an issue with subrequests. See: https://github.com/symfony/symfony/issues/6756 and https://github.com/symfony/symfony/issues/5300

katbailey’s picture

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

Crell’s picture

Status:Needs work» Needs review

And of course in #11 I meant "Setting to needs REVIEW to see what happens". Because I managed to screw that up twice in one comment. *sigh*

katbailey’s picture

StatusFileSize
new18.8 KB
new50.67 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch incoming_fuzters_combined_with_language_fixes.14.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

heddn’s picture

Status:Needs review» Needs work
Issue tags:-Framework Initiative, -API change

The last submitted patch, incoming_fuzters_combined_with_language_fixes.14.patch, failed testing.

katbailey’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+Framework Initiative, +API change

The last submitted patch, incoming_fuzters_combined_with_language_fixes.14.patch, failed testing.

Crell’s picture

Status:Needs work» Needs review

heddn: Redirect and similar modules will likely need to take a new approach, by registering their own request listener (either before or after this code, up to them) and creating a RedirectResponse themselves right there. That would then bypass the entire rest of the routing process. Or they could create a route at a given path that returns a redirect. Both are viable. Which one makes sense depends on the use case.

katbailey’s picture

StatusFileSize
new50.67 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch incoming_fuzters_combined_with_language_fixes.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs review» Needs work

The last submitted patch, incoming_fuzters_combined_with_language_fixes.patch, failed testing.

heddn’s picture

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.

Dave Reid’s picture

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.

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new50.44 KB
PASSED: [[SimpleTest]]: [MySQL] 49,972 pass(es).
[ View ]

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

Crell’s picture

heddn: I'm not sure which save operations you mean. Symfony2 fullstack doesn't have the concept of user-configuration-created routes. That's a Drupal thing (although I think Symfony CMF also supports it). By design, multiple routes can live at a single path but differ in other ways (mime type, HTTP method, etc.). The path de-futzing logic (this patch so far) would only deal with the path on the request *before* routing happens, so the route is irrelevant.

For an outgoing path, if you're linking by route then you supply a route name, case closed. If you're just specifying a path then it's no different than the url() function now.

I guess I don't see what your concern is. Do you have a more concrete example?

katbailey’s picture

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.

Crell’s picture

Yo dawg, I heard you liked futzing with paths, so I made a futzer that depends on a futzer so you can futz with your futzing.

(Sorry, too easy.)

katbailey’s picture

StatusFileSize
new20.19 KB
PASSED: [[SimpleTest]]: [MySQL] 49,102 pass(es).
[ View ]
new54.22 KB
PASSED: [[SimpleTest]]: [MySQL] 48,718 pass(es).
[ View ]

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?

Status:Needs review» Needs work
Issue tags:-Framework Initiative, -API change

The last submitted patch, 1888424.path_processors.review.do-no-test.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
Issue tags:+Framework Initiative, +API change

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

The language manager issue went in.

katbailey’s picture

StatusFileSize
new9.8 KB
new20.63 KB
PASSED: [[SimpleTest]]: [MySQL] 49,193 pass(es).
[ View ]

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.

Status:Needs review» Needs work
Issue tags:-Framework Initiative, -API change

The last submitted patch, 1888424.path_processors.31.patch, failed testing.

katbailey’s picture

Status:Needs work» Needs review
Issue tags:+Framework Initiative, +API change

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

Crell’s picture

+++ b/core/lib/Drupal/Core/PathProcessor/IncomingPathProcessorInterface.php
@@ -22,7 +22,10 @@
+  public function processIncoming($path, Request $request, $request_type);

I feel like $request_type should have a default value of MASTER_REQUEST, since most processors won't actually care.

Note that this requirement does impose a dependency on HttpKernel; without that processors were just dealing with strings so are conceptually entirely stand-alone. (Almost but not quite Component material.)

+++ b/core/modules/language/lib/Drupal/language/PathProcessorLanguage.php
@@ -25,12 +26,14 @@ public function __construct(ModuleHandlerInterface $module_handler) {
+    // Language-based patch resolution only happens for the master request.

I think you mean path resolution, not patch resolution. :-)

katbailey’s picture

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?

katbailey’s picture

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.

katbailey’s picture

StatusFileSize
new38.56 KB
new58.63 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 1888424.outbound_path_futzing.combined_with_inbound.patch, failed testing.

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new40.85 KB
new60.92 KB
FAILED: [[SimpleTest]]: [MySQL] 50,407 pass(es), 41 fail(s), and 17 exception(s).
[ View ]

OK, let's try this...

Status:Needs review» Needs work

The last submitted patch, 1888424.outbound_path_futzing.combined_with_inbound.39.patch, failed testing.

Crell’s picture

+++ b/core/lib/Drupal/Core/CoreBundle.php
@@ -320,13 +320,13 @@ protected function registerRouting(ContainerBuilder $container) {
-    $container->register('router.generator', 'Drupal\Core\Routing\UrlGenerator')
+    $container->register('url_generator', 'Drupal\Core\Routing\UrlGenerator')

Why the rename?

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -36,23 +36,114 @@ class UrlGenerator extends ProviderBasedGenerator {
+      $options['external'] = ($colonpos !== FALSE && !preg_match('![/?#]!', substr($path, 0, $colonpos)) && drupal_strip_dangerous_protocols($path) == $path);

strip_dangerous_protcols() is a rather silly function. We should probably turn it into a method of the generator, with a better name. :-)

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -36,23 +36,114 @@ class UrlGenerator extends ProviderBasedGenerator {
+      if (isset($options['https']) && variable_get('https', FALSE)) {

Flagging this as something for Greg to convert to CMI, which will likely mean a dependency for the Generator.

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -36,23 +36,114 @@ class UrlGenerator extends ProviderBasedGenerator {
+    global $base_url, $base_secure_url, $base_insecure_url;

I think we can get this data from the Request. Fabien is working on an issue to let services get a request re-set on them when the DIC scope changes without having to reinitialize the object. That would let us set the request on the Generator and then use it in place of these globals. Fewer globals++.

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -36,23 +36,114 @@ class UrlGenerator extends ProviderBasedGenerator {
+    $query = $options['query'] ? ('?' . drupal_http_build_query($options['query'])) : '';

Another function that should probably just turn into a method on this object. (Is it even needed anymore in PHP 5.3?)

katbailey’s picture

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

Crell’s picture

What's wrong with just passing $options to the outbound futzer? Route-name-based paths would just have an empty array (presumably, or maybe there are equivalents), while path-based paths would pass through $options more or less as-is.

How many places use those utility functions? And more to the point... should they be? I'm not against a "path utils" object of some kind, but I question how many legitimate uses there will be once we're using the generator in more places. And some of them I question the point of.

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new43.09 KB
new74.89 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Status:Needs review» Needs work
Issue tags:-Framework Initiative, -API change

The last submitted patch, 1888424.outgoing_path_futzing.44.patch, failed testing.

katbailey’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+Framework Initiative, +API change

The last submitted patch, 1888424.outgoing_path_futzing.44.patch, failed testing.

chx’s picture

Issue tags:+Needs profiling

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

katbailey’s picture

Status:Needs work» Needs review
Issue tags:-Needs profiling
StatusFileSize
new839 bytes
new74.77 KB
FAILED: [[SimpleTest]]: [MySQL] 52,060 pass(es), 15 fail(s), and 28 exception(s).
[ View ]

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

katbailey’s picture

Issue tags:+Needs profiling

Oops

Status:Needs review» Needs work

The last submitted patch, 1888424.outgoing_path_futzing.49.patch, failed testing.

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new11.67 KB
new85.24 KB
FAILED: [[SimpleTest]]: [MySQL] 47,194 pass(es), 150 fail(s), and 176 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 1888424.outgoing_path_futzing.52.patch, failed testing.

katbailey’s picture

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

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new5.1 KB
new83.16 KB
FAILED: [[SimpleTest]]: [MySQL] 52,385 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

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 :-/

Status:Needs review» Needs work

The last submitted patch, 1888424.outgoing_path_futzing.55.patch, failed testing.

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new2.86 KB
new84.7 KB
FAILED: [[SimpleTest]]: [MySQL] 52,200 pass(es), 4 fail(s), and 2 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 1888424.outgoing_path_futzing.57.patch, failed testing.

katbailey’s picture

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 :-/

katbailey’s picture

Status:Needs work» Needs review
Issue tags:-Framework Initiative, -API change, -Needs profiling

Status:Needs review» Needs work
Issue tags:+Framework Initiative, +API change, +Needs profiling

The last submitted patch, 1888424.outgoing_path_futzing.57.patch, failed testing.

Damien Tournoud’s picture

<?php
+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:

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

<?php
+        if ($request !== NULL) {
+         
$https = $request->server->get('HTTPS');
+         
$is_https = $https && strtolower($https) == 'on';
+         
$http_host = $request->server->get('HTTP_HOST');
+        }
?>
Crell’s picture

There are a couple of places where we are doing that sort of "targeted pseudo-event". A number of Symfony systems do that, too. I've asked around Symfony circles for a clear guideline on when to do one or the other and so far no one has had a really solid answer. I'm still looking. ;-)

In general, though, the approach here has less overhead than the event dispatcher. The classes are not loaded if not needed (whereas with an event subscriber class the class is loaded to call the static registration method, even if the event is never called). The dispatch loop is considerably smaller (as in ~2 stack calls vs. ~8, something you've observed before, Damien), which helps performance and debuggability. And we know that we're going to be using all of the registered objects here, whereas events allow for a listener to terminate the loop, much like in Javascript. Along critical path, I think this makes sense.

Agreed that we should go ahead and clean up that code from PathProcessorLanguage.

steveoliver’s picture

StatusFileSize
new5.37 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-urlgenerator--1888424-57-64--interdiff--do-no-test.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new88.58 KB
FAILED: [[SimpleTest]]: [MySQL] 52,439 pass(es), 14 fail(s), and 2 exception(s).
[ View ]

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?

EclipseGc’s picture

Status:Needs work» Needs review

didn't get tests.

Status:Needs review» Needs work

The last submitted patch, drupal-urlgenerator--1888424-57-64--interdiff--do-no-test.patch, failed testing.

webchick’s picture

Priority:Major» Critical

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.

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new10.55 KB
new87.19 KB
FAILED: [[SimpleTest]]: [MySQL] 52,834 pass(es), 20 fail(s), and 1 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 1888424.outgoing_path_futzing.68.patch, failed testing.

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new11.13 KB
new95.28 KB
FAILED: [[SimpleTest]]: [MySQL] 53,101 pass(es), 40 fail(s), and 3 exception(s).
[ View ]

Gah, stupid tests modifying global variables :-(

Status:Needs review» Needs work

The last submitted patch, 1888424.outgoing_path_futzing.70.patch, failed testing.

katbailey’s picture

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.

thedavidmeister’s picture

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new11.1 KB
new99.46 KB
FAILED: [[SimpleTest]]: [MySQL] 52,991 pass(es), 43 fail(s), and 3 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 1888424.outgoing_path_futzing.74.patch, failed testing.

thedavidmeister’s picture

#74 - it got a little bit worse :(

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new11.98 KB
new100.94 KB
FAILED: [[SimpleTest]]: [MySQL] 53,098 pass(es), 5 fail(s), and 1 exception(s).
[ View ]

Wheeeeeee!

Status:Needs review» Needs work

The last submitted patch, 1888424.outgoing_path_futzing.77.patch, failed testing.

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new6.7 KB
new101.08 KB
FAILED: [[SimpleTest]]: [MySQL] 53,121 pass(es), 5 fail(s), and 1 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 1888424.outgoing_path_futzing.79.patch, failed testing.

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new1.91 KB
new101.42 KB
FAILED: [[SimpleTest]]: [MySQL] 30,673 pass(es), 12,443 fail(s), and 1,579 exception(s).
[ View ]

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

Status:Needs review» Needs work
Issue tags:-Framework Initiative, -API change, -Needs profiling

The last submitted patch, 1888424.outgoing_path_futzing.81.patch, failed testing.

katbailey’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1888424.outgoing_path_futzing.81.patch, failed testing.

katbailey’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+Framework Initiative, +API change, +Needs profiling

The last submitted patch, 1888424.outgoing_path_futzing.81.patch, failed testing.

katbailey’s picture

I... have no words

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new100.95 KB
FAILED: [[SimpleTest]]: [MySQL] 53,076 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 1888424.outgoing_path_futzing.88.patch, failed testing.

Crell’s picture

Review based on #79:

+++ b/core/lib/Drupal/Core/EventSubscriber/PathSubscriber.php
@@ -22,10 +23,12 @@ class PathSubscriber extends PathListenerBase implements EventSubscriberInterfac
   protected $aliasManager;
   protected $pathProcessor;
+  protected $generator;

These all need docblocks.

+++ b/core/lib/Drupal/Core/EventSubscriber/PathSubscriber.php
@@ -42,6 +45,7 @@ public function onKernelRequestConvertPath(GetResponseEvent $event) {
+    $this->generator->setRequest($request);

I'm curious why we're setting the request on the generator here, rather than via the DIC. Currently using the DIC would imply a new generator gets created on every subrequest, but that should change with

https://github.com/symfony/symfony/pull/7007

Perhaps we can leave it as is for now, but add a @todo to change it to use the DIC when that PR is merged and brought back downstream, as it should resolve the performance concern?

+++ b/core/lib/Drupal/Core/Routing/PathBasedGeneratorInterface.php
@@ -0,0 +1,119 @@
+interface PathBasedGeneratorInterface {

Shouldn't this extend UrlGeneratorInterface?

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -36,23 +94,227 @@ class UrlGenerator extends ProviderBasedGenerator {
+    $this->request = $request;
+    $this->basePath = $request->getBasePath() . '/';
+    $this->baseUrl = $request->getSchemeAndHttpHost() . $this->basePath;
+    $this->baseSecureUrl = str_replace('http://', 'https://', $this->baseUrl);
+    $this->baseInsecureUrl = str_replace('https://', 'http://', $this->baseUrl);
+    $this->scriptPath = '';
+    $base_path_with_script = $request->getBaseUrl();
+    $script_name = $this->request->getScriptName();
+    if (!empty($base_path_with_script) && strpos($base_path_with_script, $script_name) !== FALSE) {
+      $length = strlen($this->basePath);
+      $this->scriptPath = ltrim(substr($script_name, $length), '/') . '/';
+    }
+    $this->serverPropertiesInitialized = TRUE;

Wowsa. Is this all for performance optimization purposes?

+++ b/core/modules/language/lib/Drupal/language/HttpKernel/PathProcessorLanguage.php
@@ -7,31 +7,164 @@
+    $this->setRequest($request);

I don't understand why we're persisting the request object here, on both inbound and outbound. Won't the request be passed in when needed anyway?

I think we're relying on inbound being called before outbound. Which... I can't think of when it wouldn't be true, but smells a bit to me since those are two separate interfaces.

Otherwise, this looks very solid. Awesome work, Kat!

On the subject of caching, since you asked about it in IRC... I think it makes sense to do what we were doing before for the alias manager and use a wrapping cache object rather than building it into this class directly. We may have to maintain 2 caches, though, one for generate() and one for generateFromPath(), since I don't know that we can key them together. The logic for that is pretty much the same as for the alias manager: get the request, key based on the incoming path, save on exit using the new DestructableInterface. It *should* be fairly straightforward. (I know, famous last words.)

The question then is what happens to the cache wrapper on the alias manager. Does it really make sense to have two of them, or should we eliminate the one on alias manager and just let the generator cache it? I'm assuming that once the generator is in 99% of all alias lookups will be going through the generator, so caching in both places is needlessly redundant.

I would also be OK with the caching being a follow-up patch, since this is already 100 KB and it would probably be WAY easier to implement the cache wrapper once this is in, especially since this is a critical and nominally a blocker for the partial-page-caching we still want to do. (Although that MAY not be true given changes in Symfony; I need to revisit that question soon. Still, it would be good to put this to bed.)

As for why those tests are failing... I'll see if I can offer advise when it's not 1 am. My testing ability has run out for now. ;-)

katbailey’s picture

StatusFileSize
new34.32 KB
new86.77 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1888424.outgoing_path_futzing.91.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

katbailey’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1888424.outgoing_path_futzing.91.patch, failed testing.

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new105.63 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]

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 :-/

Status:Needs review» Needs work

The last submitted patch, 1888424.outgoing_path_futzing.93.patch, failed testing.

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new1.89 KB
new105.52 KB
FAILED: [[SimpleTest]]: [MySQL] 25,960 pass(es), 16,321 fail(s), and 7,184 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 1888424.outgoing_path_futzing.96.patch, failed testing.

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new14.16 KB
new106.49 KB
FAILED: [[SimpleTest]]: [MySQL] 25,953 pass(es), 16,323 fail(s), and 7,186 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 1888424.outgoing_path_futzing.98.patch, failed testing.

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new750 bytes
new106.44 KB
FAILED: [[SimpleTest]]: [MySQL] 48,566 pass(es), 794 fail(s), and 458 exception(s).
[ View ]

Stupid WebTests and run-tests.sh...

Status:Needs review» Needs work

The last submitted patch, 1888424.outgoing_path_futzing.100.patch, failed testing.

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new1.96 KB
new106.49 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

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

Damien Tournoud’s picture

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.

Status:Needs review» Needs work

The last submitted patch, 1888424.outgoing_path_futzing.102.patch, failed testing.

Crell’s picture

Damien: Registering objects directly on an object that will use them is not a one-off here. It's something we're doing in a lot of places, that Symfony and Symfony CMF do in various places, and is a perfectly legitimate design in OOP code.

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new106.49 KB
FAILED: [[SimpleTest]]: [MySQL] 52,145 pass(es), 1 fail(s), and 26 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 1888424.outgoing_path_futzing.106.patch, failed testing.

Damien Tournoud’s picture

@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"?

Crell’s picture

@Damien:

- Route Enhancers (from the Symfony CMF Routing system).
- Event subscribers themselves are registered onto the dispatcher the same way. (Tagged service that results in the compiled Container having lots of ->addSubscriber() calls in it.)
- The new FragmentRenderer system in Symfony 2.2.
- The Symfony translation system (see https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/Framew...)
- Serializers in Symfony (we're doing the same).

That's just from a quick glance at GitHub.

I'm not saying "reimplementing what exists" is legit. I'm saying "skipping the more involved event system" is legit, when we know that all of the involved objects are definitely going to be used when the parent object is used. Contrast to kernel events, eg, where view and exception events may never fire, and a request listener can set a response early and short-circuit the entire rest of the process, including later event listeners. It's just a run-of-the-mill observer, in a sense, or not even that complex.

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new8.91 KB
new111.73 KB
PASSED: [[SimpleTest]]: [MySQL] 53,236 pass(es).
[ View ]
katbailey’s picture

StatusFileSize
new5.56 KB
new111.08 KB
PASSED: [[SimpleTest]]: [MySQL] 53,271 pass(es).
[ View ]

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

katbailey’s picture

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

podarok’s picture

#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

katbailey’s picture

StatusFileSize
new3.38 KB
new110.31 KB
PASSED: [[SimpleTest]]: [MySQL] 54,152 pass(es).
[ View ]

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

steveoliver’s picture

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.

Crell’s picture

Crell’s picture

Overall this looks awesome. Unfortunately because it touches bundles its guaranteed to conflict with #1939660: Use YAML as the primary means for service registration, which is already RTBC and way harder to reroll. So we'll need at least one more update here once that lands.

I've noted some minor, mostly trivial issues below. Most are documentation-related, not functionality.

I think given the size and complexity of this patch we should commit it as is, sans-caching, and do that as a follow-up. I've opened such a follow-up here: #1965074: Add cache wrapper to the UrlGenerator

+++ b/core/includes/common.inc
@@ -477,42 +478,12 @@ function drupal_get_query_array($query) {
+ * @see \Drupal\Core\Routing\PathBasedGeneratorInterface::httpBuildQuery()
  * @see drupal_get_query_parameters()
  * @ingroup php_wrappers
  */
function drupal_http_build_query(array $query, $parent = '') {

We can probably add a @deprecated here as well, not just a @see. We'll want to remove this function.

+++ b/core/includes/common.inc
@@ -477,42 +478,12 @@ function drupal_get_query_array($query) {
+  return Drupal::Service('router.generator')->httpBuildQuery($query, $parent);

Drupal::service(). Lowercase.

+++ b/core/lib/Drupal/Core/Routing/PathBasedGeneratorInterface.php
@@ -0,0 +1,108 @@
+   *     include them in $path, or use $options['query'] to let this function

Technically this should now be "let this method..."

+++ b/core/lib/Drupal/Core/Routing/PathBasedGeneratorInterface.php
@@ -0,0 +1,108 @@
+  /**
+   * Sets the scriptPath property.
+   *
+   * @var string $path
+   *   The script path to use for url generation.
+   */
+  public function setScriptPath($path);

The docblock should say what a script path is. I don't actually know...

It may also be good to specify what baseUrl and basePath are in their respective methods, because the terms are not self-evident if you've not been using them for years. (Even then I've confused them before.)

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -36,23 +73,260 @@ class UrlGenerator extends ProviderBasedGenerator {
+  /**
+   * Implements \Drupal\Core\Routing\PathBasedGeneratorInterface::setBaseUrl().

Technically we're moving over to {@inheritdoc) now. Although given how many other things we have to update with that change I'm fine not changing it here if it makes rerolling harder.

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -36,23 +73,260 @@ class UrlGenerator extends ProviderBasedGenerator {
+   *   Returns TRUE if the server variables have been set, FALSE otherwise.

Server variables?

+++ b/core/modules/image/image.module
@@ -791,23 +791,35 @@ function image_style_flush($style) {
+      $request = Drupal::service('request');

As of this weekend, we can now do Drupal::request().

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguagePathMonolingualTest.php
@@ -40,15 +40,18 @@ function setUp() {
     $edit = array();
     $edit['predefined_langcode'] = 'fr';
     $this->drupalPost('admin/config/regional/language/add', $edit, t('Add language'));
+    $this->rebuildContainer();

     // Make French the default language.
     $edit = array(
       'site_default_language' => 'fr',
     );
     $this->drupalpost('admin/config/regional/settings', $edit, t('Save configuration'));
+    $this->rebuildContainer();

     // Delete English.
     $this->drupalPost('admin/config/regional/language/delete/en', array(), t('Delete'));
+    $this->rebuildContainer();

Oh geez. Dare I ask why we need to rebuild the container 3 times in one setUp() method? And dare I ask how hard it is going to be to dissect the tests so that we don't have to do that and can just unit test things properly?

+++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php
@@ -169,6 +169,16 @@ public function containerBuild(ContainerBuilder $container) {
+    if ($container->hasDefinition('path_processor_alias')) {
+      // Prevent the alias-based path processor, which requires a url_alias db
+      // table, from being registered to the path processor manager. We do this
+      // by removing the tags that the compiler pass looks for. This means the
+      // url generator can safely be used within DUTB tests.
+      $definition = $container->getDefinition('path_processor_alias');
+      $definition->clearTag('path_processor_inbound')->clearTag('path_processor_outbound');
+    }

Clever. :-)

+++ b/core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTest.php
@@ -0,0 +1,178 @@
+ */
+class UrlGeneratorTest extends UnitTestCase {

You just made Mark's Christmas list. :-) (You're already on mine.)

katbailey’s picture

StatusFileSize
new7.01 KB
new108.88 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1888424.outgoing_path_futzing.118.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

katbailey’s picture

StatusFileSize
new1.59 KB
new110.29 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1888424.outgoing_path_futzing.119.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Status:Needs review» Needs work
Issue tags:-Framework Initiative, -API change, -Needs profiling

The last submitted patch, 1888424.outgoing_path_futzing.119.patch, failed testing.

jibran’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work
Issue tags:+Framework Initiative, +API change, +Needs profiling

The last submitted patch, 1888424.outgoing_path_futzing.119.patch, failed testing.

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new110.19 KB
FAILED: [[SimpleTest]]: [MySQL] 54,308 pass(es), 33 fail(s), and 1,362 exception(s).
[ View ]

Minor conflict, rerolled...

Status:Needs review» Needs work

The last submitted patch, 1888424.url_generator.123.patch, failed testing.

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new517 bytes
new109.86 KB
FAILED: [[SimpleTest]]: [MySQL] 54,212 pass(es), 31 fail(s), and 2 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 1888424.url_generator.125.patch, failed testing.

katbailey’s picture

Status:Needs review» Needs work
StatusFileSize
new1.14 KB
new109.86 KB
PASSED: [[SimpleTest]]: [MySQL] 54,349 pass(es).
[ View ]

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

katbailey’s picture

Status:Needs work» Needs review
katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new748 bytes
new109.89 KB
PASSED: [[SimpleTest]]: [MySQL] 54,373 pass(es).
[ View ]

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

katbailey’s picture

StatusFileSize
new466 bytes
new109.88 KB
FAILED: [[SimpleTest]]: [MySQL] 54,319 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

Crell’s picture

Status:Needs review» Reviewed & tested by the community

I've reviewed the last several interdiffs. (Yay, interdiffs!) I'm going to go out on a limb and say 'LET'S DO EET!!!" We already have a follow-up to add caching, and this is around the 100 KB threshold of doom.

Unless testbot objects, this is ready to go.

katbailey++

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 1888424.url_generator.130.patch, failed testing.

twistor’s picture

Status:Needs work» Needs review
StatusFileSize
new3.22 KB
new109.21 KB
PASSED: [[SimpleTest]]: [MySQL] 54,320 pass(es).
[ View ]

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

Crell’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:+Avoid commit conflicts

Sigh. Come on, testbot!

alexpott’s picture

Assigned:katbailey» alexpott

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

catch’s picture

Did anyone profile this?

Crell’s picture

catch: Since there's no caching on it yet, I don't know that a profiling would give much relevant data. The plan is to cache the output in #1965074: Add cache wrapper to the UrlGenerator.

catch’s picture

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.

beejeebus’s picture

i'll do some profiling today.

katbailey, Crell: what's a reasonable test set up here?

Crell’s picture

Home page with 20 nodes on it. That's a bunch of links.

To compare hook_url_outbound_alter(), which goes away in this patch but instead is "always on", essentially, add a custom_url_outbound_alter() implementation that returns the path without modification. That would force that part of the existing pipeline to fire for a more fair comparison.

effulgentsia’s picture

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.

alexpott’s picture

Assigned:alexpott» Unassigned

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

webchick’s picture

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

webchick’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+DX (Developer Experience)

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

effulgentsia’s picture

Status:Needs work» Needs review
Issue tags:-DX (Developer Experience)+Needs issue summary update

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’s picture

Status:Needs review» Needs work
Issue tags:+DX (Developer Experience)
katbailey’s picture

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

xjm’s picture

Status:Needs work» Needs review
Issue tags:-DX (Developer Experience)

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.

xjm’s picture

Augh crossposts.

David_Rothstein’s picture

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

xjm’s picture

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.

webchick’s picture

+10909340909304 to #150.

effulgentsia’s picture

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.

tstoeckler’s picture

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.

David_Rothstein’s picture

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.

effulgentsia’s picture

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.

Crell’s picture

That's definitely not what we created that class for, and I would be very much opposed to. \Drupal is to get at services from a non-injectable context. Not to get at random operations, but to get to services.

beejeebus’s picture

StatusFileSize
new109.02 KB
PASSED: [[SimpleTest]]: [MySQL] 54,724 pass(es).
[ View ]

just a reroll, so i can belatedly do some profiling.

beejeebus’s picture

StatusFileSize
new71.66 KB
new70.91 KB

ok, here are some xhprof results.

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

hitting the front page, anon, 10 nodes, with a recent content block showing the same 10 nodes.

Overall Diff Summary

Run #1 Run #2 Diff Diff%
Number of Function Calls 1,612,260 1,614,620 2,360 0.1%
Incl. Wall Time (microsec) 18,147,913 18,381,504 233,591 1.3%

attached zipped up xhprof dumps in case anyone wants to dig. 1.urls.xhprof.gz is HEAD, 2.urls.xhprof.gz is patched.

ParisLiakos’s picture

Issue tags:-PHPUnit Blocker

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

ParisLiakos’s picture

Status:Needs review» Needs work
Issue tags:-Needs profiling+PHPUnit Blocker

Ah forgot to add tag

effulgentsia’s picture

Issue tags:+PHPUnit Blocker
StatusFileSize
new1.75 KB

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.

effulgentsia’s picture

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?

effulgentsia’s picture

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

effulgentsia’s picture

Title:Unify Generator, url(), path aliasing, and url_outbound_alter» Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence
Issue tags:-Needs issue summary update

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

beejeebus’s picture

effulgentsia - ab is really not a good tool to use for this. you're measuring a whole bunch of things other than the time spent in php, which is what we care about.

i'll redo my xhprof runs and compare.

beejeebus’s picture

spent some time with xhprof today.

set up was:

- drush-d8 genc --types=article 30 20
- class loader = apc
- front page view set to page at 30 items
- anonymous, hitting the front page

instead of aggregates, i just looked at the variance between runs, and it turns out to be quite high. so, i guess disregard the numbers i posted in #159.

katbailey’s picture

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!

katbailey’s picture

Issue summary:View changes

Created a new issue summary based on the template

catch’s picture

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.

webchick’s picture

Priority:Critical» Major

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. :\

Crell’s picture

Converting url() calls to generator calls is not BC-breaking, IMO. If we're not removing url() itself, we can do such a conversion at our leisure even after 1 July, and even after 8.0.

The unification of generate() and url() and elimination of hook_url_outbound_alter() in favor of the processor design is justification enough for this issue, IMO, and makes it a release-blocker.

katbailey’s picture

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.

effulgentsia’s picture

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.

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new4.61 KB
new111.25 KB
FAILED: [[SimpleTest]]: [MySQL] 57,005 pass(es), 25 fail(s), and 1 exception(s).
[ View ]

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

katbailey’s picture

StatusFileSize
new585 bytes
new111.26 KB
FAILED: [[SimpleTest]]: [MySQL] 55,816 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 1888424.url_generator.175.patch, failed testing.

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new4.07 KB
new112.98 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1888424.url_generator.177.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fixing the things...

Status:Needs review» Needs work

The last submitted patch, 1888424.url_generator.177.patch, failed testing.

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new113.02 KB
PASSED: [[SimpleTest]]: [MySQL] 55,755 pass(es).
[ View ]

Rerolled after language constants moved to class constants...

katbailey’s picture

StatusFileSize
new20.59 KB
new113.54 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1888424.url_generator.180.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Crell’s picture

Status:Needs review» Reviewed & tested by the community

I reviewed the bulk of the changes in previous interdiffs, and the latest looks good to me, so...

alexpott’s picture

Crell’s picture

Bumping and tagging.

David_Rothstein’s picture

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

Crell’s picture

I suspect that was added due to one of Drupal's infamous bootstrap race conditions. Since drupal_goto() is slated for execution anyway, though, I'm not worried about it.

catch’s picture

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

effulgentsia’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+DX (Developer Experience), +Framework Initiative, +API change, +Avoid commit conflicts, +PHPUnit Blocker

The last submitted patch, 1888424.url_generator.180.patch, failed testing.

effulgentsia’s picture

Assigned:catch» Unassigned

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

katbailey’s picture

Status:Needs work» Needs review
StatusFileSize
new3.83 KB
new111.16 KB
PASSED: [[SimpleTest]]: [MySQL] 55,305 pass(es).
[ View ]

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

Also addressed catch's points.

msonnabaum’s picture

Status:Needs review» Reviewed & tested by the community

Looks great to me.

katbailey’s picture

Assigned:Unassigned» catch

Re-assigning back to catch...

alexpott’s picture

Title:Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence» Change notice: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence
Priority:Major» Critical
Status:Reviewed & tested by the community» Active
Issue tags:-Avoid commit conflicts+Needs change record

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!

alexpott’s picture

Assigned:catch» Unassigned
vijaycs85’s picture

+++ 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).

YesCT’s picture

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.

pwolanin’s picture

making a start on the change notice at https://drupal.org/node/2046643

pwolanin’s picture

Title:Change notice: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence» Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence
Priority:Critical» Major
Status:Active» Fixed
Issue tags:-Needs change record, -PHPUnit Blocker+WSCCI

change notice is done (enough).

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

Anonymous’s picture

Issue summary:View changes

Add additional bullet point to issue summary.

xjm’s picture

Gaelan’s picture

Gábor Hojtsy’s picture

@Gaelan also wrote a change notice for the hook change at https://drupal.org/node/2238759 :) Woot! Attached to this issue too.

olli’s picture

+++ b/core/modules/language/language.negotiation.inc
@@ -425,83 +425,6 @@ function language_switcher_session($type, $path) {
-          $options['prefix'] = $prefixes[$options['language']->langcode] . '/';

+++ b/core/modules/language/lib/Drupal/language/HttpKernel/PathProcessorLanguage.php
@@ -69,4 +90,74 @@ public function processInbound($path, Request $request) {
+        return empty($path) ? $prefixes[$options['language']->langcode] : $prefixes[$options['language']->langcode] . '/' . $path;

#2146035: drupalSettings.path.pathPrefix does not contain the language identifier needs review.