This is a good idea, but it needs work.
- If I use
555-555-1234
and add a link to a phone number, it makes the linktel:+555-555-1234
. Phone numbers without a country code should not have a+
in front. - If I enter
tel:555-555-1234
, it still matches, but clicking the Phone match will make the linktel:+tel:555-555-1234
, which is definitely broken. This can be fixed with:
// Strip the tel: prefix to match only the phone part of the string. $string = str_replace('tel:', '', $string);
- Using
filter_var($string, FILTER_SANITIZE_NUMBER_INT)
is a bad idea, and it's not used anywhere in Drupal code.- The code doesn't conform to Drupal coding standards.
- Telephone links aren't only for mobile phones. A computer can define an action for the tel: prefix, such as calling from your computer with Skype or Signal. Maybe say
Opens configured phone app to call @phone
.- The matcher needs tests. Look at EmailMatcherTest.php for an example.
- If I use
- First commit to issue fork.
- Merge request !36Update phone matcher patch with tests and fix Drupal coding standards β (Open) created by hlopez
- last update
over 1 year ago Composer require failure - last update
over 1 year ago 84 pass - last update
over 1 year ago 84 pass - Status changed to Needs review
over 1 year ago 12:37am 21 November 2023 - πΊπΈUnited States hlopez
I applied the requested changes to the patch and created an issue fork for it. I'm uploading the original interdiff for the tests and coding standards predating the issue fork. I also changed the branch to 6.1.x-dev due to issues with Jenkins initializing the 6.0.x testing suite.
- Status changed to Needs work
over 1 year ago 3:03pm 21 November 2023 You could probably use
if (preg_match("/^[- +()0-9]+$/", $string) && (preg_match_all("/[0-9]/", $number) > 9)) {
instead of
if ((preg_match_all("/[- +0-9]/", $string) != FALSE) && (preg_match_all("/[0-9]/", $number) > 9)) {
Allowing parentheses is important, at least.
- last update
over 1 year ago 84 pass - Status changed to Needs review
over 1 year ago 10:38pm 22 November 2023 - πΊπΈUnited States hlopez
I reverted the change, thanks @solideogloria
- last update
about 1 year ago 83 pass, 2 fail - πΊπΈUnited States jsutta United States
I wasn't able to get a patch generated from the merge request to install, so I rolled a patch using the contents of the merge request. Posting it here in case anyone runs into the same issue. All that said, the functionality provided by the merge request works perfectly.
The last submitted patch, 11: 3273630-11.patch, failed testing. View results β
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.- Status changed to Needs review
about 1 year ago 10:36pm 5 February 2024 Moving back to NR, since the prior patch is for installing locally, not for 6.1.x-dev.
- last update
about 1 year ago 83 pass, 2 fail - πΊπΈUnited States jsutta United States
Reposting yesterday's patch, targeted against the 6.1.x-dev branch. (Please let me know if I got this wrong, I'm still learning how to do this, and I'm happy to fix it so it's right.)
I also checked the test failure and it appears to be related to line 178 in tests/src/FunctionalJavascript/LinkFieldTest.php:
$assert_session->elementAttributeContains('xpath', '//article/div/div/div[2]/a', 'rel', "nofollow");
Meaning the test failure isn't related to this change, but to another test. However, it's possible something in this change broke the test, so if needed I can look into it further.
- last update
about 1 year ago 83 pass, 2 fail - πΊπΈUnited States jsutta United States
That's odd, I specifically picked 6.1.x-dev to test against, but somehow it got set to 6.1.x?
The last submitted patch, 14: 3273630-14.patch, failed testing. View results β
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.I assumed the branch selected was why the test was failing, but maybe I was wrong.
Were you trying to use the MR patch with the dev version of the module locally?
- Status changed to Needs review
about 1 year ago 6:56pm 6 February 2024 Don't bother posting a new patch file. The MR patch is the one that matters, as only MRs can be used with GitLab-CI.
- πΊπΈUnited States thhafner Chicago, IL
Encountering a bug in this patch where ANY url that contains 9 consecutive digits is transformed into a tel link. Propose changing regex to something that does NOT match 9 digits without anything else.
For example,
^(\+\d{1,2}\s)?\(?\d{3}\)?[\s.-]\d{3}[\s.-]\d{4}$matches
123-456-7890
(123)-456-7890
(123) 456 7890
123 456 7890
123.456.7890
+91 (123) 456-7890among other combinations
I understand phone number matching opens up an entire can of worms, but this is what we are currently using to prevent erroneous tel links.
- πΊπΈUnited States thhafner Chicago, IL
One more time with the correct patch file.