Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#5 | phone-407844.patch | 25.36 KB | ckng |
#3 | phone_407844_2.patch | 23.19 KB | ckng |
#1 | phone_407844.patch | 15.32 KB | drewish |
phone-refactor.patch | 13.58 KB | malaussene | |
Comments
Comment #1
drewish CreditAttribution: drewish commentedI 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.
Comment #2
drewish CreditAttribution: drewish commentedbetter title.
Comment #3
ckngFurther 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
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.
Comment #4
ckngHmm.. a bump, I think this should be checked in, it is troublesome to keep modifying just to add some local phone support.
Comment #5
ckngReroll 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
Comment #6
agharbeia CreditAttribution: agharbeia commentedGood 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.
Comment #7
ckngAll country specific codes have been moved into their own inc file in #5
Comment #8
ckngIs 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.
Comment #9
ckngJust 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..
Comment #10
ckngGiving 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.
Comment #11
drewish CreditAttribution: drewish commentedchng, 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.
Comment #12
ckngI 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.
Comment #13
thierry_gd CreditAttribution: thierry_gd commentedHi,
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.
Comment #14
Cyberwolf CreditAttribution: Cyberwolf commentedSubscribing.