I have a repository for this new library set up at https://github.com/cweagans/phonenumber-php

This library needs to be completely standalone. We'll eventually submit it to Packagist, and if this happens soon, we'll try to move some of the validation, storage, and parsing logic into core. We also need to set up that library with some tests and have it run through Travis.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Nephele’s picture

Are you proposing that a new module be written from scratch, completely independent from davideme/libphonenumber-for-PHP? Although davideme's code contains some bugs, it does have a ton of features, and it is open-source. Reproducing everything davideme has written (and doing without simply stealing his code) will take a lot of work. It seems to make more sense to use his code as a starting point, openly acknowledge and credit his work, then revise the code as necessary.

cdale’s picture

I think using davideme's code as a base was the intent, and then converting it to be PSR-0. Unless cweagans had a different idea? This was the impression I got however.

cweagans’s picture

The idea is to take all of the fixes from the entire network of libphonenumber-for-PHP forks and incorporate them into one library. The library must be a properly namespaced PSR-0 library. So most of this is just going to be code reorganization, but the really big bonus is that we'll have versioned releases and actual maintenance on the code (since at least the 20,000 or so users of phone.module + cck_phone.module will be using it (hopefully)).

I'm hoping that we can just make libphonenumber-for-PHP go away and people will use our library instead, but we'll see how that goes.

To answer your question, yes. It will largely be based on libphonenumber-for-PHP, just properly organized and better maintained.

Nephele’s picture

Ah, OK, I'm just a bit overwhelmed by new information right now.

I've also got a fork of the libphonenumber-for-PHP library, at https://github.com/Nephele/libphonenumber-for-PHP, and I just pushed the latest changes that I'd been working on today.

I've updated all the metadata files. Plus at this point I have my own set of bug fixes, separate from those in davideme's pull requests. So specifically:

  • I have a fix for the extension-parsing bug
  • I did some minor cleanup to the unsetting-US-country-codes fix
  • I did a complete revamp of the linebreaks-in-number-patterns fix, moving the key functionality to PhoneMetadata.php so that it can be easily applied to all number patterns that may have extraneous linebreaks and spaces. I haven't provided any details of this latest fix yet on github, though

On the other hand, I haven't changed the library's namespace in any way - although it's a pretty easy global search-and-replace that I could do if desired.

So which of the many available forks should we put in place as a starting point? It might be best to establish that sooner rather than later, given how quickly we've already been able to diverge.

The next question I'm wondering is, what changes (if any) are we going to make that affect the library's API? It would help if we could establish and stabilize the class names, functions, etc. pretty quickly, so that any other libphonenumber improvements can be done without having to continually sync library changes with phone-7.x-2.x. Other than simplifying the library's namespace, I can't think offhand of any other API-type changes that I've noticed.

Finally, I definitely agree that we need to introduce some type of version number. However, there's the additional complication that libphonenumber will (ideally) need to be updated everytime there's a new version of the PhoneNumberMetadata.xml file. Do we simply treat those updates as another +.1 version increment? Or is there some other way to differentiate metadata updates from "true" code updates?

cdale’s picture

I actually forked my repo from https://github.com/practo/libphonenumber-for-php. This was the guy who made the namespace changes. I only chose his because it seemed to be more up to date, and fixed a few issues. Not the extension parsing one however. I didn't even notice the namespace change until much much later.

I personally don't think we need to bump a version number everytime the metadata is updated. Minor number maybe? And I think phone should then require a given major number.

The only time I see a need to bump the major number, is when a country is removed or added, as I added in some countries that drupal core is not aware of using hook_countries_alter(), and it also required modifying of some code files in libphonenumber to make it aware of them so they are available to phone by updating it's country mapping list.

So it's only really when a country is added or removed that we really need to do anything. The rest is mainly rules updates that if people want the latest and greatest, update, if not, then don't.

Thinking about it, it would even be possible to have the metadata outside of the core class, or something that the class can generate at runtime. The only reason to not do this, is the metadata currently creates PHP code. It wouldn't be much to move that to a database storage engine however, instead of the flat file format currently in use. This could allow a lot more flexibility, and simplify releases for metadata updates.

I agree that a naming structure needs to be established ASAP.

cweagans’s picture

I'd like to do the rewrite of the library at https://github.com/cweagans/phonenumber-php. It's named differently to be a clean break from libphonenumber (though it will be very similar).

My biggest issue is that it does not conform to PSR-0. Conforming to PSR-0 will increase the chances of being accepted by core (which is a very good thing, in my book). That conversion will require a few namespace and class name changes (though I haven't yet had time to enumerate all of those changes). If either of you guys wants to work on this, I'll leave it up to your best judgement as to which fork to work from initially (though I'd suggest cdale's, as we know it works with what's in git right now).

I'm okay with not bumping the version for metadata updates and just including the new metadata with the next tagged release. People that want bleeding edge can work straight off of the develop branch. I'd like to keep the metadata sort of how it is right now (i.e. not moving it into the database - just automatically generated PHP files) to keep things simple, though in the future, we could introduce different ways to store metadata (disk storage, db storage, memcache storage, etc).

Nephele’s picture

So, in the hopes of getting one set of forks consolidated, I've reviewed the various changes to libphonenumber.

The cdale/chipperstudios fork does look like a better starting point -- it's had more bug fixes, and is a few steps closer to PSR-0. It has the best fix for the linebreaks-in-number-patterns bug. The unsetting-US-country-codes bug is tackled differently, but I think adding another layer of code protection would be prudent. Plus, the extension-parsing bug is not fixed. So I'm attaching a patch that incorporates those fixes (instead of doing yet another fork at github...).

From the commit history of the various forks, the only set of changes made to davideme's fork that aren't part of the chipperstudios fork are some extra tests that were added to PhoneNumberUtilTest. I've added a second patch summarizing those changes.

At that point, the only pull request on the davideme fork that is not covered is #10, to fix comment blocks. I've put that into a third patch, rolled against the chipperstudios fork.

With those patches, I think we'll then have a single fork incorporating everything that's been posted so far on github. I'd only consider the bug_fixes patch a priority, but the other two can't hurt.

cweagans’s picture

This sounds like a cdale thing. I don't have commit access to his repo :)

cdale’s picture

I've committed the tests and comments patch.

What does the extension fix do? I tried after patching to supply a phone number with the extension included, such as 5555555;ext=5555, but I just got a error that the number was too long to be a phone number. Is the patch supposed to fix this?

Nephele’s picture

That sounds like the type of error that the extension bug will cause -- and if I'm reading your post correctly, you have not yet applied the bug_fixes patch, and therefore should be getting errors.

The initialization functions are being called in the wrong order, so the extension-matching patterns are wrong and will fail to recognize nearly any extension (see here).

For example, if the demo.php is changed to use $swissNumberStr = "044 668 18 00;ext=555" and run with the unpatched code, the output is clearly wrong (e.g., nationalNumber = float(4.46681800399E+14)). After applying the bug_fixes patch, the demo.php output is correct -- the extension is set correctly, the number is valid, e

cdale’s picture

The patch to libphonenumber is good and working. :) I was testing via the phone module, and I have some code which worked around this which was causing me errors. I'll commit the patch shortly. It looks good. Creating a new patch for phone first though.

Nephele’s picture

Good! So I think that's goal #1 taken care of ("take all of the fixes from the entire network of libphonenumber-for-PHP forks and incorporate them into one library").

PSR-0 is all new to me, but from what I can understand, it looks like this fork of libphonenumber conforms to PSR-0 (unlike davideme's original). Unless we need to change the namespace to "phonenumber" or "phonenumber-php" (do namespaces allow dashes?) instead of "libphonenumber"? Or is there more needed that I'm overlooking?

The one other requirement is to put a version number somewhere. In README.md? PhoneNumberUtil.php? In a new file?

Beyond that, I can see a lot of improvements that could be made to libphonenumber, but I'm not sure how many of them must be made right away -- i.e., the changes will have repercussions for the phone module.

cdale’s picture

One thing I did notice, and I'm not sure if it's right or wrong, but the E164 output format will never output the extension, because it returns before that part is run. Is this good or bad? right or wrong? I'm not sure. Something to point out though. :)

Nephele’s picture

I'd forgotten about that quirk, and I'm equally unsure whether it is intentional or accidental. However, looking at the code in PhoneNumberUtil::format(), I'm inclined to think it's a mistake. In particular, there's a comment for E164 stating "Extensions are not formatted" suggesting that extensions are supposed to be included. I'd vote for changing the code to add extensions for E164.

Which reminds me of another minor extension-related feature that would benefit the phone module: providing an easy way to customize the prefix used for an extension (" x ", " ext. ", ";ext="). It's currently only possible by altering const DEFAULT_EXTN_PREFIX = " ext. ";. Adding another parameter that the format() function passes to maybeAppendFormattedExtension() would be a simple enough fix, at least for that one function.

cdale’s picture

+1 to fixing both of those.

Is this you? https://github.com/Nephele If so, I'll add you as a collaborator, and then you should be able to commit to that repo. Or we can just work with pull requests. Either or.

Nephele’s picture

Yep, that's me. Being able to commit to your repo should be easier all around than having duplicate forks, so that would be great!

cdale’s picture

Done. You're now a contributor! I don't think there's anything else I need to do....

cweagans’s picture

PSR-0 is all new to me, but from what I can understand, it looks like this fork of libphonenumber conforms to PSR-0

Not quite. The directory structure isn't quite correct. See https://github.com/guzzle/guzzle for a good example of directory structure and how the namespaces are set up. Also, I had a thought that might simplify namespacing: what do you guys think about calling the external library "Phony" ? Easy namespace (and we don't have to figure out where to put the dash/underscore), and as far as I can tell, there's no existing PHP library called Phony. I'm open to other names too. The more I think about it, the more I don't want it to be called "phonenumber-php".

cdale’s picture

Don't want phony phone numbers? Use Phony... I like it, and can't currently think of a better alternative. Are there any concerns with changing the name though given we're basing this off libphonenumber?

cweagans’s picture

Like I said, we'll have to change the namespaces and fully qualified class names. I'm definitely liking the clean break from libphonenumber. This way, there won't be any confusion that, while we're based on libphonenumber, we're a separate library.

cweagans’s picture

Rad. I renamed the repo: https://github.com/cweagans/Phony

Nephele’s picture

It is short, memorable, and gets rid of that hyphen. It doesn't seem that google places restrictions on names of ports (their port list includes "python-phonenumbers", for example). A search on "Phony library" doesn't show any relevant hits on the first few pages. My only hesitation is that it's, well, a bit cheesy, but I suppose that's not really a problem ;)

Nephele’s picture

I figured I'd tackle adding extensions for E164 numbers (going back to #13), in part to test my access, so I went ahead, made the change and committed it. Hopefully that wasn't too bold of me....

An unexpected minor complication was that maybeAppendFormattedExtension() required the country $metadata as a parameter. To make it possible to pass a NULL $metadata I had to remove the typing hint -- it didn't seem to be necessary for a private function to have an argument type restriction.

cdale’s picture

Not too bold at all. That's the whole point of collaboration. Also, VCS makes rolling back very easy in my experience.

Having not looked too deeply into the code apart from finding useful functions, I can't really comment. If it all seems to work I'm sure it'll be fine. If some has problems I'm sure they'll post an issue.

Nephele’s picture

Sounds like we're on the same page :)

Nephele’s picture

I'd been conveniently forgetting this whole time that we're working on a PHP port -- meaning that there's a whole mess of parent code over at google, too. With multiple versions, code updates, etc.

What we have is a port of google's version 4.5 (specifically the java code), released Jan 19 2012. Google has since had 25 releases, and is now at 5.3.2. Most of the releases have just been metadata changes, but 9 do contain code changes (a couple new functions, some new obscure country-specific exceptions, etc.). Google does roll the metadata changes in with the rest of the code, and does a version update each time (used to be +0.1, now looks like +0.0.1).

I'm going through their svn repository getting details on the updates, with the aim being to then port the changes into our library. The rate of changes seems to be slowing down, but continuing to sync up our port with google periodically probably is useful.

The bug fixes that have been made to the PHP port so far appear to all be PHP-specific bugs.

Google has not incorporated any changes to the extension-formatting functions -- i.e., they have not made any changes comparable to what we proposed in #13/#14. My impression is that it's because extensions aren't really a priority for them. There are no extension-specific issues in their queue, and only a half-dozen issues that even mention extensions.

Issues that come to mind:
- I don't think we want our version numbers to be tied to google's version numbers, but our documentation should probably specify what google version we've updated to.
- Our documentation should probably also describe our metadata version using the google version number.
- Although I think we want to not deviate too far from google, that should not prevent us from making changes that we consider necessary. In the case of the extension-formatting tweaks (#13/#14), the phone module is otherwise forced to work around the problem and duplicate libphonenumber's functionality. I think that's sufficient justification for altering libphonenumber.
- Ideally we should probably make requests in google's issue queue to get these changes done in their code too, but it's not something I want to tackle.

Nephele’s picture

Our code has now been synced with the latest google version (5.3.2). The most notable change as far as the phone module is concerned is that RFC3966 numbers are now printed with a "tel:" prefix.

I also found some more java-to-PHP errors and fixed them. The main effect of these errors was that is was impossible to get the preferred domestic carrier code.

cdale’s picture

I notice google has now pushed to 5.4 :) I've updated the metadata again. Not sure if any other changes are actually needed.

Has there been any move on PSR-0 stuff? Be really nice to get the 2.x branch kicking along.

cweagans’s picture

I haven't had time to work on it, no. Hopefully, I'll have time between the 21st and the end of the month. My availability in those dates is much expanded from what it is now, so I'm going to try to make some decent headway.

pfrenssen’s picture

Issue summary: View changes

In the meanwhile a lot has happened in the libphonenumber-for-php world. There is a new fork which has quickly gained the status of being the de-facto libphonenumber-for-php implementation: https://github.com/giggsey/libphonenumber-for-php/

It is PSR-0 compliant, actively maintained and has become very popular on Packagist / composer.

Considering that this new fork addresses the major points above, and as we also have the new Proudly Invented Elsewhere motto in core development I do not think it would be appropriate to maintain our own fork any more.

I am currently experimenting with the new library in cdale's sandbox.