-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
added laravel 9 support #1297
added laravel 9 support #1297
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some combinations of PHP / Laravel are not valid in the test matrix, can you please fix them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're not yet there, the matrix isn't correctly generated yet, see my inline feedback.
Another thing: L9 is not yet available for the integration tests, see https://github.com/barryvdh/laravel-ide-helper/runs/4802033128?check_suite_focus=true#step:4:13
So until this works, we can't merge and probably have to wait until the actual release.
laravel: [8.*] | ||
laravel: [8.*, 9.*] | ||
exclude: | ||
- php: [7.4, 7.3] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The matrix still generates jobs for PHP 7.4 and Laravel 9, see https://github.com/barryvdh/laravel-ide-helper/actions/runs/1692091626
Maybe the […, …]
is not supported and you've to manually list each?
- php 7.3
laravel: 9.*
- php 7.4
laravel: 9.*
Just a guess though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok i've updated matrix to suggested syntax.. sorry it's my first time with github actions syntax
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
np!
yeah it can be a PITA, also since you're first time contributor, we've to manually enabled running the workflows everytime 😅 (I just did, let's wait for the result)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're one step further, next problem: orchestra/benchmark also needs to have adapted version constraints, see https://github.com/barryvdh/laravel-ide-helper/runs/4803289512?check_suite_focus=true#step:5:20
I see they have not yet made a new 7.* release at https://github.com/orchestral/testbench-core but their branch is also updated. Maybe we can use dev-master
there or we need to wait.
FTR: If you feel overwhelmed / frustrated, don't be: I can take it from here later if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mfn if it's not a problem please take it from here because i don't wan't to break something... thx for the support! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's wait for the 9.x release to run the ingration tests.
@barryvdh nice, thx; seems to work already Now we've to wait for the composer laravel installer to switch to L9 I guess? |
Yeah let's skip the integration test for Laravel 9 for now, but do just the unit tests. |
The tests run :) |
@barryvdh then it's safe to merge? :) |
Yeah if @mfn approves |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work all, thanks! 🙏🏼
@barryvdh can you add this to the changelog? I can't push unfortunately:
- Added Laravel 9 support [#1297 / rcerljenko](https://github.com/barryvdh/laravel-ide-helper/pull/1297)
### Added - Add support for custom casts that using `Castable` [barryvdh#1287 / binotaliu](barryvdh#1287) - Added Laravel 9 support [barryvdh#1297 / rcerljenko](barryvdh#1297)
### Added - Add support for custom casts that using `Castable` [#1287 / binotaliu](barryvdh/laravel-ide-helper#1287) - Added Laravel 9 support [#1297 / rcerljenko](barryvdh/laravel-ide-helper#1297)
### Added - Add support for custom casts that using `Castable` [#1287 / binotaliu](barryvdh/laravel-ide-helper#1287) - Added Laravel 9 support [#1297 / rcerljenko](barryvdh/laravel-ide-helper#1297)
### Added - Add support for custom casts that using `Castable` [#1287 / binotaliu](barryvdh/laravel-ide-helper#1287) - Added Laravel 9 support [#1297 / rcerljenko](barryvdh/laravel-ide-helper#1297)
### Added - Add support for custom casts that using `Castable` [#1287 / binotaliu](barryvdh/laravel-ide-helper#1287) - Added Laravel 9 support [#1297 / rcerljenko](barryvdh/laravel-ide-helper#1297)
* added laravel 9 support * added exclude directives to matrix * fixed matrix * Update composer.json * Update run-integration-tests.yml * Update CHANGELOG.md Co-authored-by: Ricardo Čerljenko <[email protected]> Co-authored-by: Barry vd. Heuvel <[email protected]>
### Added - Add support for custom casts that using `Castable` [barryvdh#1287 / binotaliu](barryvdh#1287) - Added Laravel 9 support [barryvdh#1297 / rcerljenko](barryvdh#1297)
Summary
composer.json
and tests