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

Laravel 10 support #5753

Open
wants to merge 9 commits into
base: 1.6
Choose a base branch
from
Open

Laravel 10 support #5753

wants to merge 9 commits into from

Conversation

jf-m
Copy link

@jf-m jf-m commented Apr 3, 2023

This PR includes the last bits to make Voyager Laravel 10 compatible (work started there #5732 ).

Here's a breakdown of what I did:

This PR passes all the phpunit tests up until PHP 8.2 on my fork (https://github.com/jf-m/voyager/actions/runs/4600528910).

Any comments welcome, hope this helps !

@mdenitti
Copy link

mdenitti commented Apr 6, 2023

This looks dodgy...anyone tested successfuly?

@jf-m
Copy link
Author

jf-m commented Apr 6, 2023

@mdenitti can you clarify what is that you find "dodgy" ? Like stated before, you can add comments if there's something wrong from your point of view.

I must say that my auto-indent might have removed some spaces in the composer.json and phpunit.xml. Maybe this is what confuses you ?

I can add some more explanations about how I solved the issue at the first place.

The initial problem from the current state of the branch 1.6-l10 can be seen here: https://github.com/the-control-group/voyager/actions/runs/4080000210/jobs/7498342456?pr=5732.

ErrorException: require_once(/home/runner/work/voyager/voyager/vendor/orchestra/testbench-core/laravel/vendor/autoload.php): Failed to open stream: No such file or directory

It came from the following line of code:

https://github.com/the-control-group/voyager/blob/a74198baf70f26ca6aa02723aea39a03cc45b043/src/Commands/InstallCommand.php#L146

This require_once, to my knowledge, is here for compatibility reasons, so Voyager can be compatible for both Laravel < 7 and Laravel 8+. Indeed, since Laravel 8 and above, the seeds folder is called seeders and the namespace Database\Seeders; is mandatory on the seeders.

Right now, Voyager Seeder system is based on Laravel <7 with seeds and no namespace. Voyager in the InstallCommand check the Laravel version, if 8+ then it will rename the seeds to seeders and it will manually add the namespace to all the seeders files here :
https://github.com/the-control-group/voyager/blob/a74198baf70f26ca6aa02723aea39a03cc45b043/src/Commands/InstallCommand.php#L165-L181

In my PR, I removed all this, because Voyager is no more compatible for Laravel <7. I renamed seeds to seeders and I added all the needed namespaces.
By doing so, the require_once base_path('vendor/autoload.php'); is no more necessary because the seeders file have not changed (the Namespace is already there). This solved the main issue.

Cleaning up UsersTableSeeder.php
@jf-m
Copy link
Author

jf-m commented Apr 6, 2023

@mdenitti I added a comment on each major changes for more clarity, to help the reviewer. Hope this helps. 👍

@mdenitti
Copy link

@mdenitti I added a comment on each major changes for more clarity, to help the reviewer. Hope this helps. 👍

Jean! Wonderfull this looks much better; Genius... Merging still blocked???? We need L10 support now :)))

@dmatis2
Copy link

dmatis2 commented May 10, 2023

Is it worth downgrading somehow to L9 from L10? Or there will be support soon? 😊

@hazbu
Copy link

hazbu commented May 11, 2023

try this, worked for me on laravel 10
composer require tcg/voyager dev-1.6-l10

thank later

@mdenitti
Copy link

composer require tcg/voyager dev-1.6-l10

Found 1 security vulnerability advisory affecting 1 package.
Run composer audit for a full list of advisories.

Installation failed, reverting ./composer.json and ./composer.lock to their original content.

@octoxan
Copy link

octoxan commented Jun 20, 2023

How is this taking so long to get merged? It works!

@JonathanVorich
Copy link

It looks like the-control-group do not intend to maintain the project any further. They have had no reaction since February, when I first mentioned the issue with Laravel 10. And also some links in the documentation no longer work. I think this repository will no longer be supported. Sad.

@haydar
Copy link

haydar commented Aug 2, 2023

@marktopper, @marktopper Hello, I hope both of you are alive. Could you please pay some attention here?

@sajaddp
Copy link

sajaddp commented Aug 26, 2023

Is this package no longer updated to support new versions of Laravel?

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.

None yet

9 participants