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

fix: resolve Laravel home using __DIR__ #130

Closed
wants to merge 5 commits into from
Closed

Conversation

jaulz
Copy link

@jaulz jaulz commented Sep 22, 2023

When using a mono repository, the Laravel folder is not the root folder. Hence, we cannot simply take the Lambda Root and expect Laravel files to be present. Instead, this PR uses the current directory of the script and just finds the Laravel files relatively via going up the directory tree.

@@ -7,6 +7,8 @@
use Bref\LaravelBridge\StorageDirectories;

Bref::beforeStartup(static function () {
$laravelHome = __DIR__ . '/../../../../';
Copy link
Member

Choose a reason for hiding this comment

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

This is way to ambitious. Use a constant or add a custom ENV variable instead.

Copy link
Member

Choose a reason for hiding this comment

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

I kinda prefer this over more configuration options tbh...

Copy link
Author

Choose a reason for hiding this comment

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

What do you mean by "Use a constant"? This at the top?

define('LARAVEL_HOME',  __DIR__ . '/../../../../');

@@ -7,6 +7,8 @@
use Bref\LaravelBridge\StorageDirectories;

Bref::beforeStartup(static function () {
$laravelHome = $_ENV['LARAVEL_HOME'] ?? __DIR__ . '/../../../../';
Copy link
Member

Choose a reason for hiding this comment

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

Please default to LAMBA_ROOT_TASK.

Copy link
Author

Choose a reason for hiding this comment

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

done 😊

@deleugpn
Copy link
Member

I just opened #131 as an alternative to this and explained why I preferred the previous approach.

Copy link
Member

@deleugpn deleugpn left a comment

Choose a reason for hiding this comment

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

I'm tagging this with request changes so that we can better discuss the alternative approach on #131 before merging. If we still opt for this option, I'd like to get the HandlerResolver also implemented to solve loading namespaced Handlers with custom vendor.

@@ -7,6 +7,8 @@
use Bref\LaravelBridge\StorageDirectories;

Bref::beforeStartup(static function () {
$laravelHome = $_ENV['LAMBA_ROOT_TASK'] ?? __DIR__ . '/../../../../';
Copy link
Member

Choose a reason for hiding this comment

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

This will never work because LAMBDA_ROOT_TASK is always defined so the __DIR__ alternative here will never be executed, which will make it dead code

Copy link
Author

Choose a reason for hiding this comment

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

Ah, I thought it's just the preferred name of the variable 😉 I renamed it temporarily and wait for #131 instead then.

@deleugpn
Copy link
Member

deleugpn commented Mar 3, 2024

#131 has been merged and should address this, so I'm going to close this one.

If the author, or anybody else, feels differently, please feel free to open an issue or another PR for us to discuss it further!

@deleugpn deleugpn closed this Mar 3, 2024
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.

3 participants