-
Notifications
You must be signed in to change notification settings - Fork 360
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
Laravel 11 Support for Lunar v1 (Clean Install) #1631
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@adam-code-labx Nice work, man! 👍 |
packages/admin/database/migrations/2023_05_31_100000_create_permission_tables.php
Show resolved
Hide resolved
one of the annoying upgrade to v11 is in the migration where was this being considered in this PR? |
Should we not be considering Laravel Shift for this? I'm concerned we may have missed important updates here. |
This is currently a draft PR. I will be working further on this today so will make sure I go through the upgrade guide carefully. @glennjacobs Sure we could try to run Laravel Shift to upgrade to Laravel 11 but I feel this is targeted more for simpler projects, this would certainly suggest using multiple version constraints for different packages. The manual approach imo seems the best option. I am not sure if Shift will even modify the github test workflows? |
Reverted test workflows for now. |
Just updated deps I assume this means shifts must be private 😲 |
Can I suggest as v1 is currently in the early alpha stage migrations are simplified merging all the update/add/remove migrations into the create migrations. Then the new lunar upgrade command would perhaps run upgrade migrations from it's own directory. Then we don't even need to worry at this stage about L11 retain existing attributes. This seems like the best approach for a clean install. |
I will start the upgrade PR next weekend. This should give you guys plenty of time to review this PR 1st. Thanks have a great weekend 👍 |
Both my filament package PR's have now been accepted so we only have 2 dependencies pending L11 update. Here are my suggestions for these 2 remaining packages: For the exchange rates we should use laravel-exchange-rates
Let me know your thoughts on this @glennjacobs, if your ok with my suggestions I will add this to next weekend todos 😉 |
Hi @glennjacobs Any idea when you can review this? |
When all the packages are updated, then we can review this. I think there's one left from what I can see? |
Yep one left but tbh I think we can replace this with our own internal measurement converters. (Cartalyst doesn’t seem to be actively maintained 1 year since there last commit) “cartalyst/converter can be replaced with are own internal converters for each unit of measurement |
That could work, as UOM conversions are never going to change :) |
I will try to find some time this week. |
Might be easier to check a separate PR, thanks. |
Thanks @wychoong cartalyst updated their package now to support L11 I still believe it will be better to create are own implementation, however right now we can at least move forward with this PR. |
"kalnoy/nestedset": "^6.0", | ||
"laravel/framework": "^10.0", | ||
"illuminate/http": "^10.0|^11.0", | ||
"kalnoy/nestedset": "^v6.x-dev", |
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.
This isn't a true release. We will need either 6.0.4
or 7.0.0
tagged.
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.
Unfortunately this was never tagged just updated in their dev branch lazychaser/laravel-nestedset#598 (comment)
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.
They will tag it eventually.
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.
At least we are not using a forked branch anymore. Also as lunar v1 is still in alpha, I would not delay this based on using their dev branch when we can simply provide a patch update once they finally get round to tagging their release 👀
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.
I don't mind doing that as long as we create an issue so we don't forget :)
@glennjacobs How about we trim down this PR and just focus only on the Laravel 11 upgrade so this can be released perhaps with 1.0.0-alpha.9 this week? We can save all migration-related modifications for the Lunar install which I can then go ahead and start a draft PR if this works for you? |
Yes I agree. |
@glennjacobs Sure will get on to this tonight and start the PR for the Lunar install. Thinking we can use Laravel prompts something similar to my new RESTPresenter package 😜 |
@glennjacobs Restoring the migrations resulted in all tests failing. Let's keep this PR targeted only for clean installs, focused on updating workflows and test cases, while also ensuring we have cleaned up the migrations to leave us only with create tables, previous updated migrations have now been merged. The next PR for the lunar-install will handle the upgrade process. However this one might not be straight forward and will certainly take time to get right, this really shouldn't delay L11 support. We just to need to make sure everyone is aware that this is currently for clean installs and updating from previous v1 alpha and not currently supported to be used when upgrading from 0.x. Perhaps as previously suggested this could be released and tagged to another branch. |
@adam-code-labx many thanks for all your efforts. I reviewed the PR and decided it would be quicker for me to start on a clean branch to get this done, taking influence from your updates. The new PR can be found here #1685 The migrations and upgrade command can be discussed on #1382 to determine how we want to move forward. |
This PR gets Lunar v1 up and running with Laravel 11 🚀
I've forked and updated the following repositories:
For Lunar v1 users to benefit from these updates, they will need to add these repositories to their project's composer.json
My next step will be to submit PRs to these repositories, ensuring that everyone can benefit from Laravel 11 compatibility.
Then of course the above repositories will no longer be required.
I've put it through tests on both Laravel 10 and 11 everything passes, updated spatie-media and its enum fill for conversions, tweaked the docs for early opt-in support, and fixed up the spatie permissions migration.
Also adjusted the GitHub workflow to support Laravel 11 testing.