Don't set empty preferred_domestic_carrier_code, and if we are passed such a number from clients then treat the empty field as if unset. (#1364)

cpp/src/phonenumbers/phonenumberutil.cc

@@ -991,7 +991,11 @@ void PhoneNumberUtil::FormatNationalNumberWithPreferredCarrierCode(
     string* formatted_number) const {
   FormatNationalNumberWithCarrierCode(
       number,
-      number.has_preferred_domestic_carrier_code()
+      // Historically, we set this to an empty string when parsing with raw
+      // input if none was found in the input string. However, this doesn't
+      // result in a number we can dial. For this reason, we treat the empty
+      // string the same as if it isn't set at all.
+      !number.preferred_domestic_carrier_code().empty()
           ? number.preferred_domestic_carrier_code()
           : fallback_carrier_code,
       formatted_number);
@@ -1027,9 +1031,13 @@ void PhoneNumberUtil::FormatNumberForMobileDialing(
           number_no_extension, kColombiaMobileToFixedLinePrefix,
           formatted_number);
     } else if ((region_code == "BR") && (is_fixed_line_or_mobile)) {
-      if (number_no_extension.has_preferred_domestic_carrier_code()) {
-      FormatNationalNumberWithPreferredCarrierCode(number_no_extension, "",
-                                                   formatted_number);
+      // Historically, we set this to an empty string when parsing with raw
+      // input if none was found in the input string. However, this doesn't
+      // result in a number we can dial. For this reason, we treat the empty
+      // string the same as if it isn't set at all.
+      if (!number_no_extension.preferred_domestic_carrier_code().empty()) {
+        FormatNationalNumberWithPreferredCarrierCode(number_no_extension, "",
+                                                     formatted_number);
       } else {
         // Brazilian fixed line and mobile numbers need to be dialed with a
         // carrier code when called within Brazil. Without that, most of the
@@ -2017,7 +2025,7 @@ PhoneNumberUtil::ErrorType PhoneNumberUtil::ParseHelper(
     if (TestNumberLength(potential_national_number,
                          country_metadata->general_desc()) != TOO_SHORT) {
       normalized_national_number.assign(potential_national_number);
-      if (keep_raw_input) {
+      if (keep_raw_input && !carrier_code.empty()) {
         temp_number.set_preferred_domestic_carrier_code(carrier_code);
       }
     }

cpp/test/phonenumbers/phonenumberutil_test.cc

@@ -999,12 +999,18 @@ TEST_F(PhoneNumberUtilTest, FormatWithPreferredCarrierCode) {
   phone_util_.FormatNationalNumberWithPreferredCarrierCode(ar_number, "",
                                                            &formatted_number);
   EXPECT_EQ("01234 19 12-5678", formatted_number);
-  // When the preferred_domestic_carrier_code is present (even when it contains
-  // an empty string), use it instead of the default carrier code passed in.
+  // When the preferred_domestic_carrier_code is present (even when it is just a
+  // space), use it instead of the default carrier code passed in.
+  ar_number.set_preferred_domestic_carrier_code(" ");
+  phone_util_.FormatNationalNumberWithPreferredCarrierCode(ar_number, "15",
+                                                           &formatted_number);
+  EXPECT_EQ("01234   12-5678", formatted_number);
+  // When the preferred_domestic_carrier_code is present but empty, treat is as
+  // unset and use instead the default carrier code passed in.
   ar_number.set_preferred_domestic_carrier_code("");
   phone_util_.FormatNationalNumberWithPreferredCarrierCode(ar_number, "15",
                                                            &formatted_number);
-  EXPECT_EQ("01234 12-5678", formatted_number);
+  EXPECT_EQ("01234 15 12-5678", formatted_number);
   // We don't support this for the US so there should be no change.
   PhoneNumber us_number;
   us_number.set_country_code(1);
@@ -3620,9 +3626,6 @@ TEST_F(PhoneNumberUtilTest, ParseNumbersWithPlusWithNoRegion) {
 
   nz_number.set_raw_input("+64 3 331 6005");
   nz_number.set_country_code_source(PhoneNumber::FROM_NUMBER_WITH_PLUS_SIGN);
-  // It is important that we set this to an empty string, since we used
-  // ParseAndKeepRawInput and no carrrier code was found.
-  nz_number.set_preferred_domestic_carrier_code("");
   result_proto.Clear();
   EXPECT_EQ(PhoneNumberUtil::NO_PARSING_ERROR,
             phone_util_.ParseAndKeepRawInput("+64 3 331 6005",
@@ -3863,7 +3866,6 @@ TEST_F(PhoneNumberUtilTest, ParseAndKeepRaw) {
   alpha_numeric_number.set_raw_input("800 six-flags");
   alpha_numeric_number.set_country_code_source(
       PhoneNumber::FROM_DEFAULT_COUNTRY);
-  alpha_numeric_number.set_preferred_domestic_carrier_code("");
 
   PhoneNumber test_number;
   EXPECT_EQ(PhoneNumberUtil::NO_PARSING_ERROR,

java/libphonenumber/src/com/google/i18n/phonenumbers/PhoneNumberUtil.java

@@ -1247,9 +1247,13 @@ public class PhoneNumberUtil {
    */
   public String formatNationalNumberWithPreferredCarrierCode(PhoneNumber number,
                                                              String fallbackCarrierCode) {
-    return formatNationalNumberWithCarrierCode(number, number.hasPreferredDomesticCarrierCode()
-                                                       ? number.getPreferredDomesticCarrierCode()
-                                                       : fallbackCarrierCode);
+    return formatNationalNumberWithCarrierCode(number,
+        // Historically, we set this to an empty string when parsing with raw input if none was
+        // found in the input string. However, this doesn't result in a number we can dial. For this
+        // reason, we treat the empty string the same as if it isn't set at all.
+        number.getPreferredDomesticCarrierCode().length() > 0
+        ? number.getPreferredDomesticCarrierCode()
+        : fallbackCarrierCode);
   }
 
   /**
@@ -1286,7 +1290,10 @@ public class PhoneNumberUtil {
         formattedNumber =
             formatNationalNumberWithCarrierCode(numberNoExt, COLOMBIA_MOBILE_TO_FIXED_LINE_PREFIX);
       } else if (regionCode.equals("BR") && isFixedLineOrMobile) {
-        formattedNumber = numberNoExt.hasPreferredDomesticCarrierCode()
+        // Historically, we set this to an empty string when parsing with raw input if none was
+        // found in the input string. However, this doesn't result in a number we can dial. For this
+        // reason, we treat the empty string the same as if it isn't set at all.
+        formattedNumber = numberNoExt.getPreferredDomesticCarrierCode().length() > 0
             ? formattedNumber = formatNationalNumberWithPreferredCarrierCode(numberNoExt, "")
             // Brazilian fixed line and mobile numbers need to be dialed with a carrier code when
             // called within Brazil. Without that, most of the carriers won't connect the call.
@@ -3014,7 +3021,7 @@ public class PhoneNumberUtil {
       if (testNumberLength(potentialNationalNumber.toString(), regionMetadata.getGeneralDesc())
               != ValidationResult.TOO_SHORT) {
         normalizedNationalNumber = potentialNationalNumber;
-        if (keepRawInput) {
+        if (keepRawInput && carrierCode.length() > 0) {
           phoneNumber.setPreferredDomesticCarrierCode(carrierCode.toString());
         }
       }

java/libphonenumber/test/com/google/i18n/phonenumbers/PhoneNumberUtilTest.java

@@ -731,10 +731,15 @@ public class PhoneNumberUtilTest extends TestMetadataTestCase {
         phoneUtil.formatNationalNumberWithPreferredCarrierCode(arNumber, "15"));
     assertEquals("01234 19 12-5678",
         phoneUtil.formatNationalNumberWithPreferredCarrierCode(arNumber, ""));
-    // When the preferred_domestic_carrier_code is present (even when it contains an empty string),
-    // use it instead of the default carrier code passed in.
+    // When the preferred_domestic_carrier_code is present (even when it is just a space), use it
+    // instead of the default carrier code passed in.
+    arNumber.setPreferredDomesticCarrierCode(" ");
+    assertEquals("01234   12-5678",
+        phoneUtil.formatNationalNumberWithPreferredCarrierCode(arNumber, "15"));
+    // When the preferred_domestic_carrier_code is present but empty, treat it as unset and use
+    // instead the default carrier code passed in.
     arNumber.setPreferredDomesticCarrierCode("");
-    assertEquals("01234 12-5678",
+    assertEquals("01234 15 12-5678",
         phoneUtil.formatNationalNumberWithPreferredCarrierCode(arNumber, "15"));
     // We don't support this for the US so there should be no change.
     PhoneNumber usNumber = new PhoneNumber();
@@ -2192,12 +2197,9 @@ public class PhoneNumberUtilTest extends TestMetadataTestCase {
     assertEquals(NZ_NUMBER, phoneUtil.parse("tel:03-331-6005;isub=12345;phone-context=+64",
         RegionCode.ZZ));
 
-    // It is important that we set the carrier code to an empty string, since we used
-    // ParseAndKeepRawInput and no carrier code was found.
     PhoneNumber nzNumberWithRawInput = new PhoneNumber().mergeFrom(NZ_NUMBER).
         setRawInput("+64 3 331 6005").
-        setCountryCodeSource(CountryCodeSource.FROM_NUMBER_WITH_PLUS_SIGN).
-        setPreferredDomesticCarrierCode("");
+        setCountryCodeSource(CountryCodeSource.FROM_NUMBER_WITH_PLUS_SIGN);
     assertEquals(nzNumberWithRawInput, phoneUtil.parseAndKeepRawInput("+64 3 331 6005",
                                                                       RegionCode.ZZ));
     // Null is also allowed for the region code in these cases.
@@ -2299,16 +2301,14 @@ public class PhoneNumberUtilTest extends TestMetadataTestCase {
   public void testParseAndKeepRaw() throws Exception {
     PhoneNumber alphaNumericNumber = new PhoneNumber().mergeFrom(ALPHA_NUMERIC_NUMBER).
         setRawInput("800 six-flags").
-        setCountryCodeSource(CountryCodeSource.FROM_DEFAULT_COUNTRY).
-        setPreferredDomesticCarrierCode("");
+        setCountryCodeSource(CountryCodeSource.FROM_DEFAULT_COUNTRY);
     assertEquals(alphaNumericNumber,
                  phoneUtil.parseAndKeepRawInput("800 six-flags", RegionCode.US));
 
     PhoneNumber shorterAlphaNumber = new PhoneNumber().
         setCountryCode(1).setNationalNumber(8007493524L).
         setRawInput("1800 six-flag").
-        setCountryCodeSource(CountryCodeSource.FROM_NUMBER_WITHOUT_PLUS_SIGN).
-        setPreferredDomesticCarrierCode("");
+        setCountryCodeSource(CountryCodeSource.FROM_NUMBER_WITHOUT_PLUS_SIGN);
     assertEquals(shorterAlphaNumber,
                  phoneUtil.parseAndKeepRawInput("1800 six-flag", RegionCode.US));
 

java/pending_code_changes.txt

@@ -1 +1,3 @@
-
+Code changes:
+ - Stop setting empty preferred_domestic_carrier_code, and if we are passed such
+   a number then treat the empty field as if unset.

javascript/i18n/phonenumbers/phonenumberutil.js

@@ -1682,7 +1682,11 @@ i18n.phonenumbers.PhoneNumberUtil.prototype.
         number, fallbackCarrierCode) {
   return this.formatNationalNumberWithCarrierCode(
       number,
-      number.hasPreferredDomesticCarrierCode() ?
+      // Historically, we set this to an empty string when parsing with raw
+      // input if none was found in the input string. However, this doesn't
+      // result in a number we can dial. For this reason, we treat the empty
+      // string the same as if it isn't set at all.
+      number.getPreferredDomesticCarrierCodeOrDefault().length > 0 ?
           number.getPreferredDomesticCarrierCodeOrDefault() :
           fallbackCarrierCode);
 };
@@ -1737,7 +1741,12 @@ i18n.phonenumbers.PhoneNumberUtil.prototype.formatNumberForMobileDialing =
           i18n.phonenumbers.PhoneNumberUtil
               .COLOMBIA_MOBILE_TO_FIXED_LINE_PREFIX_);
     } else if (regionCode == 'BR' && isFixedLineOrMobile) {
-      formattedNumber = numberNoExt.hasPreferredDomesticCarrierCode() ?
+      formattedNumber =
+          // Historically, we set this to an empty string when parsing with raw
+          // input if none was found in the input string. However, this doesn't
+          // result in a number we can dial. For this reason, we treat the empty
+          // string the same as if it isn't set at all.
+          numberNoExt.getPreferredDomesticCarrierCodeOrDefault().length > 0 ?
           this.formatNationalNumberWithPreferredCarrierCode(numberNoExt, '') :
           // Brazilian fixed line and mobile numbers need to be dialed with a
           // carrier code when called within Brazil. Without that, most of the
@@ -3905,7 +3914,7 @@ i18n.phonenumbers.PhoneNumberUtil.prototype.parseHelper_ =
             regionMetadata.getGeneralDesc()) !=
         i18n.phonenumbers.PhoneNumberUtil.ValidationResult.TOO_SHORT) {
       normalizedNationalNumber = potentialNationalNumber;
-      if (keepRawInput) {
+      if (keepRawInput && carrierCode.toString().length > 0) {
         phoneNumber.setPreferredDomesticCarrierCode(carrierCode.toString());
       }
     }

javascript/i18n/phonenumbers/phonenumberutil_test.js

@@ -974,10 +974,15 @@ function testFormatWithPreferredCarrierCode() {
       phoneUtil.formatNationalNumberWithPreferredCarrierCode(arNumber, '15'));
   assertEquals('01234 19 12-5678',
       phoneUtil.formatNationalNumberWithPreferredCarrierCode(arNumber, ''));
-  // When the preferred_domestic_carrier_code is present (even when it contains
-  // an empty string), use it instead of the default carrier code passed in.
+  // When the preferred_domestic_carrier_code is present (even when it is just a
+  // space), use it instead of the default carrier code passed in.
+  arNumber.setPreferredDomesticCarrierCode(' ');
+  assertEquals("01234   12-5678",
+      phoneUtil.formatNationalNumberWithPreferredCarrierCode(arNumber, '15'));
+  // When the preferred_domestic_carrier_code is present but empty, treat it as
+  // unset and use instead the default carrier code passed in.
   arNumber.setPreferredDomesticCarrierCode('');
-  assertEquals('01234 12-5678',
+  assertEquals('01234 15 12-5678',
       phoneUtil.formatNationalNumberWithPreferredCarrierCode(arNumber, '15'));
   // We don't support this for the US so there should be no change.
   /** @type {i18n.phonenumbers.PhoneNumber} */
@@ -2819,14 +2824,11 @@ function testParseNumbersWithPlusWithNoRegion() {
       phoneUtil.parse('tel:03-331-6005;isub=12345;phone-context=+64',
                       RegionCode.ZZ)));
 
-  // It is important that we set the carrier code to an empty string, since we
-  // used ParseAndKeepRawInput and no carrier code was found.
   /** @type {i18n.phonenumbers.PhoneNumber} */
   var nzNumberWithRawInput = NZ_NUMBER.clone();
   nzNumberWithRawInput.setRawInput('+64 3 331 6005');
   nzNumberWithRawInput.setCountryCodeSource(i18n.phonenumbers.PhoneNumber
       .CountryCodeSource.FROM_NUMBER_WITH_PLUS_SIGN);
-  nzNumberWithRawInput.setPreferredDomesticCarrierCode('');
   assertTrue(nzNumberWithRawInput.equals(
       phoneUtil.parseAndKeepRawInput('+64 3 331 6005', RegionCode.ZZ)));
   // Null is also allowed for the region code in these cases.
@@ -2984,7 +2986,6 @@ function testParseAndKeepRaw() {
   var alphaNumericNumber = ALPHA_NUMERIC_NUMBER.clone();
   alphaNumericNumber.setRawInput('800 six-flags');
   alphaNumericNumber.setCountryCodeSource(CCS.FROM_DEFAULT_COUNTRY);
-  alphaNumericNumber.setPreferredDomesticCarrierCode('');
   assertTrue(alphaNumericNumber.equals(
       phoneUtil.parseAndKeepRawInput('800 six-flags', RegionCode.US)));
 
@@ -2994,7 +2995,6 @@ function testParseAndKeepRaw() {
   shorterAlphaNumber.setNationalNumber(8007493524);
   shorterAlphaNumber.setRawInput('1800 six-flag');
   shorterAlphaNumber.setCountryCodeSource(CCS.FROM_NUMBER_WITHOUT_PLUS_SIGN);
-  shorterAlphaNumber.setPreferredDomesticCarrierCode('');
   assertTrue(shorterAlphaNumber.equals(
       phoneUtil.parseAndKeepRawInput('1800 six-flag', RegionCode.US)));