laravel-bridge
laravel-bridge copied to clipboard
brefphp
Allow to run Laravel under custom vendor location
This is basically the same as https://github.com/brefphp/laravel-bridge/pull/130.
There are 2 places right now that breaks when installing Laravel under a monorepo setup without the standard Laravel folder structure - the bref-init.php makes reference to php artisan in the root of the folder and the HandlerResolver container assumes bootstrap/app.php inside /var/task.
The linked PR attempts (at request of @tillkruss) to implement an environment variable. That approach has a few downsides:
- Dynamic configuration options for things that can be resolved statically.
- Use of
LARAVEL_environment prefix outside of Laravel ownership - Forces Bref users to configure Bref to locate something that Bref can locate automatically.
We could still opt to use an environment variable if that's the preferred approach among the maintainers, but such variable should not be under the LARAVEL_ namespace unless Laravel themselves provide the value by default.
hey @tillkruss can you expand a little on why you want to create configurable runtime dynamic variables with a worse user-experience for users instead of implementing a better default value?
hey @tillkruss can you expand a little on why you want to create configurable runtime dynamic variables with a worse user-experience for users instead of implementing a better default value?
@deleugpn: I may simply not understand the use-case and this change looks like a potential breaking-change to me.
Not changing the default behavior and allowing the 1 user who needs this to specify a custom path using an ENV variable seems like the lowest risk to me.
@deleugpn: I may simply not understand the use-case and this change looks like a potential breaking-change to me.
What can I explain that would help you understand the use-case and show that this is not a breaking change?
Not changing the default behavior and allowing the 1 user who needs this to specify a custom path using an ENV variable seems like the lowest risk to me.
I need this for myself, so now we're 2 users. The creation of configuration values increases the internal compleixty of the package as it leads to different execution path depending on how the end user interacts with it. The end user needing to override the default value with a configuration is also added burden. It's lose-lose situation for both users and maintainers.
hey sorry folks, I've been using this branch in production for the last week and I haven't had time to clean this up to make it ready for merge. I haven't forgotten about it, I will get to it as soon as I finish my ongoing project.
hey @mnapoli and @tillkruss, we have tested this out on AWS Lambda and it seems to be working as expected. The only last concern I have is in regards to bref-init.php defining the function resolveBootstrapLocation. Since this is a function definition without namespace, it could potentially clash with userland code defining a function with the same name.
Any suggestions on this?
That's great, I added one comment inline.
Since this is a function definition without namespace, it could potentially clash with userland code defining a function with the same name.
How about namespacing the function? That would solve the problem without any weird thing?
hey @tillkruss and @mnapoli really sorry for how long this has dragged on. I just did a last round of refactoring to get all changes to a central place and address it in a more consistent manner.
The goal is for the LaravelPathFinder::root() method to return the root folder of Laravel, always, without a leading slash. The original implementation was kept for BC purposes.
Whenever LaravelPathFinder::root() is used, we can go ahead and concatenate it with a slash / plus whatever path we're looking for.
I'm deploying this change in my project right now to test it out inside AWS Lambda.
I have not tested this in production yet, but I did a few tests on a staging environment with a real AWS Lambda. Everything is working as expected.
Hey @tillkruss and @mnapoli
I just noticed I've been running this branch on production since December. Looks like I completely forgot to follow up on this.
I just fixed the conflicts that @KentarouTakeda mentioned, but if we have no other objections, this is good to go.