Hi,
here is a patch cleaning up code duplication by centralizing the declaration of each country metadata (error, label...).
It then simplifies the addition of new countries.

malaussene

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drewish’s picture

Status: Active » Needs review
FileSize
15.32 KB

I like the idea but there seemed like there were a lot of helper functions. I've slimmed it down to just phone_country_metadata(), phone_countrycode_from_fieldtype() and _phone_field_types(). Also provided documentation for them. I dropped the 'cc' field from the metadata since it just repeated the key's value.

drewish’s picture

Title: Code cleaning » Refactor country metadata into a single function

better title.

ckng’s picture

FileSize
23.19 KB

Further refactor by moving out the country metadata into its own .inc, so the phone module is clean from country specific stuff. Now country can be added/removed easily and without the need to update phone.module.

Each country just need to implement phone_CC_metadata, where CC is the country code.

e.g. for es

function phone_es_metadata() {
  // These strings are translated using t() on output.
  return array(
    'label' => 'Spanish Phone Numbers',
    'error' => '"%value" is not a valid Spanish phone number<br>Spanish phone numbers should only contains numbers and spaces and be like 999 999 999',
  );
}

TODO:
- notice there are still codes for ca inside phone.module, maybe implement hook for those.

p/s: someone ought to use coder to clean up the code/spaces/line ending, easier to generate patch as IDE has been configured according to Drupal coder when saving file.

ckng’s picture

Hmm.. a bump, I think this should be checked in, it is troublesome to keep modifying just to add some local phone support.

ckng’s picture

FileSize
25.36 KB

Reroll patch againsts HEAD.

Changes:
- Fully moved all country specific codes to their own inc file (incl. int)
- Added (where CC is the country code)
phone_CC_metadata
phone_CC_number_settings
phone_CC_number_validate
phone_CC_number_save

agharbeia’s picture

Good work. I've built on that further more in ticket #613606: refactoring and simplification suggesition , adding 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.

ckng’s picture

All country specific codes have been moved into their own inc file in #5

ckng’s picture

Is this module dead?
I'm seeing 20K less from usage, is there an alternative I'm not aware of?

If it is not, I wonder why this patch is still hanging here, something wrong with the patch? This is a same issue lingering for few years already. Contemplating to fork this and have a 'Better Phone' module.

ckng’s picture

Just checked out latest phone code, seems like some parts of this patch are already in, except the phone.module portion where the country specific codes just grew worst, why the partial commit??

EDIT- spoke too soon, the patch is not committed in anyway, it is still the 'old phone way' of working..

ckng’s picture

Giving up to keep re-rolling and getting this patch or new features added.
I'm releasing a new Phone Number (http://drupal.org/project/cck_phone) CCK field taking another approach.

drewish’s picture

chng, I'm usually opposed to forking modules but after watching the maintainer refuse to work on it or give it up I have to say it's the right call.

ckng’s picture

I dislike and not supporting forking of module as well, but it is not really a fork. The fundamental of cck_phone is rather different but is something I would like to see phone will be heading but it seems like it is not going anywhere or have a direction.

thierry_gd’s picture

Hi,

No need to fork, I am going to take into account the refactoring in the next weeks as I will have some free time to do so.

Cyberwolf’s picture

Subscribing.