Can the module come up with something like this:

if (is_array($node_field)) {
        foreach ($node_field as $delta => $item) {
         //format the phone number
          if ($item['value'] != '') {
             list($lang) = explode('_', $field['type']);
            $node_field[0]['value'] = format_phone_number($lang, $node_field[0]['value'], $field);
          }
        }
     }

instead of having endless if statements one after the other. When creating a patch you have to umcomfortably code within
the phone.module thath distracts enormously from the real work, e. g. adding phone number validation.

CommentFileSizeAuthor
#22 phone-alternate-numbers.patch644 bytesgnindl
#21 phone-6.x-3.x-dev-ITU.patch3.43 KBgnindl
#14 phone.module.diff17.49 KBagharbeia
#4 phone-6.x-3.x-dev.zip31.97 KBgnindl
#1 phone-6.x-2.x-dev.zip26.5 KBgnindl

Comments

gnindl’s picture

gnindl commented
Status: Active » Needs review
StatusFileSize
new phone-6.x-2.x-dev.zip26.5 KB

Basically I did the following:

- Made one field type and moved the different phone number types as options to the field settings
- Invented 3 new hooks:
- hook_phone_number_info(): Labels of formatter in option field settings widget
- hook_phone_number_settings(): Additional form settings of a phone number type
- hook_phone_number_validation(): Error message if validation fails
- Moved includes to include directory
- Moved simpletest to test directory

A phone number type (language) can now implement these three hooks while the first is mandatory. It should
be easier to write phone number types, f. e. South Africa, to it.

waddles’s picture

waddles commented

Thanks for your work. I don't have time to do a full review so hoping someone else can, but I wanted to say that I really support your efforts to modularise this module. It desperately needs functionality split out into .inc files to make it easier for more National Numbering Plans to be added.

tryitonce’s picture

tryitonce commented

Thanks for the work. With a bit of clearing the phone_module and uploading the contents of your file - phone-6.x-2.x-dev.zip I am getting something on the screen.

the first though is an error message I am reporting here as it might help in the development of this great work. (Warning - I have to be clear I have no idea about developing modules or the code used in them.):

    * warning: Illegal offset type in isset or empty in /home/mallsand/public_html/sites/all/modules/cck/includes/content.crud.inc on line 372.
    * warning: Illegal offset type in /home/mallsand/public_html/sites/all/modules/cck/includes/content.crud.inc on line 372.
    * warning: Illegal offset type in unset in /home/mallsand/public_html/sites/all/modules/cck/includes/content.crud.inc on line 373.

When I enter a telephone number now in the User Profile in the phone field such as 34 950123456 I do not get an error message - it should be +34 950123456.
When I enter 34 95012345123456
When I enter 3495012345123456 the error message is - "3495012345123456" is not a valid international phone number
International phone numbers should contain a country code prefixed by a plus sign followed by the local number. - fine, but there are also too many digits - 16.
When I enter 34 95012345123456 the error message is - the same as above and the fact that there are 16 digits (default left at 15) is not recognised.
When I enter +34 95012345123456 the error message is - "+34 95012345123456" is not a valid international phone number
International phone numbers should contain a country code prefixed by a plus sign followed by the local number. It should just tell me there are two many digits now??
When I enter +34 9501234512345 - there is no error message - fine there is a plus a valid two digit country code and just 15 digits.
When I enter +349501234512345 - there is an error message - "+349501234512345" is not a valid international phone number
International phone numbers should contain a country code prefixed by a plus sign followed by the local number. fine - I left the space out
When I enter 34 9501234512345 - there is no error message - though there should be something telling us that + is needed
When I enter 999 9501234512345 - there is an error message - "999 9501234512345" is not a valid international phone number
International phone numbers should contain a country code prefixed by a plus sign followed by the local number. fine - but there are 16 digits not 15.
When I enter 999 950123451234 - there is an error message - wrong there should be a reference to the missing +.

Well, I hope this helps - any other way I could help - let me know - and thanks for the work

Good luck

gnindl’s picture

gnindl commented
Title: A more generic hook architecture » Generic hook architecture, ITU support & phone types
StatusFileSize
new phone-6.x-3.x-dev.zip31.97 KB

I think I have overcome the problems from #3. The error message came from some dirty form settings saving.

The test should be fine now. @tryitonce: adding some formalized simple validation tests in /tests/phone.inc.test would be
very helpful. You just have to copy and insert a function like this in the class/file:

function testBasisNormalizeHyphens() {
    $this->assertConversion('+48-22-5200600', '+48 22 5200600');
  }

and modify the function name and its arguemnts: the first argument is the input, the second the expected output. Write FALSE if the input should not be acceptable.

Done:
- Removed hook_phone_number_validation() and added error message to hook_phone_number_info()
- Various simplifications and cleaning in phone.module
- Phone types (Office, Home, Mobile, Other) support (in widget as select box and as additional information in the view theme)
- Added various test functions

ToDos (any volunteers?):
- Multiple ITU number support realized by slashes('/')
- Update script from version 2.x

jguffey’s picture

jguffey commented

Hey guys, thanks for the dev version of this module, I really needed phone number types.

However, I discovered a problem. I'm using a singe phone field with unlimited instances (so you can attach multiple phone numbers to one node). By default, when creating a new node it opens 2 phone fields for me. If I only use one, it still saves the other field, with and empty phone number, but the default type "office". I tried adding another value for type "null" but even then it still saves the extra, empty phone number, now with type "null".

I'll keep working on a solution to this problem, I feel close to one, but just wanted to make you aware of it.

Thanks.

jguffey’s picture

jguffey commented

Sorry,

found the issue I mentioned here (http://drupal.org/node/275843#comment-1689888)

Always check issues before posting! :/

tryitonce’s picture

tryitonce commented

thanks gnindl , danke,

for the quick response - unfortunately I had to put it a bit to the back of the task list and shall hopefully come round to it in a week or so.

great - help - I might to get a bit more advice from you as I am not a programmer and my code knowledge is limited.

drewish’s picture

drewish commented

gnindl, I wish you'd post patches rather than .zip files. It's much harder to review what you're doing.

Have you looked at #407844: Refactor country metadata into a single function? It might clean some stuff up ahead of your patch.

Daniel A. Beilinson’s picture

Daniel A. Beilinson commented

@gnindl, could you please roll patch who kill phone types feature or make it optional?

niklp’s picture

niklp commented

I definitely concur that moving towards having ONE "phone" type field and not this crazy load of extra junk in the cck field type selectors needs to happen ASAP, as a matter or urgency. It's madness as it stands and I don't want to use this right now because I'm so confident it will change.

I'm probably leaving this out and using a text field for now - what are the likely ramifications on the data if I do that and want to move later? Or is it likely that if I carry on and use phone.module, DB updates will be incorporated that will cater for each individual case no matter what?

gnindl’s picture

gnindl commented
Title: Generic hook architecture, ITU support & phone types » Proposal for 3.x-dev snapshot: Generic hook architecture, ITU support & phone types

@drewish, I propose here a reengineering and reorganisation of the module architecture, there would be too many patches to review.

I can't work with the current 2.x versions because I just want ONE field type and ONE widget type called 'Phone' . The phone number validation (ITU, Austrilian etc.) is configured in the settings and they are plugged in via hooks. Further I need typing for phone numbers, like 'office', 'fax', 'mobile' etc.

The purpose of this suggestion is not a patch but discussing hooks I've described in #1.

mattyoung’s picture

mattyoung commented

subscribe

agharbeia’s picture

agharbeia commented

Good work.

I've separately worked on remodularisation in ticket #613606: refactoring and simplification suggesition, adding what could be described as sub-module auto-discovery, and reflection.

More work is needed with the architecture of this module. For example function phone_field_settings($op, $field) creates and handles help entries and special fields for certain country selections (currently int and ca). This functionality should be moved to the respective country sub-modules.

agharbeia’s picture

agharbeia commented
StatusFileSize
new phone.module.diff17.49 KB

I'm attaching here the diff of the changes I've made to version 6.x-2.9 of phone.module

gnindl’s picture

gnindl commented

Good luck, guys!

I gave the patches up for now and develop on my custom code base. The module maintainer doesn't seem to be interested in such a rewrite/refactoring although it looks common sense to me.

Desparetely waiting for a new "Better Phone" module or Field API of Drupal 7.

drewish’s picture

drewish commented

it seems like this module has been abandoned so if someone is interested in taking it over they should start that process.

thierry_gd’s picture

thierry_gd commented

The module has not been abandonned, some new languages continue to be added from time to time

agharbeia’s picture

agharbeia commented

Well, yes, some new countries were recently added in phone 6.x-2.11, but I think the re-factoring suggested by many here is good and needed.

By the way, why does release 6.x-2.11 has only "some fix" in the release notes, thierry_gd, when in fact new countries have been added. this is not a trivial addition? Can you be more verbose about it?

jeffschuler’s picture

jeffschuler
Location Boulder, Colorado
commented

+1 for phone types...

thierry_gd, any word on whether you're considering these changes for addition to the module, please?

colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
commented

Subscribe

gnindl’s picture

gnindl commented
StatusFileSize
new phone-6.x-3.x-dev-ITU.patch3.43 KB

The ITU standard should allow the word "extension" or "ext.":

"To show an extension number of a PABX without direct in-dialling, the nationally used word or abbreviation
for “extension” should be written immediately after the telephone numbers and on the same line as the word
“telephone”, followed by the extension number itself."

Patch included on snapshot from #4

gnindl’s picture

gnindl commented
StatusFileSize
new phone-alternate-numbers.patch644 bytes

Patch for dev snapshot of #4.

Allowing alternative phone numbers separated by a slash "/" according to the ITU standard, paragraph 7.4.