Skip to content

Support Laravel 12 and Future Version #67

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

matriphe
Copy link

@matriphe matriphe commented Apr 6, 2025

This PR removes the limitation to support future Laravel version.

This will also address issue #66.

@matriphe matriphe marked this pull request as ready for review April 6, 2025 08:43
@farzanahmad
Copy link

Can someone please merge this PR?

@3m1n3nc3
Copy link

3m1n3nc3 commented Jun 9, 2025

@farzanahmad For now you can add the repository from this pull request as a vcs in your composer.json

    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/matriphe/laravel-ipinfo"
        }
    ],

Then install the package with composer require ipinfo/ipinfolaravel:dev-laravel-future-version

@max-ipinfo
Copy link
Contributor

Thank you @matriphe @farzanahmad @3m1n3nc3 for the PR and your comments.

I'm not a Laravel user so I don't know whether it's best practice to openly support future versions. Can you point me at best practices / examples for Laravel libraries and versioning?

I don't have the full historical context on our library either, but I do know that we typically go though a manual test of every Laravel version before we state that we support it in our package. That may the reason why we added this version range constraint.

I'll start a conversation internally and will get back to you about the next steps.

@matriphe
Copy link
Author

In Laravel, the illuminate/* versions usually folllow Laravel's version because they are subtree. for example, in Laravel 12, the laravel/framework dependency will have version ^12.0 and the illuminate/* will point to the same version.

it also usually has different version for the require-dev part, especially on the phpunit/phpunit, but I think this is a minor part and will not prevent to upgrade.

because this library only uses illuminate/support to build the Facade, I think it's pretty safe to always follow the Laravel version. the only thing that might break is the PHP version, but I think you need to upgrade the PHP version to satisfy the Laravel requirement anyway.

honestly, we don't really need this library, because I can still use the ipinfo/ipinfo directly, but having access to the Facade from illuminate/support in Laravel is a nice feature. that's why I use this library on my Laravel projects.

Testing

I see there is a test part in here, but I don't think it does something. it would be beneficial if we have an automated test using GitHub Actions to make sure it's compatible with the Laravel version. for testing, we can use Orchestral for this, since it doesn't need to load all the Laravel application.

I can also help with setting up the GHA and the test, but the test scenario might a little bit tricky. ideally, we can have a kind of "integration test", where it makes a call to a "test server" using a special access token, that will always return consistent data, then the test will verify the result to make sure it gets all the data from IPInfo correctly by comparing it. otherwise, we only rely on a mocked data in the unit test.

but this testing part is out of this discussion.

@matriphe
Copy link
Author

we can also make the requirements to illuminate/support: * to not care about the Laravel version, but I think it's not a good thing. if it breaks, we will not know on which Laravel version that breaks. this will also make us follow the Laravel development, and maintain the library and not forget about this.

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.

4 participants