Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Let's merge phone.module and cck_phone.module. While we're at it, let's rewrite it to use libphonenumber (same library that is used in the Android framework).
Comments
Comment #1
slcp CreditAttribution: slcp commentedI think this is a very good idea. I think in the mean time though we should try and get the patches that are RTBC in the issue queue committed. There are some active contributors that may well want to help with this effort and we may be at risk of losing their interest if the current branch cannot be kept on top of.
Comment #2
IT-CruSounds good! Which will be the new project URL?
Comment #3
mengi CreditAttribution: mengi commentedI am looking into using phone module for a future site, and I was wondering which module will be kept?
Thanks for all the work!
Comment #4
cweagansPhone will be kept, but that's going to take a while. It shouldn't have any bearing on your decision now.
Comment #5
Nephele CreditAttribution: Nephele commentedI've been waiting two years for a viable phone field module in drupal 7.x and I think the top priority needs to be getting one of these modules to a stable release. Getting everyone collaborating on a single module seems like the only useful long-term option. I think the very existence of two incompatible phone fields is probably the main reason why neither phone field has a stable release.
Basically right now I'm a frustrated programmer who needs a phone field module and is willing to contribute code in order to get a useable product. However, because of the dysfunctional status of these two modules, I feel like it's impossible to work on either module. cck_phone hasn't been updated in a year and the maintainer hasn't responded to any issues in six months. On the other hand, phone is unusable for me until #641984: Single phone field supporting all formats is fixed -- but there's been no progress on that issue in three years. Not even a single patch has been written.
Although I understand slcp's point about the importance of getting a stable release, a stable release of the phone module is currently useless to me, because the module doesn't contain the features that I need. If the fastest way to get those required features into a stable module is to start from scratch, then I'd rather work on brand new code.
So I suppose I'm wondering:
Comment #6
ckngThe main reason cck_phone was created is previous phone maintainer wont accept any patches, personally have few patches that go back more than 5 years, which the community had been keep re-rolling them. No longer interest to dwell in that, just mention here for completeness sake.
cck_phone dev is quite stable and used by many. The only reason there are no stable release is not all features planned for D7 are included yet. Understand that most if not all modules are contributed on our free time. Instead of waiting for 2 years, it would be wise to start contributing, testing, and if you can code, contributing and patching. cck_phone already provided the basic and more, for other edge cases one have to contribute and make it as an option, as majority of people don't need it.
The agreed direction for this merge is to reuse phone namespace and libphonenumber. No work has been started. libphonenumber should cover most of our bases, the next in line of importance probably is migration from D6/D7 phone and cck_phone.
Comment #7
cweagansThere's still a nontrivial amount of work to be done to be able to use libphonenumber. I do not have the time to do that right now and it's very far down on my list of things to get done, so I do not foresee being able to make substantial progress on it in the near future. If somebody wants to pick up that concept and run with it, I'm happy to provide guidance, timely commits, etc., but I probably will not end up doing much development for at least the next 4-6 months.
Comment #8
Nephele CreditAttribution: Nephele commentedThanks for the quick responses, and the honest assessments of where the projects stand.
However, the responses essentially confirm my original impression: it doesn't really make sense for me to work on either existing module. I can code a minimal feature for my existing website far more quickly than I can code it to be a proper patch, with all the options/UI/etc necessary to be part of a contrib module (especially given that I'd have to start by converting my existing website to use the module). It only makes sense to do all the extra work if it's likely that the patch is going to help others -- but if the 7.x-1.x branches are dead ends, the extra work is wasted.
As crazy as it may sound, I'd much prefer to tackle creating a merged, libphonenumber-based version of the phone module than write patches for the 7.x-1.x branches -- as long as there a good chance that the code I write could be adopted as the starting point for a phone-7.x-2.x-dev branch.
Specifically what I'm proposing putting together is a phone module that:
I fully realize the scope of what I'm proposing to do. I'd just like feedback before I go any further on whether this is consistent with what's been proposed here and could be useful.
Comment #9
cweagansGreat! If you do that, we will have the basis of a very strong 7.x-2.x branch. I will gladly accept those contributions. Let me know if you need guidance or code review at any point and I'll do what I can (use my contact form or comment here).
Comment #10
cweagansAlso, if you could, please do this development in some public repository somewhere (a Drupal.org sandbox would be appropriate) so that other people can easily jump in and help if they are so inclined :)
Comment #11
ckngAgreed to comments #8, #9, #10.
Ping me if you need code review or testing.
Comment #12
cdale CreditAttribution: cdale commentedHi Guys. I needed this like.. yesterday... so I stayed up all night and did it. The result is at http://drupal.org/sandbox/cdale/1925578
Happy to give whoever commit to the sandbox, and looking for feedback on the module in general.
Some notes:
I'm sure there's more, but lets get some discussion and movement forward. Happy to chat about any decisions I've made that are not obvious.
Use. As far as I can tell, it's pretty stable and ready to roll!
Comment #13
cdale CreditAttribution: cdale commentedComment #14
cweagansWhoa, that's awesome!
I've got a couple of review points before I merge this in as 7.x-2.x:
- There's a few points in the module that have some fallback logic in the event that libphonenumber is not available. Is there any reason not to get rid of these and add a hook_requirements in phone.install that looks for libphonenumber? This way, we can just assume that libphonenumber is available and it'll reduce the code complexity a little bit.
- To answer your todo at the top of phone.module, I don't think this is something that we can include with phone.module (nor is it something that I particularly want to include with Phone). However, is there a reason we need a modified copy of libphonenumber? Looks like you've just updated the metadata, but that seems like a fairly minor thing. I'd much prefer to use the canonical repository for libphonenumber and submit an issue asking the maintainer to update the metadata and/or merge your changes in.
- For your hook_library_info questions:
- The vendor URL should be https://github.com/davideme/libphonenumber-for-PHP (the original source of the port)
- The download URL should actually be pointing to a tag, rather than master. This is to ensure compatibility in the event that libphonenumber is modified in the future.
- It doesn't really look like the version is specified, so 1.0 is probably a good value there.
- I'd really love a full conversion to PSR-0 and a dependency on http://drupal.org/project/xautoload. This will make a port to Drupal 8 a lot easier in the future. This, however, would require maintaining a fork of libphonenumber....not sure if that's a good idea or not. Thoughts?
- We'll have to work on the Javascript and CSS included with your module to bring it up to coding standards, but that's not a commit blocker (I think most of the things in phone.js need to be Drupal.behaviors.[whatever] and there should definitely be some @file comments in there somewhere).
Follow up tasks (not commit blockers, but definitely release blockers):
- Get an upgrade path figured out
- Add feeds/migrate integration back in
- Anything else?
Comment #15
cweagansAlso...are you going to be in Portland? If so, can I buy you a beer or three for doing this?? :)
Comment #16
cdale CreditAttribution: cdale commentedHi. Thanks for the review. Sadly I won't be in Portland, but hopefully that doesn't stop you from buying a few beers. :) On with the comments.
- I actually already have a hook_requirements() that does just that. I played around with the idea of disallowing install altogether, but wasn't sure how to hook in denying install with the libraries module stuff. Any ideas or thoughts on that? Or is the runtime check enough? The fallback logic is actually quite simple to, only in validation and formatting. If we can find a way to make the module not install without libphonenumber, that would be my preferred approach, though see some notes further on.
- The only issue of possible different versions, is the supported regions list seems to change. When I did the update, several new regions appeared, and one actually disappeared, (did you know the Netherlands Antilles was dissolved as a country a few years back? http://en.wikipedia.org/wiki/Netherlands_Antilles, I did not).
My concern with this is as I use the list of supported regions from the library (I thought about allowing regions not in the library, hence the fallback), then it's entirely possible that an upgrade or some other "outside" action in the future could lead to loss of data, or at least confusion on some editing forms. Should we just check for this case perhaps in the edit widgets? I have no idea on how to go about handling that one, but maintaining a custom port seemed like the best approach, which I'm happy to do on github, and tie it in with the releases for this module. It will also allow us to tag and release in tandem with this module as and when needed. The only time I see a need to increase a version number in the libraries info hook, would be when a country is removed or added. I've already got a few extra in hook_countries_alter() that drupal does not know about, and drupal has about 8 countries that libphonenumber does not know about, granted the only of of those with a valid calling code is Antarctica. Hopefully that won't be a problem for anyone.
- The upstream maintainer hasn't done anything in about 6 months, nor responded to any issues in that time. I actually forked my version from someone else who has made quite a few changes, on quite important one being the namespace change, effectively meaning the phone module will not work with the original right now, and only one or two forks.
- I blindly copied the CSS and JS from cck_phone. :) I haven't actually gone over it, I only did what I needed to do to get selectors to work. I'm not up to speed with the coding standards on JS and CSS files. I spend most of my time in PHP land.
- I almost included the feeds and migrate files from phone, but to be honest, I have no experience with those modules. They looked straight forward enough, I just didn't understand their exact use case, so found it hard to be sure they would work.
The upgrade path might be a PITA. Thankfully we have this handy libphonenumber code that can make it easy for us.
In regards to maintaining a separate fork, I personally think we should. This would allow us to move to the PSR-0 stuff in the future, and even include a metadata update option in the module itself, rather than having upstream do it. Again, need to be careful about loosing countries. Adding is not so much of an issue. I'm happy to maintain a port, and for now, my repo on github is not going anywhere. I'm really looking for feedback from others as to what a good approach might be. If we do go with using the original source of the port, we'd need to update the phone module to use the right namespace. It's a bit confusing when a module has 26 odd ports, many of which seem to have diverged from the original. I certainly have the skills to maintain a port from googles java code repository, it's just a question of is that the best approach for drupal and this phone module. There is definitely no viable alternative out there, and I'm not sure what impact this might have on 8.x with the inclusion of a 'telephone' module. But D8 is a long way away for me.
Comment #17
cdale CreditAttribution: cdale commentedWhile were here, I always seem to have a hard time deciding where to put setting... field, instance, widget... I've put things that affect the schema or data on the field, like allowed values so we can test if removing a value with cause loss of data, or the length of phone and extension fields in the database. I at one point even had the enable options there, but didn't like how I could not dynamically turn on or off the extension and comment display if I tied it in with the schema. The rest really made sense as widget settings, as they purely affect the widget, nothing else. Wasn't sure if some of them should be instance settings though.
Any ideas or thoughts? Does what I've done work for people? Also, I should point out that the phone_numer FAPI element will work with the libphonenumber stuff just fine on it's own. Won't do any formatting, but it will handle validation, which could be useful to centralise all phone number fields across drupal, including in modules like webform.
Comment #18
ckngGood work!
Have not tested the code, just reading it, looks like a good start, we do need upgrade path and feeds/migrate.
Comment #19
cweagansI think the runtime check would be okay. That'll show an error on every page - I think that's sufficient.
The more I think about it, the more I think we should maintain a new library based on libphonenumber that's fully PSR-0 compliant. This is something that we can submit to Packagist and other projects can use and improve. I hate to break away from libphonenumber, but it wasn't poorly maintained when I opened this issue in October. Having a well maintained library is something that I think will make this project valuable to other systems (and Drupal 8 in the future). I don't think it'd be appropriate to store that library in the same repository as the Phone module, though. In the interest of making a library that other people can use, I'm very much in favor of a nice, PSR-0 library sitting in a Github repo somewhere that other people can use (without Drupal). This also opens the door for the possibility of core inclusion in Drupal 9.
I'd be happy to take a crack at fixing the CSS and Javascript if it's something that you'd rather not deal with - just let me know. Ditto with Feeds and Migrate.
As far as the settings and stuff? I'm not really sure. To be honest, I haven't yet installed the module to play with the functionality, so I'm not sure how it works yet. I will try to get to that at some point tomorrow and I'll update here if I have any further ideas about that.
Seriously, great work on this. If I have time tomorrow, I might take a crack at putting together that PSR-0 version of libphonenumber.
Comment #20
cdale CreditAttribution: cdale commentedI've made some updates.
- I've added a hard dependency on libphonenumber, removing all the fallback code. The module can't even be enabled without it.
- Converted the formatting to be handled completely by a render array. The output is simple enough, and yet such that you can style however you want, that there is no need for theme functions.
- Converted to use Phone instead of "Phone Number"
- Added a very rough feeds and migration implementation. I have not tested either of these, and basically just ripped off the link module, or any others I could find that have those features implemented.
- Various other little touch ups and fixes.
- Update the CSS and JS. Feedback welcome here. I was guessing for the most part as to what exactly needed to be looked at.
As far as I can tell, the only things remaining, are the issue of forking/not forking/where do I point the libraries vendor and download URL's, and the upgrade path. If you're going to take a crack at re-writing libphonenumber, then the first issue is sorted at least. :)
Is the upgrade path going to be from both cck_phone and phone? I can see how the data "maps" as it were, but in terms of finding the right fields, working out which one goes where etc... I don't really want to dig into that at the moment.
Comment #21
cdale CreditAttribution: cdale commentedFew other updates:
- Backported the input type=tel element from D8. Implemented as phone_tel.
- Added devel_generate integration. I used Acquia sales office contact numbers. Do you think they'll mind?
Comment #22
cdale CreditAttribution: cdale commentedI've just committed RDF support for the foaf:phone property, and maybe support for micro-formats hcard, though I have no idea if what I've done is correct for either of these, but hopefully it's a nice start. Any feedback from someone more knowing on these things would probably be a great help. Diff for this commit is at http://drupalcode.org/sandbox/cdale/1925578.git/blobdiff/03342c5b3c04f40...
So, apart from the vendor/ 3rd-party code issue, and upgrade path, I think I'm done with this. What's the next step from here?
Comment #23
cdale CreditAttribution: cdale commentedAlso check http://drupalcode.org/sandbox/cdale/1925578.git/blobdiff/2945c7fa2a573ab... as I just committed that to make the RDF mapping attach automatically to the field. Again, not sure about this one. Thoughts?
Sorry for all the comment spam. :)
Comment #24
slcp CreditAttribution: slcp commented@cdale Brave, you're a star!
Comment #25
cdale CreditAttribution: cdale commentedI was looking into possible update paths, is there no way for a field to change it's database schema in D7? I came across this issue #937442: Field type modules cannot maintain their field schema (field type schema change C(R)UD is needed), and am not sure how to proceed.
Has any one done this? Have they progromatically created a new field, bought the data across, then deleted the old field to re-create it with the new schema, then port the data across once more and finally delete the old field? If that's how it has to be done, then fine, but I'm having trouble finding information on what others have done here.
Comment #26
cdale CreditAttribution: cdale commentedDoes something like the below seem acceptable? This is rough idea, not actual code to be used.
Comment #27
cweagansWell, it's a little more complicated than that, since we're not only upgrading phone-7.x-1.x installs to the 2.x branch, but we're also intending to merge cck_phone into this one. I think we should probably just look at providing some Migrate classes and let people do their own migrations -- hook_update_n isn't a good choice here, IMO.
Comment #28
ckngHave the same feeling, upgrade path probably should use migrate, instead of hook_update_n().
Another thing for this version, try not to add additional features yet, those can come later when a branch has been created, to reduce the testing/review scope to the 'core' features.
Looks great though (still trying to find time to test)
Comment #29
cweagansI have done a final pass over the code in your sandbox, cdale, and I think this is ready to be the development branch. I've gone ahead and merged it in. I've also ensured that you retain commit credit for all the hard work you did on this. Thank you so much!
I've also opened some followup issues for things that need to happen before we can have a final release:
#1928376: Convert libphonenumber-for-php to PSR-0 library
#1928382: Rename "comment" field
#1928420: Upgrade path
#1928422: Tests
If you guys think of anything else, please open followup issues. Thanks!!
Comment #30
cweagansComment #31
cdale CreditAttribution: cdale commentedShort summary of what I believe is in the 2.x branch, for testing etc...
NB: Only feature currently not available from the phone 1.x branch and cck_phone modules that I'm aware of is the ability to disable country validation from cck_phone. I don't expect this to be a problem using libphonenumber for validation.
Comment #32
Nephele CreditAttribution: Nephele commentedWell... I'd been chipping away in my free time at writing a new phone module, then came here ready to post what I've got, only to find out that a lot happened in the last three days. So I have a separate version of a new module, with a full set of upgrade paths, as well as bunch of other features that (based on a quick review of what's here) have not yet been incorporated into the new code. So now I'll go back and break down my code into patches that can be applied to the new branch.
Or would there be any value to me quickly posting my whole version into a sandbox so that others can grab some ideas from what I've got right away? Especially given how quickly things seem to have been evolving here....
To provide quick feedback on some of the general questions that have come up here.
I'd been thinking that some fallback to work without libphonenumber would be useful. One concern is adding an absolute dependency on a library that contains multiple known bugs and doesn't have the best maintenance record. The second concern is that requiring users to install a library might discourage some existing phone and cck_phone users from upgrading to the new branch -- meaning that it will take longer to consolidate all users into a single version of the code. I'm guessing a lot of users don't need much validation; if users are looking for something relatively simple, they might not want the complication of an external library.
I think at least in the short term we have to maintain a separate fork of libphonenumber. In terms of licensing issues, as well as the points brought up by cdale, I think it's best for libphonenumber to remain as a separate library, outside of drupal. However, the version owned by davideme has at least three significant bugs, making it unusable as is. The only clean way to provide users with a patched version is to have separate fork. I don't like the idea of having multiple forks of the library, but that seems to be the approach favored by github.
Comment #33
cweagansAh, crap! I'm sorry about that! However, I'm sure that there are bits that we can pull in pretty easily. I'd love it if you could post your code in a sandbox and create patches for the parts you'd like to see incorporated.
I thought about this, and I am really -1 to this idea. While I can understand the reasoning, the entire point of adopting libphonenumber is so that we're using some standardized library instead of something specific to Drupal. I don't think that asking users to install a library is going to be much discouragement - it's very common for a module to require external libraries.
I do see your point. Right now, we're using cdale's fork of libphonenumber, but this is a short term solution. Something that I feel is really critical before a 2.0 release of this module is #1928376: Convert libphonenumber-for-php to PSR-0 library. Basically, we're going to ditch the official libphonenumber and write our own library based on that code. This way, we're in full control of what code this module depends on and can maintain it responsibly (I'd also like to get Drupal core using this new library, so that's another reason to make it high quality).
Comment #34
Nephele CreditAttribution: Nephele commentedOK, I've put my code into a sandbox at http://drupal.org/sandbox/nephele/1928666. And I'll try to stop kicking myself for taking the weekend off to do non-computer stuff....
Feel free to adapt anything I've got to put into cdale's code. In the meantime, I think I need to start by trying to get a sense of what cdale and I have done differently, which may require addressing some nitpicky details such as the schema, etc. It's not worth adapting migration functions, etc. before the schema and available settings have been worked out.
Comment #35
cdale CreditAttribution: cdale commented@Nephele, again, sorry for stepping on your toes here. I just kind of got on a roll.
Things I can see right away that your code has that mine does not, or that is different/might cause problems in regards to migrate code:
- Support for the content_migrate module.
- Your feeds implementation provides an "all as one field" option, however it does not seem to support the type?
- Your use 'type' for the number type, I originally used 'comment', however there is a patch at #1928382: Rename "comment" field that currently looks at changing this to numbertype.
- You add indexes in your schema. Probably should have done that.
- You have an upgrade path for phone 7.1.x
- Your token implementation looks a lot cleaner and easier to grok than mine, though I based my code off what was in the name module. I like your implementation much better here.
- You allow adjusting the position of the country name and type in the formatter, as well as customising the extension prefix, where as I just rely on libphonenumber to format the extension.
- I like the labels in your commented out hcard_phone_types() list better than mine
- Your schema seems to suggest that multiple types are supported per number. Is this the case? It didn't seem to be implemented. If it is, this is also a nice to have difference I think.
- You have a locater module included to set country based on IP. My question on this, is what if the country detected is not in the allowed countries list?
- You have cck_convert support
- You have migrate support (or have started) for other versions of the phone module.
This is what I can see from scanning the code. There might be some items I've missed. Hopefully it provides a starting point.
Comment #36
Nephele CreditAttribution: Nephele commentedThanks for the nice summary of differences. I see you also noticed my separate issue at #1928688: Coding details in the 7.x-2.x branch and duplicated your list there, so I think I'll follow up in the other issue instead of making this already-very-long issue even longer.
Comment #37
Nephele CreditAttribution: Nephele commentedNot to beat a dead horse, but I just wanted to mention one other issue that's been brought up (e.g., #1925892: Php 5.2 support): the libphonenumber requirement makes the phone module be PHP-5.3-only. Although drupal 7 recommends PHP 5.3, it only requires 5.2.5. I would prefer to make the module require libphonenumber (it's definitely less work), but I'm concerned about the implications for all the existing users. Nevertheless, perhaps it's best to wait until we have a useful module and see what type of feedback we get at that point.
Comment #38
cdale CreditAttribution: cdale commentedThe only thing forcing 5.3 is the namespace requirements. If we were to remove all namespace and use calls from the libphonenumber code, and do a s/libphonenumber\\///g on includes/phone.libphonenumber.inc, then I think it would run under 5.2. I've not tested that theory though. From what I could tell it was only the namespacing stuff that forced 5.3
Comment #39
cweagansI'm fine with requiring 5.3. Most shared hosts support it these days, and there are a lot more 5.3 installations than there are 5.2.x. It's much more important to me to have a modern library that other PHP projects can use. The PHP world seems to be moving firmly in the direction of PSR-0 libraries and composer, so we need to support that.
Comment #40
johnvI'm following this thread with interest. Our provider runs on 5.2.x :-(
Another provider only moved to 5.3 a few months ago.
Comment #41
cweagansI'm sorry that you're in that situation, but we really can't provide 5.2.x compatibility for the 2.x branch. It goes totally against the entire goal of the rewrite. While I can understand where you're coming from, the solution, in my opinion, is to 1) get your provider to upgrade to 5.3, 2) move to a different provider, or 3) Use either phone-7.x-1.x or cck_phone.
Out of curiosity, which provider is that?
Comment #42
johnvThose are Dutch providers, which might be smaller then the average American one.
I guess it will be the case for more smaller countries .
Even with non-shared hosts, or semi-privately shared hosts, I is a huge (perceived) operation/risk to upgrade to 5.3, so they'll likely add new customers/installation on a 5.3 basis, and leave existing ones on 5.2.
I the upgrading cae I've mentioned, i got spammed with mails urging me to test. I didn't.. And got lucky!
Comment #43
David Stosik CreditAttribution: David Stosik commentedHello,
I see this issue marked as "fixed", but I still don't understand what does this changes for me, "mere user".
Should I simply switch to Phone module, 2.x branch, for each new project that needs phone number fields and validation?
More complex question (but just because it may be a concern for some people): will an upgrade path be provided from both old modules to the new one?
Thanks,
David
Comment #44
ckngFixed as in code has been committed into 7.x-2.x branch, where development will happen.
It is still a work in progress, upgrade path is planned but incomplete at the moment. So it is safe to use either module stable version. Once it is done, you can safely upgrade to 7.x-2.x.
Comment #45
David Stosik CreditAttribution: David Stosik commentedThank you. So I guess cck_phone development will slowly fade out to keep only Phone module moving. :)
Comment #46
cweagansIdeally, yes. But it is our intent to provide an upgrade path for your data.