laravel / tinker

Powerful REPL for the Laravel framework.
https://laravel.com/docs/artisan#tinker
MIT License
7.33k stars 130 forks source link

Vendor path is hardcoded #65

Closed taai closed 5 years ago

taai commented 5 years ago

Hi,

I had to make a Frankenstein – stitch together Rails app and Laravel app. Because of similar directory structure I decided to put Laravel app in subdirectory laravel, but for Laravel I re-used the vendor directory that was already in the root directory. I moved /laravel/public/index.php to /public/index.php and modified the paths in it. I modified /laravel/bootstrap/app.php file. I moved /laravel/artisan file to /artisan and modified the paths in it. I moved /laravel/.env file to /.env. I moved /laravel/resources to /resources. I moved /laravel/storage to /storage. I think that was it. And it works! πŸŽ‰

But when I tried to call php artisan tinker, it threw me this error: Command "tinker" is not defined.

So I did a little research and found out that I had to add Laravel\Tinker\TinkerServiceProvider::class to the list of providers in config/app.php. I guess that normally artisan just scans the vendor directory for tinker and, if it finds it, it adds it automatically? Because it works for normal installation. Am I right? πŸ€”

After I added that service provider, it threw me this error: Laravel\Tinker\ClassAliasAutoloader::__construct(): Failed opening required 'D:\project\laravel\vendor/composer/autoload_classmap.php' (include_path='.;C:\php\pear') I will tell you again – there is no /laravel/vendor directory, I am re-using /vendor directory.

It turns out that the vendor directory is hardcoded:

$path = $this->getLaravel()->basePath().DIRECTORY_SEPARATOR.'vendor/composer/autoload_classmap.php';

in this file: https://github.com/laravel/tinker/blob/v1.0.8/src/Console/TinkerCommand.php#L57

So, because the base_path is /laravel and not /, it can't find the vendor directory in it...

I ended up making custom TinkerServiceProvider that extends the original one and making custom TinkerCommand that extends the original one, but with:

$path = $this->getLaravel()->basePath().DIRECTORY_SEPARATOR.'../vendor/composer/autoload_classmap.php';

Can the vendor directory be configured in some config/tinker.php file or detected somehow from Composer or Laravel itself? This is the point of the issue. ⚠️

If someone is interested, this is how I changed the location of .env, storage and public paths (that are still located in the root directory of the project) in the /laravel/bootstrap/app.php file:

$app = new Illuminate\Foundation\Application(
    $_ENV['APP_BASE_PATH'] ?? dirname(__DIR__)
);

$app->useEnvironmentPath(dirname(__DIR__, 2));
$app->useStoragePath(dirname(__DIR__, 2).DIRECTORY_SEPARATOR.'storage');
$app->instance('path.public', dirname(__DIR__, 2).DIRECTORY_SEPARATOR.'public');
driesvints commented 5 years ago

In other places (like the artisan cli) it's hardcoded as well. That's because this is the standard place for the vendor directory to be. You'll always have to overwrite this manually somehow.

We also don't recommend putting Laravel in a sub directory for security reasons.

taai commented 5 years ago

What security reasons? πŸ˜„ I had a choice – put Rails in a sub-directory or the Laravel. I decided to put the Laravel in a sub-directory. because I knew it will be easier to do. Again – that sub-directory is not located in public directory. πŸ˜„ So, what "security reasons" are you talking about?

Tinker is already accessing Laravel app instance to get the base_path, so why do you think it will hurt to ask for the location of the vendor directory too? As I wrote, the Laravel app is in the sub-directory /laravel and the artisan file is in the root directory together with /vendor directory. I just didn't want to put the vendor directory in that sub-directory like this: /laravel/vendor... Tinker just needs to ask for the location of autoload_classmap.php...

I get that this is unusual case, but I don't get why you are not even considering this and you are closing this issue without a conversation... If there is an option to change EnvironmentPath, StoragePath, public path already, then providing a hint to Tinker about where to look for the autoload_classmap.php file should be piece of cake...

mokevnin commented 5 years ago

I came across the same issue. The main reason why I need to change the vendor directory location is sharing layers between docker images. I am creating a Laravel course, and I have a Docker image for every lesson's practice. It is about 15th images. So it is important for my platform (https://hexlet.io, courses for Russian speakers) to keep images as small as possible.

I use this configuration:

{
    config: {
         "vendor-dir": "/composer/vendor"
    }
}

And Dockerfile

COPY exercise/composer.json composer.json
RUN composer install

this setup works for everything except the Laravel

taai commented 5 years ago

this setup works for everything except the Laravel

Exactly! But in this case Laravel maintainer(s) stubbornly sticks with hardcoding things like that, except there are many customizable things too, so go figure. πŸ€·β€β™‚

We also don't recommend putting Laravel in a sub directory for security reasons.

@driesvints Sub-directory where? I can agree to that only if we are talking about putting Laravel in sub-directory of public directory. Otherways there is no security risk. Can you at least re-open this issue, so we can at least talk about it, instead of chopping off this idea at the root? πŸ˜‰ And I could contribute with code to resolve this, only if you, Laravel maintainers, would consider it and show at least some interest, so I'm not spending time for nothing…

driesvints commented 5 years ago

@taai sure. I was indeed talking about the public dir which I thought was what you meant. Feel free to send in a PR.

Vendin commented 5 years ago

@driesvints @taai I have been experiencing these problems for a long time, my first attempt to fix a year and a half ago https://github.com/laravel/tinker/pull/47.

Now PR https://github.com/laravel/tinker/pull/73

driesvints commented 5 years ago

Merged