This is a good idea, but it needs work.
- If I use
555-555-1234and 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
10 months ago Composer require failure - last update
10 months ago 84 pass - last update
10 months ago 84 pass - Status changed to Needs review
10 months 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
10 months 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
10 months ago 84 pass - Status changed to Needs review
10 months ago 10:38pm 22 November 2023 - last update
8 months 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
8 months 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
8 months 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
8 months 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
8 months 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.