phone_field_presave() calls format_phone_number(), breaking input data integrity

Oh yes, definitely!

But you're getting your $ids and field deltas confused.
Also, format_phone_number() should get the instance settings passed, not the field settings.

Actually, it's strange to have formatting of the phone number depend on field instance settings.

1)
It would be more logical if there was a field formatter with the 'phone_country_code' ("Add the country code if not filled by the user") setting, and that setting was deleted from the instance itself.

(This would also open up the way for other formatter options, like for instance: "use spaces instead of dashes".)

2)
...'unfortunately', we also have the 'phone_default_country_code' setting for fields with settings['country'] == 'int'. Right now, I am divided if we should keep using this setting on presave.

• it goes against the principle of not changing the data before saving it...
• ...but if we don't do this (i.e. add the international prefix before saving), then we introduce the new situation that fields with country == 'int' are saved into the database as non-full normalized numbers. Could that hurt anyone's install?
• if we don't do this, we certainly should not allow the setting to be changed anymore, after the field holds data. (At this moment, it's changeable.)

Opinions?

Thinking about my comment #2 more: I guess we really should not changing the data before saving it. Also when settings['country'] == 'int' and the input phone number has no country extension, we should not add one.

Why? Because the source for the country code could be external, like a country dropdown. (It isn't at the moment, but it could be... ;) ) If that is the case, we don't want to save the local input number with added country code. Because what should happen then, at the moment the user changes the country dropdown?

So:

• Scrap my previous point #2.
• except this: the 'phone_default_country_code' setting stays a field instance setting but it should at least get a warning text that changing the country number, may change (the output for) existing numbers already in the database. And possibly, changing the setting should be disabled?
• point #1 still stands: 'phone_country_code' should not be a field instance setting - but a field formatter setting. (And the field formatter should also take into account the instance setting for 'phone_default_country_code'. Well, that's possible...)

This patch does exactly that. Phone_country_code is now a field_formatter_setting, phone_field_presave function is not necessary as format_phone_number is called from hook_field_formatter_view(). Formatter settings are passed to the format function instead of instance settings.

phone_default_country_code is still an instance setting and is used in phone.int.inc, so for now I merged this array element to $display['settings']. There should be a more concise solution for this though.

Same patch as in #6, but without unnecessary whitespaces :)

Weehee, it's my FIRST EVER code review using DREditor!

Not in your patch::

function phone_field_info() {

        'phone_country_code' => 0,

line should be deleted.

1. +++ b/phone.module
   @@ -187,6 +167,7 @@ function phone_field_formatter_info() {
   +      'settings' => array('phone_country_code' => 1),
   

   Should be 0 by default. It's also the default for the now-deleted instance setting. And I think 0 is the most widely-used setting:
   * many people who have country-specific phone fields will not want the country code
   * for 'int' fields, the country code is usually already present, so having 0 is fine.

2. +++ b/phone.module
   @@ -196,11 +177,22 @@ function phone_field_formatter_info() {
          $text = check_plain($item['value']);
   
   @@ -208,10 +200,44 @@ function phone_field_formatter_view($entity_type, $entity, $field, $instance, $l
   +  $summary = '';
   

   Unnecessary code. (2x)

3. +++ b/phone.module
   @@ -196,11 +177,22 @@ function phone_field_formatter_info() {
   +        $display['settings']['phone_default_country_code'] = $field['settings']['phone_default_country_code'];
   

   $instance, not $field, no?

   On my install, it gives:
   Notice: Undefined index: phone_default_country_code in phone_field_formatter_view().

   Pro tip: when developing modules, set error_reporting = E_ALL in your PHP settings. (If you don't want to do that for some of your own sites, I won't blame you ;) but it's necessary for public code.)

Further: I thought about making an update function to move the current instance setting into display settings (for all view modes!)... but don't think it's strictly necessary, if the default setting value is made 0. Up to you.

Rrright, thank you for your feedback! An update function could be useful as well, but I don't really see how could we modify this kind of data there. Here is the corrected patch until then.

Read the instance data, update it (move the setting from field instance settings to formatter settings; they must both be in the same data structure somewhere) and update the data.

I see there's field_read_instance() and field_update_instance() for that.

I won't test that because I don't need it myself, and upgrade paths are not officially required yet because the module is still in beta.

I will act like you set this to 'needs review' and put the issue to RTBC now, since I tested your changes. If you want to write the update process, you can set this back to Needs Review when uploading your patch.

I'm not sure this really fixes the problem? With a field configured to allow an international number it accepts just about any string that has numbers and a few control characters, country code or not. It also doesn't add the plus symbol. Is this what is expected?