Add $options['base_url'] to UrlGenerator::generateFromRoute()

core/modules/language/src/Tests/LanguageUrlRewritingTest.php has sort of unit tests (it needs to build a request environment itself because the test system may not receive whatever domains, so it needs to simulate an environment) for domain + port based link generation using url().

core/modules/language/src/HttpKernel/PathProcessorLanguage.php is where the inbound (from request) and outbound (to output) URL handling is for languages; you should look at the outbound processing and why it does not work

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -183,7 +183,7 @@ public function generateFromRoute($name, $parameters = array(), $options = array
-    $base_url = $this->context->getBaseUrl();
+    $base_url = isset($options['base_url']) ? $options['base_url'] : $this->context->getBaseUrl();

It would be great to expand the test coverage in Drupal\Tests\Core\Routing\UrlGeneratorTest

There is some code in the generateFromRoute() method that doesn't really make sense to me:

return $scheme . '://' . $host . $port . $base_url . $path . $fragment; 

Doesn't base_url include the scheme, host and port?

@mfb
Nope, $base_url is $this->context->getBaseUrl() which is something else than global $base_url;

+1 for that so far.

Right. But this patch has $base_url = isset($options['base_url']) ? $options['base_url'] : $this->context->getBaseUrl();

$options['base_url'] includes the scheme, host, port.

$this->context->getBaseUrl() is the base path.

If $base_url is set to a full URL as found in $options['base_url'] then combining that with scheme, host, port down at the bottom of the function doesn't work.

Contrib modules (and custom modules) do need a way to alter all aspects of the URL, including scheme and domain (e.g. Domain Access module modifying the domain, or Secure Login module forcibly rewriting a URL to HTTPS with mixed-mode sessions disabled) so I think it's correct to allow the base_url to be altered. but the code in this method doesn't allow it - it decides what the scheme and host will be and slaps the base_url on the end of that.

Some sample problems with this patch applied, and two languages enabled, with domain URL language detection:

Clicking on Log out redirects me to http://d8.localhttp//d8.local/

Clicking the Uninstall button at the module uninstall page redirects me to http://d8.localhttp//d8.local/admin/modules/uninstall/confirm

Running tests redirects me to http://d8.localhttp//d8.local/admin/config/development/testing/results/1

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -183,8 +185,8 @@ public function generateFromRoute($name, $parameters = array(), $options = array
+    $base_url = isset($options['base_url']) ? $options['base_url'] : $this->context->getBaseUrl();
+    if (!$absolute || preg_match('/^http(s)?\:\\/\//', $base_url) === 1 || !$host = $this->context->getHost()) {

Can you explain why we need this here? At least a comment about the regular expression should be necessary. It sounds though not like a good idea, to be honest.

Thanks, this resolves the issue I noticed.

One question: Is it necessary to say that base URL is "Only used internally, to modify the base URL when a language dependent URL requires so." I realize this is how it was documented in Drupal 7, but I believe modifying base URL here may be the only way for contrib or custom modules to modify the base URL (to use a different hostname, domain or scheme)?

maybe to be more clear something like

Only used internally by a path processor, e.g. to modify the base URL when a language dependent URL requires so.

?

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -183,7 +183,26 @@ public function generateFromRoute($name, $parameters = array(), $options = array
+          $base_url = str_replace('http://', 'https://', $base_url);
+        }
+        elseif ($options['https'] === FALSE) {
+          $base_url = str_replace('https://', 'http://', $base_url);

Should the regexp specify to only str_replace if it matches http or https at the beginning of the string? I can't think of a scenario where http:// or https:// would ever occur anywhere but the beginning of a url, but it might be safer to specify ^http instead of http.

This looks good to me, setting to RTBC

From the issue summary this is a bug. Do we need to add some assertions to LanguageUILanguageNegotiationTest::testLanguageDomain() or create a new test along the lines of the issue summary:

1. Login
2. Enable Languages
3. Install Italian from admin/config/regional/language
4. Set to domain based negotiation, and configure URLs from admin/config/regional/language/detection/url
5. Place Language Switcher block in a region from admin/structure/block; enable both languages.
6. Go back to the main site.
7. Hover over English link in the Language Switcher, see it is correct.
8. Hover over Italian link, see it is the same as the English one. Snap!

Given that this blocks #2364157: Replace most existing _url calls with Url objects at the moment, this issue should be considered to be a critical one.

Apparently mixedModeSessions has been removed (as a fan of HTTPS everywhere, yay!) so setting to Needs work.

Just a nitpics,

1. +++ b/core/modules/language/src/Tests/LanguageSwitchingTest.php
   @@ -159,6 +162,72 @@ protected function doTestLanguageBlockAnonymous($block_label) {
   +    $this->assertText('The domain may not be left blank for English', 'The form does not allow blank domains.');
   +    $this->rebuildContainer();
   

   why rebuild needed here? no data changed

2. +++ b/core/modules/language/src/Tests/LanguageSwitchingTest.php
   @@ -159,6 +162,72 @@ protected function doTestLanguageBlockAnonymous($block_label) {
   +    /** @var UrlGenerator $generator */
   

   should be a namespaced

3. +++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
   @@ -172,7 +172,26 @@ public function generateFromRoute($name, $parameters = array(), $options = array
   +    else {
   ...
   +    }
   

   remove else

and assertText() should use t()

Can you also post a patch which only contains the test (XX-test-only.patch)? The intention of such a patch is to demonstrate the bug and the effectiveness of the test (by means of failing test assertions). See #2378703-4: Port denial of service fixes from SA-CORE-2014-006 to Drupal 8 for an example.

+++ b/core/lib/Drupal/Core/Routing/UrlGeneratorInterface.php
@@ -59,8 +59,8 @@
-   *   - 'base_url': Only used internally, to modify the base URL when a language
-   *     dependent URL requires so.
+   *   - 'base_url': Only used internally by a path processor, for example, 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.
@@ -132,6 +132,8 @@ public function getPathFromRoute($name, $parameters = array());

@@ -132,6 +132,8 @@ public function getPathFromRoute($name, $parameters = array());
    *   - '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.
+   *   - 'base_url': Only used internally, to modify the base URL when a language
+   *     dependent URL requires so.

Maybe use the same documentation for both methods, it looks like the second one was copied before the first one was modified.

The rest of the patch looks RTBC to me.

Applied #35 on a local instance and manually verified that the language selection block renders the links properly. As per #36 this is RTBC, provided that #37 turns green.

Committed/pushed to 8.0.x, thanks!