UrlHelper does not support tel:

I'm wondering wether \Drupal\Component\Utility\UrlHelper::parse already does what we need here? Maybe we need a UriHelper though.

I think an external compliant parser could probably be easiest.
We create an UriHelper based on the external parser, switch the needed methods were needed with the new parser. And as a followup we could do the same for UrlHelper, and mark that even deprecated in the end.

Looking at external parsers, think https://github.com/thephpleague/uri is most promising, and it's successor for php 5.6+ (in the future https://github.com/thephpleague/uri-parser)
Other option is https://github.com/Riimu/Kit-UrlParser

Give me some days to work on a starter patch.

Thanks, for the heads up. I missed the intl requirement. Then i think the only option is a custom UriHelper. I'll start with that.

Reformatted IS (mostly to remove wrapping under sidebar), and resuggested possible solution.

This adds an URIHelper class. And fixes the primitive type validation. The ExternalLink still fails (i've incorparated the tests from DualFr).
The regex-parsing comes from https://gist.github.com/kpobococ/92f120c6c4a9a52b84e3 Not sure if this is allowed to copy paste this / how to give attribution for this in code.
This will fail, but settings for needs review to test if the first tests at least pass.

This needs first discussing of this is the right approach. Needs tests and fixes for the other not OK uses mentioned in IS.

Added more (failing) tests for the other cases. (So we have a base to fix ;-) ).
Not sure about two 'wrong' parse_url usages.

addShortcutLinkInline is only used to add a page inline to the default shortcut set (through the star on a page). So abusing this from an external url (tel uri) doesn't seem possible.

system_retrieve_file is used to download content, so it's up to the developer IMO not to use it to download a URI, but only a URL. (In core only used for translations and updates).

Setting to needs review to demonstrate more failing tests.

Not commenting on the patch, just on the problem space that this issue is surfacing...

From the issue summary:

The problem is that PHP's parse_url is 'designed' for url's not for URIs.

And:

\Drupal\link\Plugin\Field\FieldWidget\LinkWidget getUserEnteredStringAsUri Not OK (it transforms tel:xxx to internal:tel:xxx)

We're in an interesting situation here. The job of LinkWidget (and most other "not ok" places identified in the issue summary) is to populate the value of an href attribute. According to https://www.w3.org/TR/html5/links.html#links-created-by-a-and-area-elements, this is defined as holding URLs, not URIs. https://www.w3.org/TR/html5/references.html#refsURL then proceeds to link to both https://tools.ietf.org/html/rfc3986 and to https://url.spec.whatwg.org/. From what I can tell, tel URIs are not URLs, per the distinction of RFC3986. Which is partly why PHP's parse_url doesn't parse them correctly. However, they are allowed into href values, which is a potential contradiction of the HTML5 spec defining href values as being for URLs. Unless the resolution to that contradiction is https://url.spec.whatwg.org/#goals, which seeks to remove the distinction between URL and URI. But that document isn't an officially adopted standard yet, which makes it odd to me that HTML5 would redefine URL to mean something based on that broader, but not yet standardized, definition.

So, we may be in a situation where HTML5 defines URL differently than PHP does. Which sucks if it means we can't use PHP's URL functions when dealing with HTML5 URLs.

#13
++ for and external vendor library. This seems to be a typical library functionality that won't exists soon php-core but will be be shared/needed in a lot of php-projects.

The framework and release managers discussed this issue awhile back and determined that the scope in the summary is primarily about tel:. Since this is a sanitization API that is not behaving as developers expect, we agreed the issue is a major bug.

Found this issue while searching for

InvalidArgumentException: "The internal path component 'tel:%2B4912345-0' is external. You are not allowed to specify an external URL together with internal:/." at /core/lib/Drupal/Core/Url.php line 419'

which is triggered via the TelephoneLinkFormatter in Drupal 8.9.

This can be triggered easily with a link field. We found this via a client trying to create a link to tel:000.

Rerolled the patch from #11 to work with 9.4.x

Fixed failed commands on #38
Attached interdiff

Will need a test case.

Added test case for #39

$ cd core
$ ../vendor/bin/phpunit --filter testFromUriTelephoneNumber tests/Drupal/Tests/Core/UrlTest.php
PHPUnit 8.5.26 #StandWithUkraine

Testing Drupal\Tests\Core\UrlTest
.                                                                   1 / 1 (100%)

Time: 132 ms, Memory: 8.00 MB

OK (1 test, 2 assertions)

Added merge request, test looks good to me.

Here is a complete patch with tests.

Wow quick turn around!

Putting into NW because the test patch seemed to pass

You want the tests only patch to fail and the full patch to pass to signify the problem is solved

Added test only patch as per comment #46.

#47 is the same as #42 so please disregard

But for test only patches it helps to append test-only.patch

So was the problem was that is wasn't marking tel:1234 as external? Appears to be doing that now without the patch?

Sure I'm missing something though.

As mentioned in #49 the I tested with URL tel:123-4567-8901 and without using the patch the URL is external. I think the tests needs to be extensive to cover the 'Not Ok ' scenarios mentioned in the description

This should be red/green.

Took a look at the code and all seems clean to me. Glad to see the test case got worked out!

Can we add a couple of more test cases? See this file from laravel: https://github.com/laravel-validation-rules/phone/blob/master/tests/Vali...

I have added the E164 cases.

The interdiff for #54 - #51

Patch did not apply, hereby another try

spellcheck :/
another retry

Tested on Drupal 9.5.x and php 8.1

- The issue summary is clear
- The patch applies cleanly and tests pass
- Used small snippet of code to test the issue and it works as expected.

Marking this as RTBC

1. +++ b/core/lib/Drupal/Core/Url.php
   @@ -281,7 +281,14 @@ public static function fromUri($uri, $options = []) {
   +    if (preg_match('/^tel:/', $uri)) {
   

   This could just be str_starts_with() in Drupal 10.

2. +++ b/core/lib/Drupal/Core/Url.php
   @@ -281,7 +281,14 @@ public static function fromUri($uri, $options = []) {
   +    if (preg_match('/^tel:/', $uri)) {
   +      $parts = explode(':', $uri);
   +      $uri_parts['scheme'] = $parts[0];
   

   Should this be explode(':', $uri, 2), or alternatively, should it fail if there's a second colon?

Also neither the title nor issue summary match the proposed solution - why are we only changing Url::fromUri() here?

After further review it appears the patch in #11 was meant to be a temporary solution while a UriHelper class was written.

#17 and #19 attempted to solve the problem to match the issue summary.

Seems around #38 the temporary patch was rerolled and carried forward as the solution (including by myself)

So are we abandoning the UriHelper class all together?