Problem/Motivation

drush ev "\Drupal\Core\Url::fromUri('base:2015/10/06');"

Gives:

exception 'InvalidArgumentException' with message 'The URI 'base:2015/10/06' is invalid. You must use a valid URI scheme.'

The same for \Drupal\Core\fromUri('internal:/2015/10/06') since that eventually falls back to base: if it can't find something.

The problem is apparently the parse_url() at the top of fromUri().

That parses that as host => base, port => 2015, path => 10/06. Which is a perfectly valid interpretation of that string but not at all what we expect to happen.

UrlHelper::parse() on the other hand seems to parse it correctly.

I originally found this due to the deslash (remove trailing slash) feature in redirect.module (previously a feature in globalredirect, we merged those together). It uses internal: on the incoming URL to convert that into a Url object as we're using an Url object in an internal helper method to deal with options and access checking if necessary.

So with redirect.module enabled with the deslash feature enabled (not the case by default), going to http://yoursite/2015/10/06/ currently throws that exception, which we noticed because we went live with a site that apparently used URL's like that before.

Proposed resolution

Use UrlHelper::parse() instead in fromUri() ?

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#14 url-port-path-2580717-14.patch1.41 KBtduong
#14 interdiff-2580717-10-14.txt739 bytestduong
#10 url-port-path-2580717-10.patch1.39 KBtduong
#10 url-port-path-2580717-10-test_only.patch672 bytestduong
#10 interdiff-2580717-8-10.txt2.03 KBtduong
#8 url-port-path-2580717-8.patch2.15 KBtduong
#8 interdiff-2580717-4-8.txt1.32 KBtduong
#4 url-port-path-2580717-4.patch751 bytesBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

Use UrlHelper::parse() instead in fromUri() ?

Does the URL is actually able to handle that? I mean we still want theoretically to support hosts, don't we?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

Use UrlHelper::parse() instead in fromUri() ?

Does the URL is actually able to handle that? I mean we still want theoretically to support ports, don't we?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Active » Needs review
FileSize
url-port-path-2580717-4.patch751 bytes

Yeah, sorry, UrlHelper::parse() is nonsense, that doesn't work at all.

Very ugly, but this works. I have no idea what else we could do?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Status: Needs review » Needs work
Issue tags: +Needs tests

How did you not started writing tests, seriously

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Sorry :)

Just wanted to see if this works and get some feedback on it. Didn't have time for more, but it came up again in a project.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

Well, the workaround in general seems to be working, and well if we document this behaviour it might be okay.

tduong’s picture

tduong CreditAttribution: tduong at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
interdiff-2580717-4-8.txt1.32 KB
url-port-path-2580717-8.patch2.15 KB

Provided test for Url::fromUri('internal:2015/10/06');, Url::fromUri('internal:/2015/10/06');, ...

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented 
+++ b/core/tests/Drupal/Tests/Core/UrlTest.php
@@ -749,6 +749,35 @@ public function providerFromInvalidInternalUri() {
+   *
+   * @covers ::fromUri
+   * @expectedException \InvalidArgumentException
+   * @dataProvider providerFromInvalidInternalDateStringUri
+   */
+  public function testFromInvalidInternalDateStringUri($path) {
+    Url::fromUri('internal:' . $path);
+  }

We are fixing that we get an exception, so we need to test that there is *no* exception :)

If it doesn't work, then we need to figure out why, might be a unit test environment problem.

Try specifying base: directly, not internal:

tduong’s picture

tduong CreditAttribution: tduong at MD Systems GmbH commented
FileSize
interdiff-2580717-8-10.txt2.03 KB
url-port-path-2580717-10-test_only.patch672 bytes
url-port-path-2580717-10.patch1.39 KB

Done. :)

tduong’s picture

tduong CreditAttribution: tduong at MD Systems GmbH commented

About the documentation @dawehner mentioned in #7, should the @Berdir's inline explanation be in the fromUri() declaration docblock?

The last submitted patch, 10: url-port-path-2580717-10-test_only.patch, failed testing.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented 
  1. +++ b/core/lib/Drupal/Core/Url.php
@@ -293,6 +293,11 @@ public static function fromUserInput($user_input, $options = []) {
+    // parse_url() incorrectly parses base:number/... as hostname/port and not
+    // the scheme. Prevent that by adding a / for those.

    s/'hostname/port'/'hostname:port'

    Maybe "Prevent that by prefixing the path with a slash."? 

  2. +++ b/core/lib/Drupal/Core/Url.php
@@ -293,6 +293,11 @@ public static function fromUserInput($user_input, $options = []) {
+    if (preg_match('/base:\d/', $uri)) {

    Lets catch here only when starts with "base:". So, '/^base:\d/' I think is better.

tduong’s picture

tduong CreditAttribution: tduong at MD Systems GmbH commented
FileSize
interdiff-2580717-10-14.txt739 bytes
url-port-path-2580717-14.patch1.41 KB

Adjusted it. :)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented 
+++ b/core/lib/Drupal/Core/Url.php
@@ -293,6 +293,11 @@ public static function fromUserInput($user_input, $options = []) {
+    // parse_url() incorrectly parses base:number/... as hostname:port/...
+    // and not the scheme. Prevent that by prefixing the path with a slash.
+    if (preg_match('/^base:\d/', $uri)) {
+      $uri = str_replace('base:', 'base:/', $uri);
+    }

Looking at https://tools.ietf.org/html/rfc3986#page-16
I'm wondering whether we could solve that more generically. If there is '//' in the URL, we expect a authority to be in there, otherwise there is none, and it doesn't make sense to use parse_url()?

tduong’s picture

tduong CreditAttribution: tduong at MD Systems GmbH commented

We need a scheme component that UrlHelper::parse() does not return (it returns only the URL's path, query, and fragment components), so it makes no sense to call it here.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Just for curiosity I tried out the guzzle URI class:

$uri = new \GuzzleHttp\Psr7\Uri('base:2015');

Class Properties:
  $charSubDelims    "!\$&'\(\)\*\+,;="
  $charUnreserved   "a-zA-Z0-9_\-\.~"
  $fragment         ""
  $host             "base"
  $path             ""
  $port             2015
  $query            ""
  $replaceQuery     [ …2]
  $scheme           ""
  $schemes          [ …2]
  $userInfo         ""

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: url-port-path-2580717-14.patch, failed testing.

The last submitted patch, 14: url-port-path-2580717-14.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests

Still RTBC, was just a CI error.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed 018dd52 and pushed to 8.0.x. Thanks!

I pondered the performance implications with @dawehner but then dismissed them as rendering a URL will be far more expensive.

  • alexpott committed 97d5da9 on 
    Issue #2580717 by tduong, Berdir, dawehner: Url::fromUri('base:2015/10/...

  • alexpott committed 018dd52 on 
    Issue #2580717 by tduong, Berdir, dawehner: Url::fromUri('base:2015/10/...

Status: Fixed » Closed (fixed)

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