WIP: Laravel upgrade 5.8 #760
Conversation
|
Thank you for the contribution! Are these the only changes needed for laravel 5.8? |
| @@ -0,0 +1,9 @@ | |||
| Activity/ | |||
There was a problem hiding this comment.
Okay, I will remove this and modified compose.json before committing finally because these are our internal companies module. I will adjust it.
There was a problem hiding this comment.
yeah no worries, I guess as much 😄
For your ease, it might be easier to have a separate asgardcms installation only for the contributions.
That way you don't risk mixing both together by accident.
| @@ -1,6 +1,7 @@ | |||
| /node_modules | |||
| /public/hot | |||
| /public/storage | |||
| /public/modules/* | |||
There was a problem hiding this comment.
Yes, again when we publish module, it just copied file from the module assets to public, we dont need I guess. Take a final call and I will remove it if there is intention to copy assets from modules to public.
phpunit.xml
Outdated
| @@ -12,7 +12,7 @@ | |||
| <testsuite name="Core"> | |||
| <directory>./Modules/Core/Tests/</directory> | |||
| </testsuite> | |||
| <testsuite name="Dashboard"> | |||
| <!-- <testsuite name="Dashboard"> | |||
There was a problem hiding this comment.
I am refactoring each of them and uncommenting manually to make it compatible for 3.8 testbench. Mostly setUp and tearDown are protected and return void strictly.
There was a problem hiding this comment.
All are done and file is reverted to its original form
|
@nWidart - that changes in Core and Menu composer.json file managed to resolve dependency and php artisan version ran successfully. However, I am passing each of module's Test case now. |
|
@nWidart - We are getting empty array from translatable in Setting test suite of phpunit, can you please help over in looking into it? The strange thing we are getting proper output in Tag and other modules using translatable package. |
…h module failing testcase due to translatable package issue
|
@nWidart - Another issue needs your attention. Let me know if you need to create them as seperate issue? |
|
That's good point. Will setup it like that.
…On Sun, 2 Aug 2020, 18:41 Nicolas Widart, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In Modules/.gitignore
<#760 (comment)>:
> @@ -0,0 +1,9 @@
+Activity/
yeah no worries, I guess as much 😄
For your ease, it might be easier to have a separate asgardcms
installation only for the contributions.
That way you don't risk mixing both together by accident.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#760 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOAXQJMU6KGUBRES2BE7IDR6VQYBANCNFSM4PSPIGTQ>
.
|
|
@nWidart , have you got time to check the failing tests? Also, been using 5.8 since a week now for improving Rarv for some new functionality, haven't found any issue at runtime, so not sure why its failing... |
|
@dakshhmehta , try changing the file: .travis.yml to Laravel 5.8. |
…tDirSize calculation. It depends on the filesystem. This commit will solve: AsgardCms#465 and the pull request to upgrade to Laravel 5.8 AsgardCms#760
…tDirSize calculation. It depends on the filesystem. This commit will solve: AsgardCms/Platform#465 and the pull request to upgrade to Laravel 5.8 AsgardCms/Platform#760
| "laravel/tinker": "^1.0", | ||
| "nwidart/laravel-modules": "^4.0", |
There was a problem hiding this comment.
This needs to be here for auto-discovery to work. Otherwise, you have to manually load the service provider and facade for Laravel Modules in the app config.
"nwidart/laravel-modules": "^5.0",
There was a problem hiding this comment.
Thanks, I guess I removed it unintentionally.
There was a problem hiding this comment.
I have reverted to 4.0, 5.0 seems to be incompatible with other module or am I missing anything?
|
It looks like we'll have to wait for floatingpointsoftware/stylist#45 or maintain our own fork. I tried to pull this PR down and test it, but I can't get Stylist to provide the backend theme. Every view/partial that the backend needs is giving me an error about not being found. I haven't even tried the frontend yet. |
|
Can you share a screenshot I am already using 5.8 version in my dev
machine. Might be able to help you resolve it.
…On Fri, Sep 4, 2020 at 3:53 AM Micheal Mand ***@***.***> wrote:
It looks like we'll have to wait for floatingpointsoftware/stylist#45
<floatingpointsoftware/stylist#45> or maintain
our own fork.
I tried to pull this PR down and test it, but I can't get Stylist to
provide the backend theme. Every view/partial that the backend needs is
giving me an error about not being found. I haven't even tried the frontend
yet.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#760 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOAXQNRRI7CXICN5XW7P4DSEAJN7ANCNFSM4PSPIGTQ>
.
--
Best Regards,
*Daksh Mehta*
http://dakshhmehta.com
[email protected]
+91-9545438367
Skype: dakshhmehta
|
|
I've already gone through this with a couple of the other partials ( All of the views I'm having an error about are in |
|
Im not understanding. How come it is working for me here? Also, I was just
wondering, do you think it is possible to start a new project with Laravel
8 maybe, and pull over each of the modules one by one rather than going to
convert to 5.8 to 6 to 7 to 8?
Any comments from the community on this?
…On Sat, Sep 5, 2020 at 12:05 AM Micheal Mand ***@***.***> wrote:
@dakshhmehta <https://github.com/dakshhmehta>:
[image: Screen Shot 2020-09-04 at 11 53 35 AM]
<https://user-images.githubusercontent.com/745184/92274540-ad71f880-eeaa-11ea-9af7-cd738ecdc4e6.png>
I've already gone through this with a couple of the other partials (
partials.notifications, partials.top-nav). As soon as I add one view to
resources/views/partials, another one pops up an error.
All of the views I'm having an error about are in Themes/Adminlte/views.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#760 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOAXQIFNUOYFZIQSIMW32TSEEXONANCNFSM4PSPIGTQ>
.
--
Best Regards,
*Daksh Mehta*
http://dakshhmehta.com
[email protected]
+91-9545438367
Skype: dakshhmehta
|
|
What does your It would be quite a bit of work to start with a new Laravel base, I think. It might be a good idea though, to get the CMS up to current Laravel versions. The problem is some of the module's required packages. Some of them haven't been updated in two or more years and would need to be replaced by something new. |
|
That needs a fork and guys like us to maintain it I guess because we love
AsgardCMS!
…On Sat, Sep 5, 2020 at 12:26 AM Micheal Mand ***@***.***> wrote:
What does your composer.json look like? I think the problem is I ran composer
install and it replaced the modules from this PR with the main v4 ones...
Not sure why, when they were already "installed".
It would be quite a bit of work to start with a new Laravel base, I think.
It might be a good idea though, to get the CMS up to current Laravel
versions. The problem is some of the module's required packages. Some of
them haven't been updated in two or more years and would need to be
replaced by something new.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#760 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOAXQLVSF7AKNKYFJE4V7DSEEZ6DANCNFSM4PSPIGTQ>
.
--
Best Regards,
*Daksh Mehta*
http://dakshhmehta.com
[email protected]
+91-9545438367
Skype: dakshhmehta
|
|
Right now the platform is only compatible with PHP 7.1.x. I just send a PR to fix travis-ci build and all the tests are passing for 7.1.x but not >= 7.2 Maybe that's your problem @mikemand |
|
JFYI, I managed to run it on 7.4 without any runtime issue, except 1 change
to replace array_key_exists to isset() method in one of the core laravel
package, which normally thrown on first php artisan serve
…On Tue, Sep 8, 2020 at 3:21 AM Imagina Colombia ***@***.***> wrote:
Right now the platform is only compatible with PHP 7.1.x.
I just send a PR to fix travis-ci build and all the tests are passing for
7.1.x but not >= 7.2
Maybe that's your problem @mikemand <https://github.com/mikemand>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#760 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOAXQMGHPJHH3QTJSFIZ4TSEVIVRANCNFSM4PSPIGTQ>
.
--
Best Regards,
*Daksh Mehta*
http://dakshhmehta.com
[email protected]
+91-9545438367
Skype: dakshhmehta
|
|
Hello @dakshhmehta , yesterday i sent a new PR solving the testing issues for PHP 7.2,7.3 and 7.4 Can you rebase your PR with that changes to check if your PR pass the travis builds? |
|
Sure, will do tomorrow. Thanks!
…On Fri, Sep 11, 2020 at 8:35 PM Imagina Colombia ***@***.***> wrote:
Hello @dakshhmehta <https://github.com/dakshhmehta> , yesterday i sent a
new PR solving the testing issues for PHP 7.2,7.3 and 7.4 Can you rebase
your PR with that changes to check if your PR pass the travis builds?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#760 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOAXQNDE5UIF7L4IKRE5L3SFI4FNANCNFSM4PSPIGTQ>
.
--
Best Regards,
*Daksh Mehta*
http://dakshhmehta.com
[email protected]
+91-9545438367
Skype: dakshhmehta
|
|
@imagina - I want to rebase to your branch, what is your branch and repository? can you share the branch name please? Edit: I got it. I just pull master, sorry :( |
@nWidart - I need to bump the version 4.0 for make the Menu module in master (idavoll/menu-module) compatible, shall I send the merge request there or what is the right approach? |
|
@nWidart , after merging 5.8 compatibility of @imagina , I get following error in the fresh install after cloning master branch. Seems composer.lock needs to be part of repository or it has never been before? |
|
I'll close this ticket as the other PRs of upgrades are ready to be merged. Lets see what errors come up after the laravel 8 merge. |
Changes to bump the laravel version to 5.8.
We do have failing tests for following modules:
Setting, Menu and Media
If anyone got an idea on failing test case for above module, please comment.
but despite of having the failing tests, php artisan serve runs the website smoothly. I have tested core modules including Setting, Menu, Page - front-end flow, Login, Forgot Password etc...
Regards,
Daksh