Support from Acquia helps fund testing for Drupal Acquia logo

Comments

g1smd’s picture

This update is a complete rewrite of the Drupal CCK phone British file.
It corrects several serious flaws found in previous versions.

>> Verifies that $phonenumber is a valid eleven-digit United Kingdom phone number

UK phone numbers have 10 or 9 digits after the 0 trunk code or the +44 country code.
The old version of CCK phone could not cope with the 9 digit numbers found in more than 41 area codes as described at #570190: Valid UK numbers being rejected - change the rules?.

>> Regular expression adapted from Amos Hurd's regex at RegExLib.com

The original RegEx didn't cope with the 4+5, 5+5, 5+4, 3+6 formats that are in use. It worked only with 2+8, 3+7, 4+6 formats.

I have replaced it with a more detailed pattern similar to one of those found at:
http://www.aa-asterisk.org.uk/index.php/Regular_Expressions_for_Validati...

>> (^\+44\s?(\(0\))?\d{1}|^\(?0\d{1}\)?){1}\s?\d{5}\s?\d{5}
>> "1 digit area code with optional +44 internationalisation or not, optional spaces and brackets".

The above code is not valid. There's no such format in use in the UK and it's a digit too long.

>> Convert a valid United Kingdom phone number into standard +44 (0)1970 123 456 #001 international format.

The (0) is NOT valid in the international format. See:
http://www.aa-asterisk.org.uk/index.php/(0)
http://revk.www.me.uk/2009/09/it-is-not-44-0207-123-4567.html

>> If we already have the formatting we want, just return.

Never trust the user to enter their number in the right format. Just make sure the number, as entered, has the right number of digits and is in a valid range.

Users in London are equally likely to type 0203 000 5555 or 02030 005 555 as they are the correct 020 3000 5555 format.

Users in Glasgow may well type 01415 557 777 when they meant to enter 0141 555 7777.

Let them enter the number in any format they want, then clean it up for display later on.
"Be conservative in what you do, be liberal in what you accept from others." (Postel's Law)

011 44 20 3000 7000 is a perfectly valid UK phone number. It's a London number shown with the international access code as dialled from the US or Canada. It should never be shown like that, but if a user wants to enter it that way then let them do so. Immediately reformat it as +44 20 3000 7000 for display.

The important thing is to ensure that the user typed the correct number of digits and the number is in a valid range.

Once that has been done, use formatting rules to display it. These rules are sorted by both number length and by initial digits and are detailed at:
http://www.aa-asterisk.org.uk/index.php/Number_format
http://www.aa-asterisk.org.uk/index.php/Regular_Expressions_for_Validati...

>> Simplify to 10 digit number and clean up ready for international reformat.

Some 41 area codes have 9 digit numbers with a variety of area code lengths. CCK couldn't previously cope with those.

The new formatting routine correctly copes with any and every valid GB number range.

>> If there are some spaces in the number assume some level of preformatting.

No. Don't do that. The format might not be the right one for this number. Accept any format for entry, then clean up the format using the correct formatting rules for that number range.

>> If there are no spaces in the number assume 4 digit area code.

No. Absolutely do not do this. UK numbers can have 2, 3, 4 or 5 digit area codes.

Assuming 4 digit area codes for everything gets the codes wrong for 30 area codes including those of many major cities.

The correct rules are simple and are included in the new code.

Note: The digit count for the area code should not include the 0 trunk code.

The new routine works as follows. Users are allowed to enter numbers in almost any format they want to, either in international format with country code or in national format with leading zero.

The data is then cleaned and checked:
- remove and separately store any extension number details,
- remove any access code (+, 00, 011, etc), if present,
- remove the country code, if present, and store it for later use,
- use the default country code from form input when no country is specified in the number,
- remove all spaces and non-digits from whatever is left,
- remove any leading 0 if present, and
- check the length of the remaining number (the NSN) is valid for this range.

Finally, format the number according to the correct rules for the identified number range and return it in international format.

Separating the validation routines from the formatting routines makes for code that is much easier to maintain.

g1smd’s picture

The included files are as follows:

cck.phone_.gb_.inc_.test1_.txt -- this test file is run standalone and contains a boatload of test numbers and debug code. Rename this file to have a .php extension for standalone testing.

cck.phone_.gb_.inc_.test2_.txt -- this test file is run standalone. It has a long list of test numbers, but no debug code. Rename this file to have a .php extension for standalone testing.

cck.phone_.gb_.inc_.txt -- new version of CCK Phone GB with both test numbers and debug code removed. Rename this file to phone.gb.inc for testing within Drupal.

cck.phone_.gb_.patch -- patch for new version.

g1smd’s picture

FileSize
19.99 KB

Error in this patch - do not use:

g1smd’s picture

FileSize
19.99 KB

Error in this patch - do not use:

g1smd’s picture

Status: Needs review » Active
FileSize
19.99 KB

Error in this patch (built from folder, not root).

Use the one built by slcp a long way down the page.

g1smd’s picture

The above patch is ready for testing.

If you can't deal with .patch files, then scroll back up the page and download the file: cck.phone_.gb_.inc_.txt which contains the raw PHP code. Rename that file to phone.gb.inc and use it instead of the original.

g1smd’s picture

Any takers?

g1smd’s picture

format_gb_phone_number needs two parameters ($phonenumber) and ($field).

What does $field usually contain? Is it 'GB', 'gb', '44' or something else?

g1smd’s picture

Status: Needs review » Active
g1smd’s picture

g1smd’s picture

format_gb_phone_number needs two parameters ($phonenumber) and ($field).

What does $field usually contain? Is it 'GB', 'gb', '44' or something else?

slcp’s picture

Haven't checked your work or tested this yet but thanks in advance as it has to be an improvement.

Proper handling of GB numbers will be a treat. I will drop in some more feedback when I get to this.

slcp’s picture

Status: Active » Needs review

Ok, haven't looked at the code but it works a treat in my use case. No more undefined offset errors that it suffered from before.

g1smd’s picture

Status: Needs review » Active

Cool. I have more improvements planned for later on, but it would be good to get this version made official before I move on to the next stage.

g1smd’s picture

Status: Active » Needs review
slcp’s picture

What does $field usually contain? Is it 'GB', 'gb', '44' or something else?

This does not make sense, have you looked at the module code?

Nothing to do with you but $field should not be called $field as it represents the $instance['settings'] array. Your code implies that you know this:

        // Add prefix back on to NSN
        if ($field['phone_country_code']) {
            // Use formatting from form selector
            $phonenumber = '+44 ' . $phonenumberNSNformatted;
        } else {
            // Use formatting from type-in
            $phonenumber = $phonenumberPrefix . $phonenumberNSNformatted;
        }
g1smd’s picture

Status: Active » Needs review

I used $field['phone_country_code'] as that was in the original code.

It looked like it switched whether the returned value was always in international format, or in a tidied up national or international format dependant on whatever had originally been entered.

I'm merely checking that $field['phone_country_code'] "exists" rather than checking what the value is. I have concerns that I need to check the actual value.

I don't fully understand the rest of the code outside the GB module. The number format validation, range checking and reformatting within the module is correct.

Do I need to test the actual value of $field['phone_country_code'], and if so, what should I test for: '44', 'gb', 'GB' or something else?

if ($field['phone_country_code'] && $field['phone_country_code'] == "XXXX") {

slcp’s picture

Check 'setting' in the return of field_info_instance('entity_type', 'your_field') for the contents. And also check phone_field_presave() as this is where $field originates in this case.

$field['phone_country_code'] is a boolean representing the 'add country code if not present' field/instance setting.

Have you heard of the dsm($variable) function? If not, look it up :o)

slcp’s picture

I had dropped in your replacement file, just tried your patch and it does not apply from module root.

Remade patch to apply from module root.

g1smd’s picture

Thanks for re-making the patch. Not sure how I built it from within the folder. I usually build from the root.

Now where's the module owner? AWOL for 8 months now.

slcp’s picture

Status: Active » Needs review

Check out Dealing with unsupported (abandoned) projects if you fancy it :o)

g1smd’s picture

I'm pretty sure this is ready to commit.

What do other people think?

cweagans’s picture

Assigned: Unassigned » cweagans
Status: Needs review » Needs work

No, this is not ready to commit just yet. You didn't follow any Drupal coding standards and you had in-file attribution (which is a no-no).

However, I'm going to clean it up and commit it here shortly.

cweagans’s picture

How's this look?

cweagans’s picture

Status: Needs work » Needs review
g1smd’s picture

The flat file rather than a patch would be better for me.

I have more changes queued up for GB after this, so I hope to get those in before anyone makes other changes to the code.

g1smd’s picture

I wish someone had pointed that out when I first submitted.

Seriously not impressed as I have done a lot of other work since.

cweagans’s picture

Well, you're rewriting the entire file here, so why not just submit all of your changes at the same time?

And the coding standards are public knowledge - you're welcome to peruse them as you see fit. http://drupal.org/coding-standards

jamesbisset’s picture

Just had to duplicate a phone field to plain text field on a live site because one phone number was 9 digits long, which broke the phone number and the HTML render too.

This rewrite sounds as if it will address this and a whole lot of other issues. There seems to have been a whole lot of work done, but it's been sitting 'in review' for eight months.

What action is needed to get it over the last hurdle?

seanr’s picture

I've been using #24 successfully for some time now, do we want to mark this RTBC? Maybe work on the other stuff you mention in a separate issue? It would be very helpful to get this patch in right now so the module at least works while we work on a better solution going forward.

MustangGB’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
MustangGB’s picture

Status: Reviewed & tested by the community » Needs work

"071xx xxxxxx" and "073xx xxxxxx" numbers fail in valid_gb_phone_range(), "072xx xxxxxx" are also coming soon.

Here is the change to fix it:

- $pattern_gb_valid_range .= '(1[1-9]|2[03489]|3[0347]|5[56]|7[04-9]|8[047]|9[018])\d{8}';
+ $pattern_gb_valid_range .= '(1[1-9]|2[03489]|3[0347]|5[56]|7[0-9]|8[047]|9[018])\d{8}';
MustangGB’s picture

Status: Needs work » Needs review
FileSize
19.16 KB

And an updated patch with #32.

MustangGB’s picture

Status: Needs review » Needs work

Been using this for ages, however it now triggers some warning notices in PHP 7.1, these can be fixed by ensuring a few variables are declared in extract_gb_phone_parts() before using them.

  function extract_gb_phone_parts($phone_number) {
    // RegEx to extract number parts: 44 prefix ($2), NSN ($3), extension ($4)
    $pattern_gb_number_parts = '/^';
    $pattern_gb_number_parts .= '(\(?(?:0(?:0|11)\)?\s?\(?|\+)(44)\)?\s?)?\(?0?(?:\)\s?)?'; # country or trunk prefix
    $pattern_gb_number_parts .= '(';
    $pattern_gb_number_parts .= '[1-9]\d{1,4}\)?[\d\s]+'; # NSN
    $pattern_gb_number_parts .= ')';
    $pattern_gb_number_parts .= '(\#\d+)?'; # optional extension
    $pattern_gb_number_parts .= '$/x';
+   $phone_number_nsn = NULL;
+   $phone_number_prefix = NULL;
+   $phone_number_extension = NULL;
    if (preg_match($pattern_gb_number_parts, $phone_number, $matches)) {

      // Extract NSN part of GB number
      if (ISSET($matches['3'])) {
        $phone_number_nsn_raw = $matches['3'];
        // Trim NSN and remove space, hyphen or ')' if present
        $phone_number_nsn = trim(str_replace(array(")", "-", " "), "", $phone_number_nsn_raw));

        // Extract 44 prefix if present and set prefix as 0 or as +44 and space
        if (isset($matches['2']) && $matches['2'] == '44') {
          $phone_number_prefix = '+44 ';
        }
        else {
          $phone_number_prefix = '0';
        }

        // Extract extension
        $phone_number_extension = NULL;
        if (isset($matches['4'])) {
          $phone_number_extension = ' ' . $matches['4'];
        }
      }
    }

    $phone_number_parts = array(
      'NSN' => $phone_number_nsn,
      'prefix' => $phone_number_prefix,
      'extension' => $phone_number_extension,
    );

    return $phone_number_parts;
  }

Needs a re-roll to add these.

djac’s picture

Re-rolled patch for PHP7 notices (as per #34).