laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.51k stars 11.02k forks source link

[5.1] [5.2] Composer update issues #9678

Closed GrahamCampbell closed 8 years ago

GrahamCampbell commented 9 years ago

Say you add a new service provider and try to deploy, current Laravel will mess up when it comes to clear the compiled file. This has been brought up many times before, but nothing ever done about it. I'm creating this issue so we can get this sorted.

garygreen commented 9 years ago

Just posting here I fixed this with a custom standalone clear-compiled command in my base folder:

clear-compiled

#!/usr/bin/env php
<?php
foreach (glob(__DIR__ . '/bootstrap/cache/*.*') as $file) {
    @unlink($file);
}
echo 'Cleared compiled directory.';
exit();

Then in composer.json change php artisan clear-compiled to simply php clear-compiled

"scripts": {
    "pre-install-cmd": [
        "php clear-compiled"
    ],
    "post-install-cmd": [
        "php clear-compiled"
    ]
},

This could be placed in laravel and call that file when artisan detects clear-compiled is being run.

crynobone commented 9 years ago

Why not include some custom symfony console app which can be executed from vendor/bin.

GrahamCampbell commented 9 years ago

@crynobone That would probably be overkill. Why make it complicated?

dan-har commented 9 years ago

The problem here is that the command depends on code that is not known, so this issue can be solved if all the service providers that are not part of the framework will not be loaded by the console kernel.

Two steps can be done:

  1. To depend only on the framework code - the kernel will not use the service provider bootstraper and instead will use a different bootstrapper that will load only the minimum required services (or only the framework services)
  2. When adding a command to the console, set the command's application instance with a new laravel application / default application instead of the kernel's application that starts the commands

This results in a separation between the application that activates the console commands, and the application (or applications) that actually being used by each of the commands

Is it worth to write a PR for this? It can require some work, also this solution need to support the queue and scheduled tasks

garygreen commented 9 years ago

@dan-har honestly, that sounds like a lot of work for a command that basically deletes some files. Why go to that trouble when all you need is a simple php script like the one I gave?

dan-har commented 9 years ago

@garygreen Yes, your code is simple and perfect and also does the job for my needs, but I think the purpose here is to find a way that the application will delete these files.

garygreen commented 9 years ago

@taylorotwell any news on this? There's so many issues being reported and it's a very simple fix, if only clear-compiled worked without booting the framework. How do you feel about the command I proposed above?

taylorotwell commented 9 years ago

Might be something to do for 5.2.

leemason commented 9 years ago

Am i missing something, or can this not just be moved from "pre-update-command" to "post-update-command":

change:

"pre-update-cmd": [
    "php artisan clear-compiled"
],
"post-update-cmd": [
    "php artisan optimize"
],

to:

"pre-update-cmd": [],
"post-update-cmd": [
    "php artisan clear-compiled",
    "php artisan optimize"
],

surely if its done before "php artisan optimise" it will work the same only after the vendor directory has been updated?

Whats the need for seperate bootstrapers, no bootstrapers or console applications when it just needs to be given the correct time to execute?

leemason commented 9 years ago

Thinking about it, what i said before would work in more cases than now, but in some cases would have the same error IF the existing code had services providers REMOVED from the newer version (during a build of course).

why not just add a condtional in the bootstrap.php file to read from an Symfony\Component\Console\Input\ArgvInput and if there is an option "--no-cache" dont load the cached files?

leemason commented 9 years ago

ok, what about this?

This is an amendment to the bootstrap/autoload.php file that simply checks if its cli, if it is looks for a --no-cache option, and if it finds it defines a constant and removes the option from the argv so it doesnt effect anything else.

Then anywhere a cached file is included we can adjust the if statement to include

!defined('NO_BOOTSTRAP__CACHE');

https://gist.github.com/leemason/3727023cc5a08c219833

happy to make a pull request if approved, simple bug fix that could be applied to current versions i think

garygreen commented 9 years ago

I think that's far too complicated. All the clear-compiled command does is delete the cache stuff. It should just be it's own standalone command -- the whole framework doesn't need to be booted up to delete a few files.

leemason commented 9 years ago

@garygreen totally understand your point, but its a few lines of code that has no effect or overhead on anything else.

Plus explaining to users

"all console commands go through artisan, except one which is different because of xy and z"

is surely less the laravel way than just saying:

"add --no-cache to prevent loading config and service cache files"

if artisan cant "delete a few files" and we require a seperate file to do it, its not selling the artisan console very well.

from memory there is only the config cache file in the autoload.php and the services.json (cant remember where its required) that would need this conditional defined check so its not really that complicated.

leemason commented 9 years ago

good point, it would still need to be done before "artisan" though as if its not done before the end of autoload it will include the compiled file.

maybe something like this, preserving the "php artisan clear-compiled" usage but deletes files and exits before ever getting to the application:

https://gist.github.com/leemason/3f1290446f856760b057

vlakoff commented 9 years ago

May be somehow related to #10161. TL;DR: requests to a "downed" website (through artisan down) should short circuit the Application as well.

crynobone commented 9 years ago

requests to a "downed" website (through artisan down) should short circuit the Application as well.

That a different issue, the error does occur when request are loaded while another request is still compiling compiled.php (which would load a broken file). Fixing this problem doesn't solve that issue.

GrahamCampbell commented 9 years ago

May be somehow related to #10161. TL;DR: requests to a "downed" website (through artisan down) should short circuit the Application as well.

Yeh, you should NEVER be compiling on a website that's actually live. Switch it out using symlinks or something.

taylorotwell commented 9 years ago

We could probably get around that limitation by writing a temp file?

On Tue, Nov 10, 2015 at 12:05 PM, Graham Campbell notifications@github.com wrote:

May be somehow related to #10161 https://github.com/laravel/framework/pull/10161. TL;DR: requests to a "downed" website (through artisan down) should short circuit the Application as well.

Yeh, you should NEVER be compiling on a website that's actually live. Switch it out using symlinks or something.

— Reply to this email directly or view it on GitHub https://github.com/laravel/framework/issues/9678#issuecomment-155515977.

GrahamCampbell commented 9 years ago

Still wouldn't help. People's apps still get broken during a composer install.

kalfheim commented 8 years ago

So.. why run php artisan clear-compiled in pre-update-cmd. Couldn't this be fixed by moving it to post-update-cmd? I have moved it, and everything seems to run perfectly.

Right now, running any Artisan command in pre-update-cmd is risky business, as it will break the update process if there's for example a service provider defined in the config which doesn't exist yet. We gotta let Composer pull in the files before attempting to read them, right? Right.

I have run in to this issue multiple times, and I gotta say, it's really annoying. I'm surprised it hasn't been fixed. When it happened the first time, I though I had done something wrong, you know? It's messing with my feelings. :sob:

crynobone commented 8 years ago

I have moved it, and everything seems to run perfectly.

Everything will run smoothly until one of the cache class has an update, add new method etc. While this is rare but it does happen, even in patch releases.

kalfheim commented 8 years ago

@crynobone: Sure, but why can't the command be executed after the composer update instead? Doesn't php artisan optimize recompile (and overwrite) the cached files anyway?

crynobone commented 8 years ago

@krisawzm php artisan optimize will first load the whole app (and since compiled.php still exist and containing the old structure), it would throws an error.

Errors such as #8520 #8127 #8124 #9660

kalfheim commented 8 years ago

Yes, I understand that, but given this:

"post-update-cmd": [
    "php artisan clear-compiled",
    "php artisan optimize"
],

the compiled.php file will be removed before php artisan optimize is executed. Why is this not a sufficient fix?

crynobone commented 8 years ago

Why is this not a sufficient fix?

Because php artisan clear-compiled would also first load the whole app. Same applied in any artisan command. How would it be any different?

garygreen commented 8 years ago

@krisawzm the problem is Laravel loads the compiled.php file ALWAYS if it exists, regardless of how you've setup your composer.json. The only way to remove compiled.php is manually, or thru the php artisan clear-compiled command -- but that command bootstraps the whole app which can fail if your dependencies have changed.

SelimSalihovic commented 8 years ago

Any update on this? Having major issues right here: http://stackoverflow.com/questions/34465721/laravel-5-1-to-5-2-upgrade-error

ryanmortier commented 8 years ago

What is the work around for this? I just upgraded one of our apps from 5.1 to 5.2 and I'm getting:

image

garygreen commented 8 years ago

@ryanmortier use my clear-compiled command ;-)

wjaspers commented 8 years ago

The underlying issue appears to be composer.

Proof of concept:

# Install laravel
composer.phar install
php artisan # expect a list of commands

# Add a provider to your app config

php artisan # expect an error

# Rebuild the autoloader
composer.phar dump-autoload --no-scripts
php artisan # expect an error

# Rebuild the autoloader with optimizations
composer.phar dump-autoload -o --no-scripts
php artisan # expect a list of commands

Altering the scripts section as follows has done well in previous versions.

Personally, post-autoload-dump makes more sense to me, since I expect update and install to always affect the autoloader.

...
scripts: {
  "post-autoload-dump": [
    "php artisan clear-compiled"
    "php artisan optimize"
  ],
  "post-create-project-cmd": [
      "php artisan key:generate"
  ]
}
...
ryanmortier commented 8 years ago

Guys, my issue turned out to be that I had a space in one of my environment variables in the phpdotenv (.env) file.

For example: MAIL_NAME=First Last

lol? This must be new as of Laravel 5.2 or something because I've never had an issue before.

The solution was to wrap it in quotes: MAIL_NAME="First Last"

GrahamCampbell commented 8 years ago

This issue is unrelated.

GrahamCampbell commented 8 years ago

lol? This must be new as of Laravel 5.2 or something because I've never had an issue before.

Yes, that's a consequence of us upgrading from phpdotenv 1.x to 2.x.

ryanmortier commented 8 years ago

Yes, that's a consequence of us upgrading from phpdotenv 1.x to 2.x.

Ok, thanks for the info.

TommyC81 commented 8 years ago

Bumping this as I keep running into it as well. Gets a bit annoying to have to realize a deployment has failed a day later. Any news on a potential Laravel-ish solution?

ba0708 commented 8 years ago

Also waiting for this, as it has been causing me major issues for a long time now.

DaneEveritt commented 8 years ago

For the time being if you need a quick workaround you can do:

composer update --no-scripts; php artisan clear-compiled; php artisan optimize
GrahamCampbell commented 8 years ago

For the time being if you need a quick workaround you can do:

That won't always work either.

jcorry commented 8 years ago

This issue is jacking up my ability to run peridot tests (requires symfony console 2.something...can't use L5.2) on my package in Travis CI.

My travis build fails because the pre-update-command fails after creating a new laravel 5.1.* project.

I see that the 5.2 branch has been updated removing the pre-update-cmd but I need 5.1 for peridot.

GrahamCampbell commented 8 years ago

Closing, since this is being addressed now. It's up to Taylor if he wants to have this on 5.1. So, that part doesn't really need discussing.

nicktc commented 8 years ago

I still have a problem. Using the updated composer.json inside Laravel 5.2:

        "post-root-package-install": [
            "php -r \"copy('.env.example', '.env');\""
        ],
        "post-create-project-cmd": [
            "php artisan key:generate"
        ],
        "post-install-cmd": [
            "Illuminate\\Foundation\\ComposerScripts::postInstall",
            "php artisan optimize"
        ],
        "post-update-cmd": [
            "Illuminate\\Foundation\\ComposerScripts::postUpdate",
            "php artisan optimize"
        ]

But still getting an error when installing or updating:

Script php artisan optimize handling the post-install-cmd event returned with an error

  [RuntimeException]                                                                                                                                                                                       
  Error Output: PHP Fatal error:  Class 'App\Modules\XXXServiceProvider' not found in XXX/vendor/laravel/framework/src/Illuminate/Foundation/Pro  
  viderRepository.php on line 146   

I am doing something wrong?

garygreen commented 8 years ago

@nicktc use my clear-compiled command whilst Taylor fixes this for next laravel version ;-)

nicktc commented 8 years ago

@garygreen but there is no php artisan clear-compiled anymore in the composer.json. So what should I replace with your command? I only have 'php artisan optimize'.

barryvdh commented 8 years ago

That's an issue in your own project, make sure the class exists.

nicktc commented 8 years ago

@barryvdh but I don't have this problem on my local dev?

        "psr-4": {
            "App\\": "app/",
            "Modules\\": "app/modules"
        }

Inside app/modules I have the file ModuleServiceProvider.php with the following content:

<?php

namespace App\Modules;

use \Illuminate\Support\ServiceProvider as ServiceProvider;

class ModuleServiceProvider extends ServiceProvider
{
    public function boot()
    {
        $modules = config("module.modules");
        while (list(,$module) = each($modules)) {
            if (file_exists(__DIR__ . '/' . $module . '/routes.php')) {
                include __DIR__ . '/' . $module.'/routes.php';
            }
            if (is_dir(__DIR__ . '/' . $module . '/Views')) {
                $this->loadViewsFrom(__DIR__ . '/' . $module . '/Views', $module);
            }
        }
    }

    public function register()
    {
        //
    }
}

And inside my config/app.php inside the providers list the following:

App\Modules\ModuleServiceProvider::class

nicktc commented 8 years ago

Problem resolved. Apparently my local dev environment (Mac) is case insensitive. The ModulesServiceProvider could not be loaded because the app/modules folder needed to be app/Modules.

chintanvadi commented 7 years ago

Script @php artisan package:discover handling the post-autoload-dump event returned with error code 1