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

[Upsun] Upsun ify a Laravel app should invite users to add the Laravel bridge #199

Closed
thomasdiluccio opened this issue Jan 30, 2024 · 3 comments · Fixed by #202
Closed
Assignees

Comments

@thomasdiluccio
Copy link
Contributor

upsun ify applied on a new Laravel app makes no mention of platformsh/laravel-bridge. It seems those lines are not hit:

var suggest = "\nPlease use composer to add the Laravel Bridge to your project:\n"
var composerRequire = "\n composer require platformsh/laravel-bridge\n"

How to reproduce

composer create-project laravel/laravel new-laravel-app

cd new-laravel-app

upsun ify

image

@gilzow
Copy link
Contributor

gilzow commented Jan 31, 2024

can confirm. brand new laravel project, platformsh/bridge is not a listed composer dependency. running upsun ify does not prompt me to add the laravel bridge.

@gilzow
Copy link
Contributor

gilzow commented Jan 31, 2024

this appears to be an issue with appRoot at line 31 of laravel.go

What's odd is that ApplicationRoot.Ask determines the correct value:

ApplicationRoot.Ask called!
working directory is: /Users/gilzow/projects/delete/flyio-test-larave
path.dir is: 0x1098c00
Composer path is: /Users/gilzow/projects/delete/flyio-test-larave/composer.json
determined appRoot in application_root.go is: .

so we've determined the correct location at this stage. However, once we create input via answers.ToUserInput() in platformify.go at line 66, input.ApplicationRoot is set back to /:

app root after creating input: /

ah-ha! found it!

@akalipetis in answer.go, when converting the answers into a UserInput object (?), the return of filepath.Join() when a.ApplicationRoot is a . results in a return of / instead of ./ or just ..

Seems like what we want at this point is to have appRoot be a relative directory, correct? Shouldn't we prepend the ApplicationRoot property with a . at line 117? ie

ApplicationRoot:    "." + filepath.Join(string(os.PathSeparator), a.ApplicationRoot),

When doing that, everything works:

working directory is: /Users/gilzow/projects/delete/flyio-test-larave
path.dir is: 0x1098c00
Composer path is: /Users/gilzow/projects/delete/flyio-test-larave/composer.json
determined appRoot in application_root.go is: .
inside of ToUserInput method of Answers
The current value of ApplicationRoot from the answers variable is: . 
The testPath when using Join: ./ 
app root after creating input: ./
application root is: ./
appPath: .
jsonPaths: [composer.json]

Please use composer to add the Laravel Bridge to your project:

    composer require platformsh/laravel-bridge

and even if i move composer.json into a subdirectory:

working directory is: /Users/gilzow/projects/delete/flyio-test-larave
path.dir is: 0x1098c00
Composer path is: /Users/gilzow/projects/delete/flyio-test-larave/foo/composer.json
determined appRoot in application_root.go is: foo

inside of ToUserInput method of Answers
The current value of ApplicationRoot from the answers variable is: foo 
The testPath when using Join: ./foo 
app root after creating input: ./foo
application root is: ./foo
appPath: foo
jsonPaths: [foo/composer.json]

Please use composer to add the Laravel Bridge to your project:

    composer require platformsh/laravel-bridge

@akalipetis akalipetis self-assigned this Feb 1, 2024
akalipetis added a commit that referenced this issue Feb 1, 2024
akalipetis added a commit that referenced this issue Feb 1, 2024
akalipetis added a commit that referenced this issue Feb 1, 2024
@akalipetis
Copy link
Member

Thanks a lot @gilzow for the help - the problem was indeed there, but the root cause was in the way we handle root directories in the .Find() command, which affected both Laravel and Django.

It should be fixed in #202

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 a pull request may close this issue.

3 participants