Url::fromUri() should accept protocol-relative URLs

The protocol is needed to differentiate between internal and external links.
If there is no protocol specified Drupal will check if it is a valid internal link.

The proposed solution, "//example.com", would use // just for omitting http: or https:. This is not a common URL elsewhere and would be considered a "drupalism".

Changed "//example.com" to "example.com" at the issue summary. Then, it will not sound as a drupalism.
Leaving it open for discussion, as there still the technical problem differentiating between internal and external links.

Just for some background -- protocol-relative (or protocol-agnostic) URLs were popularized by Paul Irish back in 2010. They're particularly useful for frontend assets from third-parties. For instance, if you're loading a web font from Typekit, and you use http:// on a site that exists on https://, you'll get a mixed-content security warning. By using the protocol-relative URL, the browser will load whichever is appropriate to how the site is being served. Though it's worth pointing out that Paul Irish now considers this an anti-pattern, because https should now always be used.

I'm not sure there's a concrete use case for this in the link module, but that's how these URLs get used in the context of theming.

The HTML5 url field allows protocol-less URI, if a user enters //google.com the link isn't even rendered, but it is shown in edit mode correctly

This sounds like a major bug, because it will confuse people

Here is a patch for it.

Switching version so it gets tested

I think the correct term is protocol-relative.

per #6needs justification in the issue summary as to why you shouldn't use https:// if the content you are linking to can be served that way. We could just throw a more specific exception?

So here's what Paul Irish writes in his update, calling protocol-relative URLs an anti-pattern, that I referenced in #6:

Now that SSL is encouraged for everyone and doesn’t have performance concerns, this technique is now an anti-pattern. If the asset you need is available on SSL, then always use the https:// asset.

Allowing the snippet to request over HTTP opens the door for attacks like the recent Github Man-on-the-side attack. It’s always safe to request HTTPS assets even if your site is on HTTP, however the reverse is not true.

More guidance and details in Eric Mills’ guide to CDNs & HTTPS and digitalgov.gov’s writeup on secure analytics hosting.

I think it's significant that he says asset. That's the context in which this technique was originally developed (as I mentioned above) and where it usually shows up: Calling things like CSS, JS and font files from third-party services. And given that most good third-party services now run on HTTPS, it makes sense to only request those assets on HTTPS, even if you're on HTTP.

But link module fills a different need, which is linking to all kinds of things, as outbound links, not necessarily for front-end/asset-requesting reasons. Given that, I don't think protocol-relative URLs are necessarily an anti-pattern, because you might be linking out to sites whose protocol you don't know.

(Admittedly I can't think of an example scenario like that, but I'm not sure what the scenario was for the OP either.)

So my reading is that given the use case of link module, protocol-relative links might be legitimate, and not the anti-pattern that Paul Irish describes for asset requests. But perhaps we want a concrete example to confirm that.

In general I kinda think we should implement what is possible, and well protocol relative URLs are a thing on the internet.

@dawehner But only for included assets, not links.

It isn't asset specific, it can be used for links as well.

In what case does it make sense to have a protocol-relative link?

For assets like external JS or images, the idea was to load the JS or image over https if the current page was https to avoid mixed content warnings.

When would it be important or even appropriate to toggle an external link between http and https based on the protocol the current page is using?

@pwolanin I agree, this issue makes no sense to me.

Well, I believe strongly that the URL object, as being an abstract concept, should support what browsers support. Whether this is also needed for the link field, I'm not sure.

I like the patch at #8 because without the patch Drupal does not display any error message yet, the published node does not display the link if typing //example.com. After patched the link is displayed as //example.com.

I am marking it as "needs work" because the error message displayed if typing example.com is this: "Manually entered paths should start with /, ? or #. "

This error message is regarding internal links only. We should change it to something like: "Manually entered internal paths should start with /, ? or #. External paths should start with //, or a protocol such as https://"

@dawehner I agree with your last comment at #19. When I said it looks like a drupalism I mean I never seen a browser which I had to type // at the address. IMHO, it sounds much better for user experience if we could find a way to just accept example.com as an external link without the double slash. Do you guys it worth more discussion about keeping/removing the double slash? - I'm adding it to the issue summary.

I changed the error message to display as said in comment #20
I also added code to trim the external URLs starting with // because it may be weird to end users and site visitors to see a link in content such as //example.com with no leading protocol.

Fixed the failing test at #21

IMHO the field widget itself have some description which points out that you can use https://...
The other case: using "//" is IMHO fine to not be described somehow to the user.

@dawehner as an end user I would expect example.com to be a valid input. The current field widgets description nor its error message do not explain clearly why this is not a valid input. Then, this different error message is relevant in this case.

I would prefer to change the whole field validation to accept example.com, but this is a task for another issue and I am not sure this would be possible to be done with the validations we have for internal links. Anyway, something to be discussed in a different issue.

BTW, I agree on not adding the // use case in the field widget description, as it could be more confusing than helpful.

Here is a patch for 7.x-1.x

well, i could apply the last patch from #23 using 8.1.x branch, so no need to reroll it

it seems the failed test belongs to a deleted comment

Marked #2710785: \Drupal\Core\Url::fromUri('//example.com') - allow wildcard HTTP scheme as a duplicate.

Patch against 8.1.x
Simple rebase fixed it.

Confirmed that the patch applies and does what it says.

Screenshots of the "front-end" are the same, as it should be.

Edit page, with or without patch:

Node, with or without patch:

What changes is the linked URL. Without the patch, the linked URL is simply empty:

With the patch, the URL is correctly linked:

+++ b/core/lib/Drupal/Core/Url.php
@@ -272,7 +272,15 @@ public static function fromUri($uri, $options = []) {
+    // We support protocol relative URLs.

+++ b/core/modules/link/tests/src/Unit/Plugin/Validation/Constraint/LinkExternalProtocolsConstraintValidatorTest.php
@@ -50,6 +50,8 @@ public function providerValidate() {
+    // Protocol relative URLs

+++ b/core/tests/Drupal/Tests/Core/UnroutedUrlTest.php
@@ -82,6 +82,8 @@ public function providerFromUri() {
+      // A protocol relative URL.

Nit: s/protocol relative/protocol-relative/

Can be fixed on commit.

Hi all,
I try to fix the nits before apply. Hope that i understood you correctly.

Its nice to see this done properly. In earlier comments both @pwolanin and @cilefen disagreed with the feature itself. I'm wondering whether their concerns got addressed.

For me, this feature simply belongs into the system, given that we support all kind of URLs already. Additionally protocol relative URLs are super useful for assets.

I don't think @pwolanin and @cilefen concern's have been addressed at all. And given we ask users to prefix with a leading slash all over the place. I haven't seen a convincing argument that the UI should support protocol relative link fields. I think there is an argument that the URL API should.

@alexpott
On thing we could do is to support it but don't hint the capability to users.

I agree the URL API should support this, but if it is going to confuse people I don't think we necessarily need to explain it, as @dawehner suggests.

Since protocol-relative URLs are valid, I think Drupal should support them.

However, it is mostly something used in assets (see background links in #6 and #12), which are usually not set through the UI but through theme or module files, so I've been trying to think of a use case to address @pwolanin and @cilefen's concerns.

So here's one: Imagine a contrib module that takes a value from a link field on a node and inserts it on the page. It could be third-party JS (say an analytics add-on script for a landing page), an external image (say set as a the background on the entity), or CSS hosted on a CDN (say inlining critical styles).

Admittedly that seems like a clumsy way of doing things -- relying on a core link field instead of the module creating its own -- but it's possible.

Still, the strongest argument to me is that they are valid URLs, and Drupal should accept valid URLs as valid. Use case or no use case, we shouldn't have some weird Drupalism exception to a valid URL.

#44++, and especially:

Still, the strongest argument to me is that they are valid URLs, and Drupal should accept valid URLs as valid. Use case or no use case, we shouldn't have some weird Drupalism exception to a valid URL.

If this gets knocked down again from RTBC, then I propose we do #2710785: \Drupal\Core\Url::fromUri('//example.com') - allow wildcard HTTP scheme, which is just the URL API portion of this. Then that portion can at least land.

IMHO ideally we should still keep the UI text as it is ... I could imagine that people could be confused by that sentence.

Discussed with @catch. We think the API changes should land and we’ll hold off on the link field until there’s a use-case beyond “it’s a valid URL” since the use-case of attaching assets via a link field is something we should discourage.

I think just making the API changes in this issue is okay since it'll make crediting the contributors easier.

For #50

Okay, I'll reroll this patch, rescope this issue, and file a new issue for the Link field aspects.

As of #50, the scope of this issue is solely about making Url::fromUri() work with protocol-relative URLs. That means the scope of this issue is now effectively identical to #2710785: \Drupal\Core\Url::fromUri('//example.com') - allow wildcard HTTP scheme, but for practical reasons (see #50), we chose to do it in this issue instead.

I opened the follow-up #2744729: Link field should accept protocol-relative URLs to do what this issue originally intended to do.

And finally, attached is a rerolled patch that only keeps the hunks that are relevant for Url::fromUri().

And to be perfectly clear: this does belong in Drupal 8.1, not in Drupal 8.2, because it's an oversight, a bug, that prevents certain contrib modules from working.

I personally maintain the CDN module, which transforms file URLs into protocol-relative URLs pointing to a CDN. Without this API support, it simply cannot function, because PHP errors will be thrown in many places, including when uploading files (when a file is uploaded, a preview to that file is rendered). See #2711529: File upload widget broken when using CDN module, fixed in Drupal 8.1.4: require that version.

I would have went the other way round and added a new issue for the URL change, given how much discussion already existed around link field, but meh.

I agree, but

I think just making the API changes in this issue is okay since it'll make crediting the contributors easier.

In the end, it won't matter all that much :)

+++ b/core/lib/Drupal/Core/Url.php
@@ -272,7 +272,15 @@ public static function fromUri($uri, $options = []) {
+    // We support protocol-relative URLs.
+    if (strpos($uri, 'internal://') === 0) {
+      $uri = substr($uri, strlen('internal:'));
+      $uri_parts['scheme'] = '';
+    }
+    elseif (strpos($uri, '//') === 0) {
+      $uri_parts['scheme'] = '';
+    }

nit: the documentation should be above the ifelse that handles protocol relative URLs, not the section that handles internal protocols.

As Wim mentioned in IRC, that can be fixed on commit. The rest looks very well defined and solid.

+++ b/core/lib/Drupal/Core/Url.php
@@ -272,7 +272,15 @@ public static function fromUri($uri, $options = []) {
+    if (strpos($uri, 'internal://') === 0) {
+      $uri = substr($uri, strlen('internal:'));
+      $uri_parts['scheme'] = '';
+    }

This change seems out of scope... since later on in this method we do:

    elseif ($uri_parts['scheme'] === 'internal') {
      $url = static::fromInternalUri($uri_parts, $uri_options);
    }

Which this will have stopped from happening afaics.

@alexpott @catch let's think the opposite way. Is there a reason for NOT supporting a valid URL?
I do not agree with the arguments on #50.

Just as @Wim Leers said on #45 I also think the argument on #44 is very strong:

Still, the strongest argument to me is that they are valid URLs, and Drupal should accept valid URLs as valid. Use case or no use case, we shouldn't have some weird Drupalism exception to a valid URL.

#57: Fixed.

#58: AFAICT you're right. It seems to have been a work-around for a UI problem, and likely an incorrect work-around. Thanks for your strictness. Apologies for my lack of strictness.

#59: the thing is that there is an usability impact. So, let's first land the API support here.

@wim-leers, superb. You've developed a patch that I've already did: https://www.drupal.org/node/2710785#comment-11110037

#61: no need for the sarcasm. We have one common goal here. See #48 — you're credited. So don't worry ;) I don't care about the issue credit. I only care about getting this fixed.

In fact, I see you've got an extra test there that is very valuable! :) So, bringing that into this patch. And I see that one part was not yet tested: whether protocol-relative URLs are correctly considered as external by the Url class.

Great minds think alike I guess. :) Too bad it didn't get more attention earlier but we got there eventually.

It might be nice to rewrite the main testcase test to confirm all the outputs like the new test does but that is definitely overkill here. Especially because it might be that is actually impossible. If someone wanted to do that in a follow up I'd definitely review it though. :)

Thanks again guys!

Committed a8d3c98 and pushed to 8.1.x and 8.2.x. Thanks!

Thanks! This is one important blocker less for the CDN module to get a stable Drupal 8 release!

See #2711529-10: File upload widget broken when using CDN module, fixed in Drupal 8.1.4: require that version.

(Sadly, #2313917-67: Core version key in module's .info.yml doesn't respect core semantic versioning and #2641658-22: Module version dependency in .info.yml is ineffective for patch releases mean that modules can't actually declare a patch version of Drupal 8 as the minimum version…)

Been through a couple of these issues now and wondering if someone can point me to the one that covers the basic issue which i think started these:

we should be able to enter example.com as a valid url (as we could in D7).

I tried this patch and the one here #2744729: Link field should accept protocol-relative URLs and unclear that either of these does anything.