Allow to run Laravel under custom vendor location #131

deleugpn · 2023-09-26T13:25:59Z

This is basically the same as #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.

tillkruss

Don't change defaults. Make it configurable. Please.

deleugpn · 2023-09-26T13:39:10Z

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?

tillkruss · 2023-09-26T16:16:54Z

src/HandlerResolver.php

@@ -72,7 +72,8 @@ private function laravel(): Application
            return $this->laravel;
        }

-        $bootstrapFile = getcwd() . '/bootstrap/app.php';
+        // ./vendor/bref/laravel-bridge/src/
+        $bootstrapFile = __DIR__ . '/../../../../bootstrap/app.php';


Can we use realpath() on both strings?

tillkruss · 2023-09-26T16:20:56Z

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 · 2023-09-26T17:30:53Z

@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.

src/bref-init.php

src/HandlerResolver.php

deleugpn · 2023-10-06T12:08:31Z

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.

deleugpn · 2023-11-09T10:12:06Z

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?

mnapoli · 2023-11-09T10:33:18Z

src/HandlerResolver.php

@@ -96,4 +90,23 @@ private function laravel(): Application

        return $this->laravel;
    }
+
+    private function resolveBootstrapLocation(): string


This function seems to duplicate the other resolveBootstrapLocation() function, but with a slightly different implementation (but same name), any reason for that?

Can we merge both functions into one?

I just addressed this

mnapoli · 2023-11-09T10:34:16Z

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?

deleugpn · 2023-11-13T14:41:00Z

src/bref-init.php

+    }
+
+    // Going up 4 directories will get us from `vendor/brefphp/laravel-bridge/src` to the Laravel root folder
+    return realpath(__DIR__ . '/../../../../');


hey @alustau this is breaking the invocation of php artisan config:cache because realpath() returns the folder without the last /. We need to concatenate / at the end of the string

This inline comment is not marked as "obsolete", is this resolved? I didn't see any new commits related to this, not sure what to do.

I had forgotten to merge the PR that was addressing this 😅

Support PHP 8.3

Add slash in the end of laravel root path and namespacing function

deleugpn · 2023-11-24T11:29:09Z

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.

deleugpn · 2023-11-24T11:44:54Z

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.

KentarouTakeda

It seems that there is a conflict due to my Pull Request #143 that was merged yesterday.

I have confirmed that the following modification works fine.

src/bref-init.php

deleugpn · 2024-02-19T13:26:16Z

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.

deleugpn added 2 commits September 26, 2023 15:13

Allow running with Custom Vendor location

2542004

Allow Bref-init to run with custom vendor

4728576

deleugpn requested review from tillkruss and mnapoli September 26, 2023 13:26

deleugpn mentioned this pull request Sep 26, 2023

fix: resolve Laravel home using __DIR__ #130

Closed

Coding Style

7b9da48

tillkruss requested changes Sep 26, 2023

View reviewed changes

tillkruss reviewed Sep 26, 2023

View reviewed changes

deleugpn added 7 commits September 26, 2023 13:34

use realpath()

d77f507

use realpath()

adaa506

Keep existing behavior unchanged for better BC compatibility

70d36a1

syntax

b0af019

variable

4d3627d

syntax

74f4192

func

e516f0a

tillkruss approved these changes Sep 26, 2023

View reviewed changes

tillkruss reviewed Sep 26, 2023

View reviewed changes

src/bref-init.php Outdated Show resolved Hide resolved

mnapoli reviewed Sep 27, 2023

View reviewed changes

src/HandlerResolver.php Outdated Show resolved Hide resolved

Add comment

d283a70

tillkruss reviewed Sep 29, 2023

View reviewed changes

src/HandlerResolver.php Outdated Show resolved Hide resolved

alustau and others added 4 commits November 6, 2023 14:41

Implement same logic on bref-init

f7a8b08

Merge pull request #135 from alustau/implement-custom-vendor-placement

06438bc

Fix coding standards

cf8341c

Merge pull request #136 from alustau/implement-custom-vendor-placement

e6b346c

tillkruss approved these changes Nov 6, 2023

View reviewed changes

mnapoli reviewed Nov 9, 2023

View reviewed changes

deleugpn commented Nov 13, 2023

View reviewed changes

Add slash in the end of laravel home path when use realpath

ad2c698

mnapoli mentioned this pull request Nov 14, 2023

Add slash in the end of laravel root path and namespacing function #140

Merged

Merge pull request #142 from brefphp/support-php-83

235b2fa

Support PHP 8.3

deleugpn mentioned this pull request Nov 23, 2023

Support PHP 8.3 #141

Merged

deleugpn added 3 commits November 24, 2023 07:41

Merge pull request #140 from alustau/implement-custom-vendor-placement

727e4f0

Add slash in the end of laravel root path and namespacing function

final refactoring this time for real

f604978

add comment about reason for the code (BC)

70976ec

add more realistic log message

6e242be

KentarouTakeda reviewed Dec 7, 2023

View reviewed changes

src/bref-init.php Outdated Show resolved Hide resolved

src/bref-init.php Outdated Show resolved Hide resolved

Merge branch 'master' into custom-vendor-placement

d27c72b

mnapoli approved these changes Mar 3, 2024

View reviewed changes

mnapoli merged commit 9f18f59 into master Mar 3, 2024
12 checks passed

mnapoli deleted the custom-vendor-placement branch March 3, 2024 13:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to run Laravel under custom vendor location #131

Allow to run Laravel under custom vendor location #131

deleugpn commented Sep 26, 2023

tillkruss left a comment •

edited

Loading

deleugpn commented Sep 26, 2023

tillkruss Sep 26, 2023

tillkruss commented Sep 26, 2023

deleugpn commented Sep 26, 2023

deleugpn commented Oct 6, 2023

deleugpn commented Nov 9, 2023

mnapoli Nov 9, 2023

deleugpn Nov 24, 2023

mnapoli commented Nov 9, 2023

deleugpn Nov 13, 2023

alustau Nov 13, 2023

mnapoli Nov 23, 2023

deleugpn Nov 24, 2023

deleugpn commented Nov 24, 2023

deleugpn commented Nov 24, 2023

KentarouTakeda left a comment

deleugpn commented Feb 19, 2024 •

edited

Loading

Allow to run Laravel under custom vendor location #131

Allow to run Laravel under custom vendor location #131

Conversation

deleugpn commented Sep 26, 2023

tillkruss left a comment • edited Loading

Choose a reason for hiding this comment

deleugpn commented Sep 26, 2023

tillkruss Sep 26, 2023

Choose a reason for hiding this comment

tillkruss commented Sep 26, 2023

deleugpn commented Sep 26, 2023

deleugpn commented Oct 6, 2023

deleugpn commented Nov 9, 2023

mnapoli Nov 9, 2023

Choose a reason for hiding this comment

deleugpn Nov 24, 2023

Choose a reason for hiding this comment

mnapoli commented Nov 9, 2023

deleugpn Nov 13, 2023

Choose a reason for hiding this comment

alustau Nov 13, 2023

Choose a reason for hiding this comment

mnapoli Nov 23, 2023

Choose a reason for hiding this comment

deleugpn Nov 24, 2023

Choose a reason for hiding this comment

deleugpn commented Nov 24, 2023

deleugpn commented Nov 24, 2023

KentarouTakeda left a comment

Choose a reason for hiding this comment

deleugpn commented Feb 19, 2024 • edited Loading

tillkruss left a comment •

edited

Loading

deleugpn commented Feb 19, 2024 •

edited

Loading