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

Add support for Laravel 11 #1510

Closed
wants to merge 5 commits into from

Conversation

KentarouTakeda
Copy link
Contributor

Summary

This pull request resolves #1509.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Misc. change (internal, infrastructure, maintenance, etc.)

Checklist

  • Existing tests have been adapted and/or new tests have been added
  • Add a CHANGELOG.md entry
  • Update the README.md
  • Code style has been fixed via composer fix-style

Overview of responses to each issue

  • PHPUnit version upgrade

    I have expanded the version range accordingly. There was no corresponding breaking change in PHPUnit 10, so there is no need to modify the test code.

  • Support for Doctrine DBAL removal

    I have migrated the minimal necessary code for the Laravel IDE Helper from the implementation existing in Laravel 10, which was removed in Laravel 11.

    Ideally, it should be implemented in a newly created Laravel API with the removal of Doctrine DBAL, but the version with that modification would be laravel/framework > 10 only. We'll need to support <=10 for a while, so this is the way to go for now.

  • Migration method changes for floating-point types

    I have removed the corresponding migration method from the test. Only $table->unsignedDecimal() was applicable.

    I don't think there is any need to worry about narrowing the scope of the test.

    • The migration method itself is not the target of the test; it is simply used to create the test table.
    • This feature has already been deprecated and has been removed in the latest version.
    • Tests where the database-side floating-point type is converted to string are covered by other tests such as $table->decimal().
  • Adding methods to Authenticatable contract

    I have implemented a new method in the Authenticatable contract. Like any other request method, it is simply declared and empty.

Test Results

  • In my repository from which this pull request is based, I have confirmed that all existing tests pass with newer versions including php-8.3 and Laravel 11. CI results can be seen from here.
  • CI testing is done with SQLite only. I set up PostgreSQL and MySQL in my local environment and confirmed that the User class (users table and its migration) in the initial state of Laravel produced the same execution results as before.
    • SQL Server was not confirmed as it was difficult to set up.

I'm looking forward to continuing to use the great tools you've created in Laravel 11. Please let us know if you have any feedback regarding this pull request. Thank you for confirmation.

Regards.

@hafezdivandari
Copy link
Contributor

Why not remove doctrine/dbal in favor of native schema methods? You are using Doctrine DBAL to inspect database tables on ModelsCommand, instead of bringing DoctrineConnection back, you may use Schema::getTables(), Schema::getColumns(), Schema::getForeignKeys(), etc native methods.

Therefore, there's no need to register custom column types and you will have full support for all native column types (e.g. enum, geometry, etc.). please let me know if you have any question on using this methods.

@barryvdh
Copy link
Owner

barryvdh commented Feb 7, 2024

Thanks! Yeah I heard at Laracon that they removed doctrine altogether, and use their own functions. See laravel/framework#48864

And now that I open it again, I see that @hafezdivandari did that PR, so good job :)

But yeah, my thought was also to drop doctrine and use that, so only support Laravel 11.

We released 2.14 yesterday, so I think we could probably skip directly to Laravel 11 for 2.15? Maybe a small maintenance release in the 2.14.x range, but removing doctrine would be a nice change to aim for. Perhaps @hafezdivandari would be open to help with this?

@barryvdh
Copy link
Owner

barryvdh commented Feb 7, 2024

@mfn what do you think about the Laravel 11 only support voor 2.15?

@hafezdivandari
Copy link
Contributor

hafezdivandari commented Feb 7, 2024

Thank you @barryvdh

I added most of the new schema methods on Laravel 10 first (you may check the list of PRs on laravel/framework#48864), then removed Doctrine DBAL on Laravel 11. So the schema methods I mentioned are also available on L10.

I'm ready to help; I think supporting Laravel 11 on laravel-ide-helper would be a good example for other packages.

@mfn
Copy link
Collaborator

mfn commented Feb 7, 2024

Upfront: awesome for the PR! I planned to do it, but that's even better ❤️

what do you think about the Laravel 11 only support voor 2.15?

A though pill to swallow for sure, but seeing there's not much feature change going in ATM, I'm in for it. In the worst case we can make a 2.14 branch.

But TBH: if we make such a clear cut, I would make it in 3.x for ide-helper; that's much easier to reason because there's a lot more code to be dropped and phpunit upgrade and and and.

However: I would wait until official release of Laravel 11 (maybe in the next hours, maybe next days) and then, if @barryvdh agrees, we make a Laravel 11 only 3.x release?

@barryvdh barryvdh mentioned this pull request Feb 7, 2024
@barryvdh
Copy link
Owner

barryvdh commented Feb 7, 2024

Yeah we could wait for L11.
See work in progress here: #1512
Probably missed some things, but we could do a L10.x release if we figure out what the minimum version is.

@barryvdh
Copy link
Owner

barryvdh commented Feb 7, 2024

So if we look at the release schedule, Laravel 9 is not supported since yesterday:
image

And looking at #1512 we can actually still support Laravel 10 (at least the latest minor versions). And it doesn't seem to change that much actually for the user (at least, no test changes required). PHPunit also doesn't impact te users.

I kinda prefer not bumping the major version, albeit just because it makes it easier for users to upgrade to Laravel 11.
Laravel 11 should drop in about 4 weeks, so no real rush. But there is also no reason to keep supporting Laravel 9 or older 10.x releases. But supporting 10.x is a big pro, as long as it's officially supported by Laravel.

@mfn
Copy link
Collaborator

mfn commented Feb 7, 2024

But there is also no reason to keep supporting Laravel 9 or older 10.x releases. But supporting 10.x is a big pro, as long as it's officially supported by Laravel.

Agree on all accounts. It makes sense to me to put in effort for officially supported versions.

Dropping L9 is "okay", we just made a release which still covers it 👍🏼

@barryvdh
Copy link
Owner

I've cherry-picked your commits in #1520
Together with #1512 this replaces this PR

@barryvdh barryvdh closed this Feb 17, 2024
@barryvdh barryvdh mentioned this pull request Feb 17, 2024
6 tasks
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.

Modifications required to support Laravel 11
4 participants