-
Notifications
You must be signed in to change notification settings - Fork 3
Allow willdurand/geocoder 5.0 for PHP 8.4+ #4
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
Conversation
|
@oscarotero thanks for tagging me. I think I removed my notifications by mistake. @steffans in reality this will be applied for PHP >=8.2, not for 8.4 only |
|
@steffans @oscarotero just enabled the CI and seems like the JSON is wrong lol . A comma is missing at the end of the line |
|
It allows to install geocoder 5.0 which brings PHP 8.4 compatibility, otherwise this middleware remains incompatible with the latest PHP version. |
|
@steffans that's not true but it's needed The constraint for v4 is php: ^7.4 || ^8.0 (so technically it supports all php8 versions) or do you mean that it actually breaks? |
|
@oscarotero @steffans so my thoughts are these: For those users who installed the geocoder lib as a side effect of installing this middleware, their code will break if they use any PHP 8 version because installing this library made them install the v4 of the geocoder library, and unless they manually installed the geocoder library (gain control of its version), when they upgrade to the next release, similarly, the geocoder lib will be upgraded to v5 thus making this a breaking change. And given that probably most users installed the v4 as a side effect, if they have customised code for the geocoder lib, it will break. So I think the best is to create a new major version of this middleware (v4). @oscarotero share thoughts. |
|
Looking at the changelog, seems that there are no breaking changes, only a deprecated method and drop support for old PHP versions. So maybe a major version is not necessary. |
|
@oscarotero I guess deprecating that method is a Breaking change already? |
|
@filisko The geocoder 5.0 release fixes PHP 8.4: Implicitly nullable parameter declarations deprecated messages |
|
@filisko if the method is not removed, I wouldn't consider it a breaking change. It's like when PHP deprecates some functions between minor versions, and remove them after a major version. In my opinion, a minor version is okay, but I leave the final decision to you, that you have more context. If you think a major version is better, I'm okay with that too. |
|
@oscarotero all right, makes sense if it's only the notice! Thanks! Will release it today ASAP |
|
@steffans thanks! |
|
@oscarotero @steffans ah, one thing that I thought about why a major is needed is to rename the package dependency to |
|
I'm a bit confused about existing two geocoder packages? |
|
@oscarotero I mean with this one https://github.com/geocoder-php/Geocoder If you go to the current package https://packagist.org/packages/willdurand/geocoder It says "READONLY" 🤔 |
|
@filisko No idea. |
No description provided.