-
Notifications
You must be signed in to change notification settings - Fork 468
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
libphonenumber v9 - Feedback requested for BC changes #676
Comments
Thank you for the ping. I'll give feedback shortly. |
Great news, @giggsey!
Agree. 8.1+ is the way to go.
Following your earlier comment, I've actually been playing with adding types to everything private & internal in this branch. I can pursue with adding types everywhere else if you wish.
Adding types is indeed a BC break (for properties and return values, not for method parameters, due to the LSP), but a major version is here to break things. And this one is easy to fix. Note that adding
Converting to enums is a good idea IMO. As you said it doesn't affect comparison: $phoneNumberUtil->getNumberType() === PhoneNumberType::MOBILE; // will still work But indeed, using the actual int value for other usages (like storing in a database) will require code adjustments.
+1 for phpstan! |
Thanks for your investment in this project! I agree to push forward with all mentioned topics. My take on the Enums part:
I think now is a great opportunity to leverage additional language features. In the end people know that a major version bump will require some code changes. I think it's up to us as package maintainers to just smoothen the path for end users, for which I see two possible scenarios:
|
Hi @giggsey! Thanks for your amazing job and thanks (a lot!) to take care of your users (us)! 💚 I won't be original: I also agree with everything you've proposed and we'll do our best to deal with those changes on our side. :) ping @jderusse @maxhelias @Nek- @OskarStark @Taluu who may be interested as co-maintainers. |
Do you plan to change the file tree (location of metadata) with the new version? In the Debian package, I have added a script to update only the metadata without having to update the code. I know this is suboptimal, but given that a stable version of Debian is shippied every 2 years, having a script to do update just the metadata can be useful. Next release of Debian (trixie) is due in a few months, with a freeze starting Apr/15. We would have 2 choices for libphonenumber-for-php:
Maybe the major version upgrade could be an opportunity to rethink the way to manage updates of metadata only (and keep the same code/API)? For example with one package containing the code/API, and one for the metadata which can be updated when the user wants to update it through a script? By the way if you plan to update version of phpunit, we have a patch to support phpunit 11 |
libphonenumber v9 was released earlier today.
The reason for the bump on Google's side was due to bumping the minimum version of Java supported. The public API has remained the same.
This version bump gives this library the opportunity to make some backwards incompatible changes.
However, I'm keen to avoid anything too onerous for users, so I'm suggesting (for this library and libphonenumber-for-php-lite):
Min PHP Version
Potentially bumping the min PHP version to support supported PHP versions only (currently 8.1 and above).
Packagist suggests ~15% of installs are using unsupported PHP versions.
Types
@BenMorel has been doing some work on this recently, but we should go further now we have the BC ability.
Adding types to add methods and variables, and setting strict_types on every package file.
This could cause issues for anyone extending the classes.
Marking classes as
@internal
and other attributesAnother example:
@no-named-arguments
Enums
Convert the const classes (e.g. PhoneNumberFormat) to enums.
I'm less sure about this one. If people are accessing the constants, it shouldn't impact them. But if they are using the underlying ints, they would need to change code to convert that back to the enum.
Testing
Revamp the testing framework to match libphonenumber-for-php-lite, adding PHPStan etc. Whilst this doesn't impact BC, the PHP version support gives the opportunity for this to happen.
Metadata changes
Investigate whether the metadata could be stored in the files as direct objects instead of arrays. Check if this is any faster than the arrays. It could open up to using classes, and autoloading the metadata instead of loading it via require.
Feedback is welcome. As a result of this discussion, and the necessary changes, the 9.0.0 release will be a few weeks late.
/cc @odolbeau @Propaganistas @simonschaufi as maintainers of the active linked framework integrations
The text was updated successfully, but these errors were encountered: