Skip to content
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

Use libphonenumber-for-php-lite instead of libphonenumber-for-php #248

Merged

Conversation

bperel
Copy link
Contributor

@bperel bperel commented Apr 4, 2024

This change allows to use giggsey/libphonenumber-for-php-lite instead of giggsey/libphonenumber-for-php.

giggsey/libphonenumber-for-php-lite is only 1.6MB in comparison with 19MB for giggsey/libphonenumber-for-php, allowing for a much smaller package size.

The main caveat is that only version 8.x of the library can be used, since the lite variant was created based on version 8 of the original library.
It is also not possible to pass null as a phone number with the lite version, hence the test that I removed.

@Propaganistas
Copy link
Owner

Thank you - I wasn't aware of the lite version yet.

I'd rather have this package handle null values properly instead of completely removing support for it. Could you look into that?

@bperel
Copy link
Contributor Author

bperel commented Apr 5, 2024

I'd rather have this package handle null values properly instead of completely removing support for it. Could you look into that?

The thing is that libphonenumber-for-php-lite itself doesn't support null values, contrary to libphonenumber-for-php: see here

So the only alternative solution to not supporting null values anymore would be to handle null values in the PhoneNumber constructor and replace them with something that libphonenumber-for-php-lite will accept (i.e. any non null value), so these lines would become:

    public function __construct(?string $number, $country = [])
    {
        $this->number = is_null($number) ? '': $number;
        $this->countries = Arr::wrap($country);
    }

In this case the it_returns_empty_string_when_null_is_cast_to_string test passes so we don't need to remove it.

Would that be an acceptable approach?

@Propaganistas
Copy link
Owner

Looks great. I tend to preserve it like this because empty form input gets converted to null values. Applications might blow up if the phone helper suddenly doesn't support null anymore.

@bperel
Copy link
Contributor Author

bperel commented Apr 5, 2024

Updated!

@Propaganistas
Copy link
Owner

Adjusting minimum version to 8.13.1 to make the lowest tests succeed (see giggsey/libphonenumber-for-php-lite#9).

@Propaganistas
Copy link
Owner

Propaganistas commented Apr 8, 2024

I'm wondering: what if someone actually listed the original giggsey/libphonenumber-for-php in its project dependencies because some method not available in lite is being used?

lite marks the original as a conflicting dependency, so installation of Laravel-Phone would suddenly fail.

https://github.com/giggsey/libphonenumber-for-php-lite/blob/feef98fb9ee2c7bc6c689a007ad38ccc8569486f/composer.json#L79-L81

I think giggsey/libphonenumber-for-php first needs to define that it replaces (composer.json replace) lite if requested elsewhere as a dependency.

@bperel
Copy link
Contributor Author

bperel commented Apr 8, 2024

I'm wondering: what if someone actually listed the original giggsey/libphonenumber-for-php in its project dependencies because some method not available in lite is being used?

lite marks the original as a conflicting dependency, so installation of Laravel-Phone would suddenly fail.

https://github.com/giggsey/libphonenumber-for-php-lite/blob/feef98fb9ee2c7bc6c689a007ad38ccc8569486f/composer.json#L79-L81

I think giggsey/libphonenumber-for-php first needs to define that it replaces (composer.json replace) lite if requested elsewhere as a dependency.

I don't see any technical solution to that problem apart from indeed using this replace property.
However, I don't think that this situation would occur that frequently though, so maybe writing about it in the release notes would be enough?

@Propaganistas
Copy link
Owner

Created upstream PRs for that:

giggsey/libphonenumber-for-php#625
giggsey/libphonenumber-for-php-lite#53

@giggsey
Copy link
Contributor

giggsey commented Apr 19, 2024

8.13.35 is now released that should cover this for you

@Propaganistas
Copy link
Owner

Thanks for the heads-up! Merging this now.

@Propaganistas Propaganistas merged commit b7461f0 into Propaganistas:master Apr 20, 2024
10 checks passed
@gdebrauwer
Copy link

gdebrauwer commented May 17, 2024

This was a breaking change. I updated to the latest version of this package and suddenly some tests were failing because the code was using features that are not available in the lite version

@Propaganistas
Copy link
Owner

It should not be breaking in any way for functionality provided by the package itself.

Only in case of a self-defined macro you might have introduced additional features. But as soon as you do that, i.e. directly using features of libphonenumber, you should have listed libphonenumber as a direct project dependency because you're relying on that package in your own code. If libphonenumber was listed, everything should keep working properly because lite is configured to be superseded by libphonenumber whenever already present.

@giggsey
Copy link
Contributor

giggsey commented May 17, 2024

@gdebrauwer I'd recommend adding something like Composer Require Checker to your CICD. It tells you if you have any indirect dependencies within your project.

@bperel bperel deleted the use-libphonenumber-for-php-lite branch July 9, 2024 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants