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 Laravel support #2249

Merged
merged 11 commits into from May 29, 2020
Merged

Add Laravel support #2249

merged 11 commits into from May 29, 2020

Conversation

NBZ4live
Copy link
Contributor

@NBZ4live NBZ4live commented May 13, 2020

The Problem/Issue/Bug:

There is currently no official Laravel support in form of a project-type.
https://github.com/drud/ddev-contrib/tree/master/recipes/laravel suggests to use the generic php project type, but it's Nginx config is more Drupal inspired than generic (for example the try_files $uri $uri/ /index.php?q=$uri&$args;)

How this PR Solves The Problem:

This PR does add the laravel project type and official Laravel config for Nginx.
Additionally, in the postStartAction, it does check if the the ddev db container is configured in the .env and prints some helpful instructions how to do it with a one-liner.

Manual Testing Instructions:

How I tested:

  1. Create a new Laravel app same as I described in the quickstart docs:
mkdir my-laravel-app
cd my-laravel-app
ddev config --project-type=laravel --docroot=public --create-docroot
ddev start
ddev composer create --prefer-dist laravel/laravel
ddev exec "cat .env.example | sed  -E 's/DB_(HOST|DATABASE|USERNAME|PASSWORD)=(.*)/DB_\1=db/g' > .env"
ddev exec "php artisan key:generate"
ddev launch
  1. Clone any working Laravel demo app from GitHub. (I used the Backpack demo):
git clone git@github.com:Laravel-Backpack/demo.git laravel-demo
cd laravel-demo
ddev config --project-type=laravel
ddev composer install
ddev exec "cat .env.example | sed  -E 's/DB_(HOST|DATABASE|USERNAME|PASSWORD)=(.*)/DB_\1=db/g' > .env"
ddev exec "php artisan key:generate"
ddev exec "php artisan migrate"
ddev exec "php artisan db:seed"
ddev launch

Automated Testing Overview:

Will add some demo app to ddevapp_test.go later. Will probably need to write one, because using some random demo app from GitHub for this purpose does feel wrong.

Related Issue Link(s):

#898, #2084

Release/Deployment notes:

No. This should not affect any other project types

PS: I did add a wrapper for artisan (Laravel's cli), but decided to do this in a separate branch, because it's first time I do something in Go and I need to study the codebase a bit more to be able to make a test for it. (And at the moment I have no idea how to test the wrapper for a command which does exist only inside a Laravel project codebase)

@CLAassistant
Copy link

CLAassistant commented May 13, 2020

CLA assistant check
All committers have signed the CLA.

@rfay
Copy link
Member

rfay commented May 13, 2020

Yay!

@rfay rfay force-pushed the feature/laravel-support branch from 4039f9e to e339ca4 Compare May 14, 2020 01:25
@rfay
Copy link
Member

rfay commented May 14, 2020

@NBZ4live I took the liberty of rebasing this, so you'll need to update your local branch against the github fork. Hope that's OK. But I know you were concerned about the broken Windows container test.

I also pushed a branch-specific version of ddev-webserver and added that into version.go, so you should be able to use your built ddev as is.

@rfay
Copy link
Member

rfay commented May 14, 2020

Looks like you're right on track here.

The key thing that needs to be added is a set of data to test against, as in https://github.com/drud/ddev/blob/f6e43e080196e5ade4f1e8f878d3bf3645878457/pkg/ddevapp/ddevapp_test.go#L41-L166

@NBZ4live
Copy link
Contributor Author

NBZ4live commented May 14, 2020

Yes, I know. As I wrote in the PR, I will need to write an app because I think that taking something random from GitHub doesn't feel to be right.

But I will be able to do this only on the weekend.

@rfay
Copy link
Member

rfay commented May 14, 2020

Ah, thanks. I didn't understand the context of your comment about "writing an app", had thought you were going to do more go test writing than was necessary. However, just simple codebase built with ddev composer create laravel/laravel probably wouldn't do any harm. But that wouldn't give much opportunity for actual test URLs, so I think your plan is better. Thanks!

There's no rush, this will take some time to get in anyway.

@NBZ4live
Copy link
Contributor Author

This would give test urls, but would not give a proper dbimport test

@rfay
Copy link
Member

rfay commented May 15, 2020

Yes, agreed. It's best to have real URLs that depend on a matching database.

@rfay
Copy link
Member

rfay commented May 23, 2020

I'll be back to review this hopefully in the next week. It will also have to get moved probably to a new repo (drud/ddev-images), but I can take care of that.

@rfay rfay force-pushed the feature/laravel-support branch from 968215e to 4a47493 Compare May 27, 2020 19:55
Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked this over and took a pass through at manual testing and everything worked great, including apptype detection and everything else.

I rebased this and pushed the latest version of the web image.

Thanks so much for a great contribution, and looking forward to having this in the next alpha release.

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NBZ4live there may be a problem with the nginx config. When going through the backpack demo I happened into the pages (/admin/page) and all of them are 404, even if I edit them and stuff. Is that a problem with the nginx config?

(Note that I rebased again, so you'll need to reset to the branch). I had been hoping to pull this, but want you to check that situation first.)

@rfay rfay force-pushed the feature/laravel-support branch from 4a47493 to 3367c74 Compare May 28, 2020 03:14
@rfay
Copy link
Member

rfay commented May 28, 2020

To be more specific, here's the pages admin at http://laravel-backpack-demo.ddev.site/admin/page:

Pages____Backpack_Admin_Panel

If I click where the arrow is, it takes me to http://laravel-backpack-demo.ddev.site/mollitia, which is a 404.

If I go to News->Articles, I'm able to preview the items there, but don't actually know how to see if they're really published or if the published URL works.

@rfay
Copy link
Member

rfay commented May 29, 2020

@NBZ4live confirmed in slack that the behavior here is correct.

@rfay rfay merged commit ef23672 into ddev:master May 29, 2020
@rfay rfay mentioned this pull request Jun 29, 2020
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

3 participants