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

Utf8 encode #1225

Merged
merged 2 commits into from
May 18, 2024
Merged

Utf8 encode #1225

merged 2 commits into from
May 18, 2024

Conversation

kornrunner
Copy link
Contributor

Trying to avoid utf8_encode deprecation warning on PHP 8.2+

It will try to do the same using mbstring or iconv functions depending on available extension, and eventually fallback to utf8_encode (not to break BC?).

It's a bit of duplicated code - but I didn't think this specific case (used in two providers only) would warrant moving it to parent class.

@jbelien
Copy link
Member

jbelien commented May 16, 2024

Thanks a lot ; I indeed miss utf8_encode() was deprecated!
To be honest, I think we could fully rely on mbstring only and replace utf8_encode() by mb_convert_encoding(). We "just" need to add mbstring as required extension in the composer.json file for those 2 providers (and create a new major version).

@kornrunner kornrunner force-pushed the utf8_encode branch 2 times, most recently from 368807a to ef240ed Compare May 16, 2024 15:06
@kornrunner
Copy link
Contributor Author

Sure thing, that works as well (and is simpler). I wasn't sure what's the policy on BC changes.

PR is updated as suggested. Thank you!

@jbelien
Copy link
Member

jbelien commented May 17, 2024

Thanks @kornrunner 👍
Don't forget to make sure all checks are passing.

@kornrunner
Copy link
Contributor Author

kornrunner commented May 17, 2024

Both failing checks (phpstan and php cs fixer) seem to be regarding code that hasn't been changed in this PR (GoogleMaps/BingMaps and GeoIP2).

I can submit another PR regarding those - as I don't think they address current issue?

EDIT: submitted #1226

@norkunas
Copy link
Member

We "just" need to add mbstring as required extension in the composer.json file for those 2 providers (and create a new major version).

Another option would be to require symfony/polyfill-mbstring to avoid breaks for apps that doesn't have mbstring installed

@kornrunner
Copy link
Contributor Author

Changed requirement from mbstring extension to symfony/polyfill-mbstring

@kornrunner kornrunner force-pushed the utf8_encode branch 2 times, most recently from 9e0434a to e325817 Compare May 17, 2024 08:04
@jbelien
Copy link
Member

jbelien commented May 18, 2024

As soon you've merged #1226 and rebased this PR, I'll approve it! Thanks a lot! 👍

@kornrunner
Copy link
Contributor Author

Sure thing, just - I have no privileges to merge it. Thanks!

@jbelien
Copy link
Member

jbelien commented May 18, 2024

Ah that's strange ... I've just done it 👌

@kornrunner
Copy link
Contributor Author

It's rebased, thank you!

@jbelien jbelien merged commit b502473 into geocoder-php:master May 18, 2024
155 checks passed
@kornrunner kornrunner deleted the utf8_encode branch May 18, 2024 20:13
@kornrunner
Copy link
Contributor Author

Weirdly - it seems this change hasn't propagated to https://github.com/geocoder-php/ip2location-binary-provider ?

@jbelien
Copy link
Member

jbelien commented May 19, 2024

Indeed! I'll investigate.

@jbelien
Copy link
Member

jbelien commented May 19, 2024

IP2Location is indeed not in the list : https://github.com/geocoder-php/Geocoder/actions/runs/9142343555

There is probably a reason. I'll check and add it if it's an oversight.

@jbelien jbelien mentioned this pull request May 25, 2024
jbelien added a commit that referenced this pull request May 25, 2024
@jbelien
Copy link
Member

jbelien commented May 25, 2024

🔖 IP2LocationBinary version 1.3.1: https://packagist.org/packages/geocoder-php/ip2location-binary-provider#1.3.1
🔖 MaxMindBinary version 4.3.1: https://packagist.org/packages/geocoder-php/maxmind-binary-provider#4.3.1

@kornrunner
Copy link
Contributor Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants