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

[5.0] Illuminate\Foundation\Artisan is no longer needed in after the HTTP Kernel refactors. #6178

Closed

Conversation

crynobone
Copy link
Member

Signed-off-by: crynobone [email protected]

@GrahamCampbell
Copy link
Member

👍 Nice. :)

@bart
Copy link
Contributor

bart commented Oct 23, 2014

I'm not sure if this is the right solution because calling

Artisan::call('migrate', ['--seed' => true]);

from inside a controller now returns

InvalidArgumentException in Application.php line 549:
Command "migrate" is not defined.

@crynobone
Copy link
Member Author

@bart I would argue that using Artisan command from within a controller is a hack and (personally feel) at some point should be stopped. You should actually using the Illuminate\Database\Migrations\Migrator directly.

Why is the code failing? because MigrationServiceProvider is deferred, and only inside console environment does all deferred service provider is preloaded.

@bart
Copy link
Contributor

bart commented Oct 23, 2014

@crynobone Thanks a lot for your explanation. I already figured out that the ServiceProvider doesn't load the migrate command. So using Illuminate\Database\Migrations\Migrator will be the right choice. But this also means, that using Artisan:call('mycommand') from outside the CLI wouldn't work anymore, right?

@crynobone
Copy link
Member Author

@bart the trick with Illuminate\Foundation\Artisan it just that it forced deferred service provider to be loaded, before loading Illuminate\Console\Artisan which fire artisan.start event. https://github.com/laravel/framework/pull/6178/files#diff-3915f7c1d5906f98625395fd56d70a3fL41.

@bart
Copy link
Contributor

bart commented Oct 23, 2014

And that means calling Artisan::call() from inside a controller wouldn't be possible anymore for non-default (registered) Artisan command, right? It's no problem as long as you can call things directloy by class methods. But it was a fancy simple way which will be lost. Not a big thing but a fact people, who are switching to L5, should know. What do you think?

@bart
Copy link
Contributor

bart commented Oct 23, 2014

Could you please give an example how to call a command (migrate --seed) from a controller now? Didn't really get it :(

@crynobone
Copy link
Member Author

$app = app();

$app->make('DatabaseSeeder')->setContainer($app)->run();

@bart
Copy link
Contributor

bart commented Oct 23, 2014

Hmm I think I will wait for a comment of @taylorotwell. Maybe he won't kick the Artisan::call(), too. Your example seeds the database but how to migrate it? Sth like $app->make('Migrator')->setContainer($app)->run();.

Sorry for these questions. I know it's not a support forum but really want to know how to do it now and in future.

@crynobone
Copy link
Member Author

What is so hard with:

$app = app();
$migrator = $app->make('migrator'); // Illuminate\Database\Migrations\Migrator
$migrator->run($app->databasePath().'/migrations');

Or go with the hackish way (should work in theory):

App::loadDeferredProviders();
Artisan::call('migrate', ['--seed' => true]);

@bart
Copy link
Contributor

bart commented Oct 23, 2014

Nothing. It simply is more than Artisan::call('migrate --seed');

First sentence of Laravel Philosophy: Laravel is a web application framework with expressive, elegant syntax.

Maybe this elegant syntax will be lost in some parts with L5 which would be a real pity.

@crynobone crynobone force-pushed the patch/remove-artisan-foundation branch from 505d03f to db577cd Compare October 24, 2014 00:14
@bart
Copy link
Contributor

bart commented Oct 24, 2014

When I'm calling Artisan::call('migrate --seed'); after your last commit db577cd I'm still getting

InvalidArgumentException in Application.php line 549:
Command "migrate --seed" is not defined.

@crynobone
Copy link
Member Author

Artisan::call('migrate', ['--seed' => true]); !== Artisan::call('migrate --seed');

@GrahamCampbell
Copy link
Member

@bart Please continue this on the forums.

@bart
Copy link
Contributor

bart commented Oct 24, 2014

For sure I will and just for your information @crynobone: It's still not working:

ContextErrorException in QuestionHelper.php line 112:
Notice: Use of undefined constant STDIN - assumed 'STDIN'

My current solution is running the migrate command using Symfonys Process class.

@crynobone
Copy link
Member Author

@bart see #4721 (issue with STDIN exist even in 4.2 due to update on symfony/console, which all artisan command depends on).

@@ -41,6 +41,8 @@ public function __construct(LaravelApplication $laravel, Dispatcher $events)
$this->laravel = $laravel;
$this->setAutoExit(false);

$this->laravel->loadDeferredProviders();
Copy link
Member Author

Choose a reason for hiding this comment

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

Latest commit show that @taylorotwell explicitly load deferred providers from within Illuminate\Foundation\Console\Kernel, so I'm not sure if this still relevant (only useful if Laravel still want to support calling "artisan" from non-console environment).

Copy link
Member Author

Choose a reason for hiding this comment

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

After discussion with @taylorotwell on irc, he said to removed this line. Latest commit contain rebased on solve a merge conflict in console kernel.

Copy link
Contributor

Choose a reason for hiding this comment

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

Which means that the hacky way using Artisan:call() won't be supported in L5 anymore. Nice to know. Thanks sir!

@crynobone crynobone force-pushed the patch/remove-artisan-foundation branch 2 times, most recently from bc5b04c to bd75489 Compare October 25, 2014 02:57
@GrahamCampbell
Copy link
Member

@taylorotwell Without this patch, my test suites are broken for some of my laravel 5 branches: https://travis-ci.org/GrahamCampbell/Laravel-Core/jobs/39004380.

@crynobone
Copy link
Member Author

@taylorotwell has made the change upstream. Now just need to remove the dead code in ArtisanServiceProvider.

@crynobone crynobone closed this Oct 26, 2014
@crynobone crynobone deleted the patch/remove-artisan-foundation branch October 26, 2014 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants