Telephone Link field formatter InvalidArgumentException with 5 digits or fewer in the number

Good catch

I'll take a stab at this, new to the process so any advice is welcome.

Here's a patch so the minimum length for a telephone number is set to 6 (I still experienced the same behavior at min length 5)

Should we really limit the number of digits in a phone number? Short numbers do exist, just think about emergency numbers, or internal numbers in a company (unless there is a special way of dealing with them in the URL?)

The bug, if there is one, occurs in the parse_url PHP function, which processes numbers with 5 digits or less like TCP ports, for instance :

php > var_export(parse_url("tel:65535"));
array (
  'host' => 'tel',
  'port' => 65535,
)
 
php > var_export(parse_url("tel:65536"));
false // Tries to match with a port, but outside range.
 
php > var_export(parse_url("tel:6000"));
array (
  'host' => 'tel',
  'port' => 6000, // Shouldn't be a port !?
)
 
php > var_export(parse_url("tel:600000"));
array (
  'scheme' => 'tel',
  'path' => '600000', // Expected result.
)

This RFC may provide relevant information.

Wait, why is parse_url() being used to validate a phone number? That seems like the larger issue to address.

Actually no, parse_url() is not used to validate phone numbers. This is what happens:

• TelephoneLinkFormatter::viewElements() builds a render array which will be used to display a link like <a href="tel:123456">123456</a>.
• Apparently Link::preRenderLink() expects #url to be a Url object, which is why TelephoneLinkFormatter::viewElements() calls Url::fromUri, which is a generic method which uses parse_url() before returning a Url object.

Is there another way to format a link other than using a render array with '#type' => 'link' and a #url property pointing to a Url object?

Since we cannot use Url::fromUri, we could build the URL object this way :

$url = new Url('tel:' . rawurlencode(preg_replace('/\s+/', '', $item->value)));
// Not sure this option is actually needed.
$url->setOption('external', TRUE);
// We do need to use such a method, otherwise Drupal will try to find a route for this URI and crash.
$url->setUnrouted();

Unfortunately, this will not work, because setUnrouted is a protected method. Reading #2346787: Update Url to reflect that it handles both routes and non-routed URIs and #2417459: Provide internal API for special schemes and thin public wrappers for user input and non-routed URLs, I am not sure making it public is an option.

In summary, I think we need a way to build a Url object for exotic protocols like tel: before solving this issue.

It seems that the real solution is for PHP to solve https://bugs.php.net/bug.php?id=70588, that I just reported.

In the meantime, we can possibly fix Drupal to circumvent this bug.

From a talk with @jcnventura and @pwolanin I started reviewing all the calls to parse_url() in the Core just to be sure that any of them could produce a failure.

I extracted that comment to put it in a new issue: #2575577: UrlHelper does not support tel:
(Ignore the attached files)

Wow, this is a nasty one. The fact that the number component of the URL changes from 'path' to 'port' based on the length leads me to believe this is definitely a bug in PHP and one that might be fixed in a future version.

If it's agreed that parse_url is appropriate for use on telephone numbers what are the next steps for fixing this? Should we encapsulate the function, fix the behaviour and continue to allow tel: links to be processed like any others?

Hiding the files that have been moved to #2575577: UrlHelper does not support tel: so they are not disturbing readers.

Patch in #4 works. I thinks it should be committed as soon as possible because it fixes the issue that a wrong (too short) number breaks the site. Applying the patch we have an message instead, which makes much better user experience than the current situation (white screen with long error message about InvalidArgumentException).

Than the underlying validation problem could be managed in a follow up issue.

(And we could have an other follow up issue about that the phone field validation should happen without actually sending the form: just like as it works with a required textarea.)

Short telephone numbers are not wrong. The patch in #4 hides the issue but don't fix it. In France (and in some other countries) we have a lot of short-4-digits telephone numbers for hotlines or companies. Plus, a lot of countries in the world have optional area codes that are the biggest part of the number so the subscriber number, which is mandatory, part is 6 digits or less.

If you really think that this patch should be commited you'll need to pass the rc target triage:

We should also add a statement to the issue summary of why we need to make this change during RC, including what happens if we do not make the change and what any disruptions from it are. We can add a section <h3>Why this should be an RC target</h3> to the summary.

The patch can be altered to enable less characters. It can be even 1.

But I really think it (or an altered version) should be committed because it let the user know what the problem is so she can fix the number she provided. Without this if you provide a short number which is used in France in a Drupal 8 telephone field all you will get after sending the form is a cryptic error page. Will you know what to do if you are not a Drupal developer? ;)

(Thanks for setting the correct RC tag)

I don't know if you didn't have the time to read the entire issue or if it's just not clear enough but we cannot display a telephone number that is shorter than 7 digits for now because of a misuse of a PHP function in the core of D8. (see #2575577: UrlHelper does not support tel:)

I agree that this is a really important issue that could need a temporary fix. But, it has to be very clear that it have to be deeply fixed later.
We should at least have some TODO comment in the code.

@DuaelFr I read the whole thread and I understand that there is an underlying problem which has to be fixed. But as you mention, until it is done we are better to go with a temporary solution what makes the visitors of a Drupal 8 site (using a telephone field in a form) keep using the site.

Added a comment. I hope my english is good enough.
@thamas could you, please, read it and move the issue back to RTBC if it's understandable.

@DuaelFr I'm not a native English speaker too, but I think it is totally OK! :)
Thanks for the patch update!

@thamas, for reference on the above, please review https://www.drupal.org/core/d8-allowed-changes#rc. Only core committers should add the rc target tag. Thanks @DuaelFr for getting it tagged correctly for triage.

This change can be made safely in a patch release. At this point we will not be committing any runtime code changes except the most urgent fixes (such as security issues, etc.) following RC4, so untagging for that.

Also, this needs test coverage. The error message looks fine to me too, although I'm not entirely sure whether this workaround is best. Thanks!

@xjm Thanks for checking this issue. Sorry for using a tag I should not!

It is not clear for me when this change can be committed to core? (I understand the "needs test" part.)

@thamas, sorry, I guess I did not explain clearly enough in #28. This change can be made safely during a patch release (see https://www.drupal.org/core/d8-allowed-changes#patch). As soon as 8.0.0 is released in less than a week, we will start committing patches allowed for patch releases. Then it could be available in 8.0.1 as early as December 2, if it's ready then and we agree this workaround is the best thing to do until we sort out the underlying bug. (I'm still undecided about that.)

@xjm thanks for explaining it. I did not know what a patch release is but I understand now!

What could I do to help this patch to get in in the next 8.0.4 ? Because is working, and is a need.

I replicated this issue on:
Drupal 8.1.1
Database system MySQL, MariaDB, Percona Server, or equivalent
Database system version 5.6.21

I found the following in my watchdog log:
a:5:{s:5:"%type";s:24:"InvalidArgumentException";s:8:"@message";s:62:"The URI 'tel:123' is invalid. You must use a valid URI scheme.";s:9:"%function";s:26:"Drupal\Core\Url::fromUri()";s:5:"%file";s:86:"C:\Users\Jason\Documents\Work\vhosts\d8.awi.local\docroot\core\lib\Drupal\Core\Url.php";s:5:"%line";i:276;}

Unable to reproduce the error on 8.2.x. Could you please mention the error you are getting or you can share some screen-shots regarding it.

Why the code is so different from 26 to 40?

Would be a good idea to have Min value and Max value on the Field settings? instead hardcode this value, just let a default value for them.

I think we should mark this as a duplicate of #2575577: UrlHelper does not support tel:. That is the underlying issue, which should be fixed.

I re-rolled this against 8.3.x, I don't see why we wouldn't get this patch in and work on #2575577: UrlHelper does not support tel: as a follow-up given that seems to be a much bigger chunk of work.

EDIT: Fixed issue link.

Never imagined the other issue would be so difficult ;-)

(We ran into the same issue recently and had to reject <5 digit inputs as a workaround, which is clearly not ideal.)

The problem seems that parse_url is not actually for URLs but rather for (possibly partial, scheme-less) strings that can be best-guessed into HTTP URLs. Thus the function is designed to accept "localhost:80" as an abbreviation for "http://localhost:80/", rather than as a "localhost"-scheme URL. Changing that in the PHP specification would either require a BC-breaking change, or adding "tel:" as a special case. I suggested a strict mode flag, but in either case this would probably be a feature-level change rather than a bug fix.

Meanwhile, fromUri always requires a scheme prefix or '//' for protocol-relative URLs. So what we actually want is a strict URL parser that doesn't try to guess. Maybe this should be implemented directly using regular expressions instead of relying on the native function?

Edit:

I'd point out that non-validating component-separation for URLs is an almost trivial regular expression documented in Appendix B of https://www.ietf.org/rfc/rfc3986.txt. Adapting that expression in PHP with further parsing of sub-components (user, password, host, port, as parse_url does) would be something like the following code:

/^
  (?:
    (?<scheme>[^:\/?#]+):
  )?
  (?:
    \/\/(?<authority>
      (?:
        (?<userinfo>
          (?<user>[^:@\/?#]*)
          (?::(?<password>[^@\/?#]))?
        )
      @)?
      # host is either a bracketed ip literal, or free of :.
      (?<host>\[[^\]*]\]|[^\/?#:]*)
      (?::(?<port>\d*))?
    )
  )?
  (?<path>[^?#]*)
  (?:
    \?(?<query>[^#]*)
  )?
  (?:
    \#(?<fragment>.*)
  )?
$/x

Seems like one * is missed (for the password group).

The result expression should be:

^
  (?:
    (?<scheme>[^:\/?#]+):
  )?
  (?:
    \/\/(?<authority>
      (?:
        (?<userinfo>
          (?<user>[^:@\/?#]*)
          (?::(?<password>[^@\/?#]*))?
        )
      @)?
      # host is either a bracketed ip literal, or free of :.
      (?<host>\[[^\]*]\]|[^\/?#:]*)
      (?::(?<port>\d*))?
    )
  )?
  (?<path>[^?#]*)
  (?:
    \?(?<query>[^#]*)
  )?
  (?:
    \#(?<fragment>.*)
  )?
$/x

In fact telephone link field formatter breaks the site now. Maybe will be better handle URI like this in the Url::fromUri method as temporary solution as it is for URIs with base scheme.

The next patch for Url class handles URI of the :
type when path contains only number with 5 digits or less. The scheme can contain alphanumeric symbols, + and -.
The . (dot) is excluded to prevent collisions with relative URLs like example.com:8080. But according to the list of available schemes (https://www.iana.org/assignments/uri-schemes/uri-schemes.xml) there are only several schemes with a dot and I don't think that the case of their usage a link is highly probable.

So the patch should cover almost all cases of URI usage.

Patch is updated.
Tests for the edge cases were added.

Reviewed and tested:
- Recreated the issue using Drupal 8.4.2-dev
- Installed patch #54
- Confirm that the issue is fixed
- Tests added are also looking good

I've read through the entire dialog above and believes this is ready to go into Core. Note that it is a temporary fix awaiting the much bigger https://www.drupal.org/node/2575577

I fixed less vs fewer and renamed the issue more accurately.

The fix in #54 looks really good! It's a bit hackish but it's a great workaround. It looks far better than the original solution. Thanks!

(Updated the IS to use the official template and change the proposed solutions to fit what's done in the patch)

There's a PHP issue here: https://bugs.php.net/bug.php?id=70588 let's add a reference to that in the comments.

I'm not sure about this patch, the issue is that when parse_url() sees the short number, it thinks it's getting a port - should we not make this behaviour specific to tel:?

Same problem here in Drupal 8.5.1

#54 fix it

Thanks @kyvour and others

Same problem here in Drupal 8.5.3

#54 fix it

Came across this today, nice to see its several years old!

So, tel: URIs allow visual indicators that are to be stripped out. Why not just insert one after the first digit if the number is 5 digits or fewer?

So, for 999, it would be tel:9-99?

parse_url() doesn't think it's a host:port formatted URL and handles it how the telephone module wants it to parse it.

Browsers will strip out the dash and call the proper number.

Simple solution, no?

Patch demonstrating my idea in #64. Only applies to the telephone formatter and so won't break links to internal hostnames, for example "router:8080", which patch #54 does.

FWIW, the patch applies cleanly to current 8.6.x and 8.7.x branches.

I have tried the patch from #65 and it works for me, using Drupal Core 8.6.1

#65 looks great! I love the simplicity and elegance. Thanks!

Now we need:
- A test-only patch with the tests from #54
- A combined patch with those tests plus the fix from #65

Then this is RTBC.

I'll work on that now, stay tuned.

Thanks everyone!
-Derek

Interdiffs seem pointless, the patches are exactly what I said.

Authorship should definitely go to @mjpa (fix) and/or @kyvour (for the tests). Perhaps commit #69 authored by kyvour and then #65 by mjpa.

Thanks!
-Derek

Oh right, except those tests only show that https://bugs.php.net/bug.php?id=70588 is not yet resolved, since the fix in #65 is at a different layer of the system. #54 only works because we're testing and changing fromUri(), not just the place it's being invoked in a way that's tickling PHP's internal bugs/problems. Sorry for that noise.

If we want #65, we need a different test that actually touches the layer we're fixing.

Leaving at Needs work, but adding a tag to indicate we need a test (and test-only patch) showing the current bug we're facing when trying to use the telephone link formatter. Stay tuned again.

Maybe just this?

This also includes link to https://bugs.php.net/bug.php?id=70588 in code comments per @catch in #60.
Perhaps this is now RTBC? ;)
Let's see what the bot thinks.

Bot is happy with #74 and #75 both as expected. Test is pretty simple. I did add the hyphen to the link we're asserting in #75 to make sure it passes, but the whole thing references a work-around for a PHP bug so I think that's legit. I can no longer RTBC this. Anyone else want to review / test / RTBC?

Thanks!
-Derek

Nice catch ! I try on both 8.6.x & 8.7.x. Everything works like a charm. For me it's RTBC too.

That was a testbot hiccup. If you look at the history, the patch is passing again.

p.s. Dear core committer: the default commit message is now pointing at me as the primary author of this fix. Please make sure @mjpa gets authorship (at least of the fix from #64) and 1st billing in the issue credits. It was their idea, I only wrote the test. Thanks!

Bypassing the php-bug in parse_url() with the trick of adding a - in those phone-numbers, seems legit to me. Drupal core has the telephone field and it thus should support widely used short phone-numbers.
So RTBC for this solution.

Reading through earlier patches, one stands out.
Patch #54 also adds test for the coap+tcp and ms-word protocols. Those protocols were added in this issue, so it should not be a problem to not support them now and keep a narrow path by only supporting tel protocol. If we want coap+tcp and ms-word protocols too, this can be done in a follow up.

+++ b/core/modules/telephone/src/Plugin/Field/FieldFormatter/TelephoneLinkFormatter.php
@@ -67,6 +67,17 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
+      // If the telephone number is 5 or less digits, parse_url() will think
+      // it's a port number rather than a phone number which causes the link
+      // formatter to throw an InvalidArgumentException. Avoid this by inserting
+      // a dash (-) after the first digit - RFC 3966 defines the dash as a
+      // visual separator character and so will be removed before the phone
+      // number is used. See https://bugs.php.net/bug.php?id=70588 for more.
+      $phone_number = $item->value;
+      if (strlen($phone_number) <= 5) {
+        $phone_number = $phone_number[0] . '-' . substr($phone_number, 1);
+      }

PHP bug report #70588 states that the bug is not triggered by a number of 5 or less digits, but by a number lower than 65535.
Let's add that number to the code-comment.
And probably also use that number in the conditional (if ($phone_number < 65535)) and a test for the threshold phone-number tel:65534.

+++ b/core/modules/telephone/src/Plugin/Field/FieldFormatter/TelephoneLinkFormatter.php
@@ -67,6 +67,17 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
+      $phone_number = $item->value;
+      if (strlen($phone_number) <= 5) {
+        $phone_number = $phone_number[0] . '-' . substr($phone_number, 1);
+      }

It looks that above code will return different output for numbers with 5 or less digits.
Is $phone_number an array or string? What is in $phone_number[0]?
If the output is different, we should add test-coverage for it.

For the last 2 comments I put this back on Needs Work.

Doing parse_url('tel:80000'); will return FALSE which is even worse. So the check needs to be 5 digits or less. Guessing PHP internally checks for 5 digits or fewer and assumes a port number and so fails on anything >65535.

$phone_number is a string, and $phone_number[0] returns the first character. There's test coverage from @dww in #75 for this.

I'm tentatively setting back to RTBC because the 2 comments are not an issue.

Re: #82:

It looks that above code will return different output for numbers with 5 or less digits.

That is *exactly* the fix proposed in #64 that we're now going with. Indeed, there's a test for it.

Re:

a test for the threshold phone-number tel:65534.

Sure, that sounds okay. Frankly, I wouldn't mind tests for tel:65534, tel:65535 and tel:65536. But I agree with mjpa that we should still add the dash in the link to be safe for all 5 digit #s. Since this is simply an output hack to defend us from trouble, we should err on the side of caution and always do this for 5 digit numbers or less. If someone desperately wants to alter this behavior, they can override and extend the field formatter if they have to.

Still RTBC to me. We can add more tests in a follow-up if we want.

I agree with #84. More tests could be a plus but if can be done in a follow-up of this issue.
For me everything works properly. RTBC.

$phone_number is a string, and $phone_number[0] returns the first character. There's test coverage from @dww in #75 for this.

Learned something new today. I didn't know PHP parses string this way.
It looks a bit odd to me though, but that is a personal preference.
What about $phone_number = substr_replace($phone_number, '-', 1, 0);
http://php.net/manual/en/function.substr-replace.php

Doing parse_url('tel:80000'); will return FALSE which is even worse. So the check needs to be 5 digits or less. Guessing PHP internally checks for 5 digits or fewer and assumes a port number and so fails on anything >65535.<

Another new thing. I was under the assumption that only PHP bug report #70588 was a problem here.
With your information I found 4 ranges for parse_url('tel:...') 0, 1-65535, 65536-99999 and 100000+
Results:
parse_url("tel:0"); --> FALSE
parse_url("tel:1"); --> ['host' => 'tel', 'port' => 1]
parse_url("tel:65535"); --> ['host' => 'tel', 'port' => 65535]
parse_url("tel:65536"); --> FALSE
parse_url("tel:99999"); --> FALSE
parse_url("tel:100000"); --> ['scheme' => 'tel', 'path' => 100000]
Only the last one is correct.

So yes, definitely important to handle every 5 digit case in this issue!

I don't want to be a troublemaker, but what about single digit phone-numbers?
Those will result in <a href="tel:1-"></a>
Is that acceptable?

I would prefer to add tests for all edge-cases now, it probably won't happen in a follow-up.

• Changed the patch to use substr_replace as it's a bit clearer.
• Extended the comment a bit to include a note about 5 digit numbers above 65535 to make it clear we're working around 2 issues.
• Add test cases for the numbers in #86

I'm not going to say "tel:1-" looks good, but there isn't an alternative really...

Setting to needs review because tests.

Nice 👌

Great, thanks! This would be RTBC, except someone's probably going to way to see the new test-only patch, too. Here it is, the improved tests from #87 on their own. This will generate a bit of noise (bot will mark this issue needs work, we'll have to re-upload the full patch and RTBC it), but then we'll hopefully be done.

Thanks,
-Derek

I noticed that we strip whitespace from the phone number *after* we count digits. Therefore, "12 34 5" would fail as a phone number, even with the patch. The if (strlen($phone_number) <= 5) test will fail, we won't hit the work around, but we'll end up with "tel:12345" which we know fails.

I've added a new test for this case. In this comment I'll attach the new test only and a version of the new test with the old logic. Both of these should fail. Next comment will include the new test plus a fix to the logic. Stay tuned.

Same tests, new logic. Hopefully this should be green.

Sorry! Now without syntax errors. ;)

Thanks all for this awesome works !

Here I have some suggestions to improve the tests readability of Drupal\Tests\telephone\FunctionalTelephoneFieldTest

With these changes, we will improve the readability to move the whole setup part in the setup method - where it belongs to. And we will split the tests in 2 methods, one with the widget visibility & one with the phone number on form submit.

This will also separate responsibility (TelephoneDefaultWidget::formElement & TelephoneLinkFormatter::viewElements & we will be able to add some proper @covers tag).

1. Move the field setup in the setup part of this test

  // Add the telephone field to the article content type.
  FieldStorageConfig::create([
    'field_name' => 'field_telephone',
    'entity_type' => 'node',
    'type' => 'telephone',
  ])->save();
  FieldConfig::create([
    'field_name' => 'field_telephone',
    'label' => 'Telephone Number',
    'entity_type' => 'node',
    'bundle' => 'article',
  ])->save();

  entity_get_form_display('node', 'article', 'default')
    ->setComponent('field_telephone', [
      'type' => 'telephone_default',
      'settings' => [
        'placeholder' => '123-456-7890',
      ],
    ])
    ->save();

  entity_get_display('node', 'article', 'default')
    ->setComponent('field_telephone', [
      'type' => 'telephone_link',
      'weight' => 1,
    ])
    ->save();

2. Then, create a new method to assert the form widget visibility

  // Display creation form.
  $this->drupalGet('node/add/article');
  $this->assertFieldByName("field_telephone[0][value]", '', 'Widget found.');
  $this->assertRaw('placeholder="123-456-7890"');

3. And, create a new method to assert those various phone number property (basic and advanced) & use a dataProvider. This will simplify the tests with only 1 $this->drupalPostForm & $this->assertRaw using given phone & expected result.

  '123456789' => '123456789',     
  '1234 56789' => '123456789',
  '0' => '0-',
  '1' => '1-',
  '123' => '1-23',
  '1 2 3 4 5' => '1-2345',
  '65534' => '6-5534',
  '65535' => '6-5535',
  '65536' => '6-5536',
  '99999' => '9-9999',
  '100000' => '100000',

What do you think about it ?

Updated the test based on feedback in #96

Think this is what you meant @wengerk?

Yess mjpa ! This is exactly what I try to explain :D !
Seems pretty good. Thanks for your work on this !

Here still a minor change I would suggest (following code standards of Drupal):

1. add the inheritdoc to the setup method

  /**
   * {@inheritdoc}
   */

2. Remove space on array of ::providerPhoneNumbers

      'standard phone number' => ['123456789', '123456789'],
      'whitespace is removed' => ['1234 56789', '123456789'],
      'parse_url(0) return FALSE workaround' => ['0', '0-'],
      'php bug 70588 workaround - lower edge check' => ['1', '1-'],
      'php bug 70588 workaround' => ['123', '1-23'],
      'php bug 70588 workaround - with whitespace removal' => ['1 2 3 4 5', '1-2345'],
      'php bug 70588 workaround - upper edge check' => ['65534', '6-5534'],
      'php bug 70588 workaround - edge check' => ['65535', '6-5535'],
      'php bug 70588 workaround - invalid port number - lower edge check' => ['65536', '6-5536'],
      'php bug 70588 workaround - invalid port number - upper edge check' => ['99999', '9-9999'],
      'lowest number not affected by php bug 70588' => ['100000', '100000'],

I also added an interdiff with 94->99 for others reviewers.

Changes from #100 applied.

Fine to me.

I can no longer RTBC this. Anyone else want to review / test / RTBC?

@wengerk : Thanks for the suggestions! You haven't uploaded any patches, you're just making suggestions. I think you're still a candidate to RTBC this.

@mjpa : Thanks for the fixes! Looking great. Again, it seems the expectation in core dev is that you always upload a test-only version and a full version of each patch. If you do that yourself, it'll speed up the process. Thanks!

Here's test-only and a re-post of #101 so the bot doesn't get confused and set this issue to needs work.

Once the bot comes back red on the 1st and green on 2nd, this is RTBC.

Thanks!
-Derek

@dww: Thanks for the heads up, makes sense really, test only to show a bug / issue then a patch to fix the code so it passes the tests. Didn't realise that was the case but do now!

If you say so @dww !
I think it's RTBC then let's change his status !

Happy to see the bigger fix #2575577: UrlHelper does not support tel: exists. And also that @jcnventura opened a PHP bug because that is where some of this should be fixed to. However this is a good workaround for a common issue to let's go forward. I've credited all the contributors who have left reviews that have helped this issue get to rtbc.

Committed and pushed f1b1d3f87c to 8.7.x and 726220cbbc to 8.6.x. Thanks!

Great! :)

Nice one!

Happy that this went through.. Adding Barcelona tag, as this where I first paid attention to this.