Posted by malaussene on March 19, 2009 at 11:52pm
| Project: | Phone |
| Version: | master |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
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
| Attachment | Size |
|---|---|
| phone-refactor.patch | 13.58 KB |
Comments
#1
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.
#2
better title.
#3
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.
#4
Hmm.. a bump, I think this should be checked in, it is troublesome to keep modifying just to add some local phone support.
#5
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
#6
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.
#7
All country specific codes have been moved into their own inc file in #5
#8
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.
#9
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..
#10
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.
#11
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.
#12
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.
#13
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.
#14
Subscribing.