yiisoft / yii2

Yii 2: The Fast, Secure and Professional PHP Framework
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
14.24k stars 6.9k forks source link

Console commands run #1764

Closed Ragazzo closed 10 years ago

Ragazzo commented 10 years ago

Would be great to have ability to run console commands inside each other if user want to. Example: user designed command and want it to be able also to apply migrations (and then make some decisions on exit-code results), maybe call some other his written command. In Yii1 i was needed in such case, but there is nothing about it for Yii1. I got brief look of Yii2 console commands handling, should not be difficult. But anyway would be great if someone from core developers will handle this one. Proposed syntax

/**
 *
 * @param array|string $command command name to run. If array then first element
 * is command name, other are command options. ['migrate/mark', 'params' =>[20],
 * 'options' => ['db' => 'myConnection']]
 * @param boolean $isolated if command should run in isolated env. (application). 
 * New application will be created if needed.
 *
 */
public function call($command,$isolated)
qiangxue commented 10 years ago

Isn't Controller::run($route, $params = []) the one for this?

Ragazzo commented 10 years ago

@qiangxue yep, but i would like to have handful method, because for those developers that are not into core, it could be a little be difficult to guess for that. So useful method that can be a wrapper is just the case. But dont know how isolation will be handled in this case as it would be simple wrapper against Controller.

qiangxue commented 10 years ago

The run() method is designed to be used by users. Why call() is more understandable than run()?

Ragazzo commented 10 years ago

Why call() is more understandable than run()?

yes. And currently how would i call my migration command for example migrate/mark 20 --db='my_connection' with Controller::run($route, $params = []) could you give an example?

qiangxue commented 10 years ago

run('migrate/mark', ['db' => 'my_connection', 20])

Ragazzo commented 10 years ago

hm, so actions parameters should only be after controller properties? for example

run('migrate/mark', ['db' => 'my_connection' , 'table' => 'some_my_table', 20])

and not

run('migrate/mark', ['db' => 'my_connection', 20, 'table' => 'some_my_table'])

Anyway call() method would be very good for end user, because he is not good with internals. And in simple controller-run we can not make isolated call.

qiangxue commented 10 years ago

No, you can put action parameters before options or mix them.

I still don't understand the difference between call() and run() other than the extra $isolated parameter.

qiangxue commented 10 years ago

Also note that if you want to create new application instance, the old one will be gone because of the singleton.

-- update: actually, the one app is still there, but the new one will take Yii::$app. And you may end up using the new app by the old command after the new command finishes.

Ragazzo commented 10 years ago

I still don't understand the difference between call() and run() other than the extra $isolated parameter.

for end user it is simpler. Also one call() against (new Controller)->run(). And yes second param. User can even override cal() method if needed. So definitely call

Also note that if you want to create new application instance, the old one will be gone because of the singleton.

yes i know, very bad practice to be true, dont know why it was accepted. Anyway we can save Yii::$app before creating new one, similar to $oldController when application run.

qiangxue commented 10 years ago

How will the new application instance be created?

Ragazzo commented 10 years ago

i was thinking about $config (and if class is set there than creating application with that class) param as 3rd param for method call or we can simply do

new Application(require(Yii::getAlias('@app/console/config.php')))

or just require yii file under the framework @app path, but need to set some $_SERVER['argv'] then.

qiangxue commented 10 years ago

I think we should still use run() method because introducing another similar method call() would cause confusion.

We can introduce an extra parameter $isolated to run().

Application needs to save its initial configuration array internally and provides a method to re-create itself using the saved configuration.

qiangxue commented 10 years ago

Note that this method is not just used by console controllers, it can also be used by web controllers.

Ragazzo commented 10 years ago

Note that this method is not just used by console controllers, it can also be used by web controllers.

yes, thats why we should add call to yii\console\Controller and not to yii\base\Controller to not touch web-controllers.

We can introduce an extra parameter $isolated to run().

same as said above. But this could be used in HMVC for example in Yii1 it was like forward(). But if for HMVC we need also to save $_POST/$_GET if needed (child should not get params of parent.).

Application needs to save its initial configuration array internally and provides a method to re-create itself using the saved configuration.

not sure if it is needed since it is singleton, maybe then just remove Yii::$app set in constructor so it will not be singleton? or yes some getConfig() getter would be nice.

qiangxue commented 10 years ago

...I still don't get why you insist in adding call(). What's the difference between call() and run() if they both have $isolated?

not sure if it is needed since it is singleton...maybe then just remove Yii::$app set in constructor so it will not be singleton?

We still need the singleton. Otherwise how will you access application components?

new Application(require(Yii::getAlias('@app/console/config.php')))

This is not good. Internal code should not need to know how the configuration is obtained. The configuration could be coming from some cache. That's why I suggested saving the configuration within the application so that it could create a new one using it. The drawback is that it requires extra memory.

Ragazzo commented 10 years ago

...I still don't get why you insist in adding call(). What's the difference between call() and run() if they both have $isolated?

only because for console it makes more sense. also call is simle wrapper against Controller:run(). Or you sugges for end user to do this (new yii\console\Controller)->run('migrate/mark',[20]) when he wants to call custom command? I sure he will create then this method-wrapper by himself in 100%. So it is better to have it. And as for controller i dont think that Controller::run() should have $isolated because this param only makes sense for calling commands from commands and not for regular usage, so it is more of internal call logic then by Controller:run.

We still need the singleton. Otherwise how will you access application components?

like in Yii1, dont see any changes here. Yii::setApplication($app) vs Yii::$app. Our benefit is to eliminate strange unexpected behavior as setting in constructor Yii::$app.

This is not good. Internal code should not need to know how the configuration is obtained. The configuration could be coming from some cache. That's why I suggested saving the configuration within the application so that it could create a new one using it. The drawback is that it requires extra memory.

true, memory question i think can be solved by application property public $saveConfiguration = false; by default. In console config it can be true.

qiangxue commented 10 years ago

No, it's as simple as $this->run('migrate/mark', ['db' => 'my_connection', 20]);.

Ragazzo commented 10 years ago

But we dont need $isolated in run().

qiangxue commented 10 years ago

Why? This is just a parameter controlling how an action should be run. Supposedly, this is also useful for implementing HMVC. That is, it may be needed by web controllers.

Ragazzo commented 10 years ago

Because if you want full HMVC then you need to isolate other input parameters as GET/POST i dont think it would be added in next few days. But yes your argumentation is valid.

qiangxue commented 10 years ago

True, there's more work to do to fully support HMVC, which is not in our near-term plan.

Ragazzo commented 10 years ago

Yes, thats why i think to not confuse users we can add simple wrapper call and then after release if needed add hmvc support.

qiangxue commented 10 years ago

My point is that the name call() and run() are so similar and their functions are also very similar. It will cause more confusion. By adding $isolated parameter to run(), we can write clearly in the API comment about what isolation it means. Note that even for console command, we don't achieve full isolation because of $_SERVER.

Ragazzo commented 10 years ago

My point is that the name call() and run() are so similar and their functions are also very similar.

nope, if we will not add HMVC now. Then call will have some code for handling application creating with $isolated param, and run will not.

Note that even for console command, we don't achieve full isolation because of $_SERVER.

can back-up it, i guess. yii\console\Application::_backUp after application is executed can restore. Just an idea, we can find better solution i think, or we can avoid it by setting params explicitly.

klimov-paul commented 10 years ago

From my point of view this discussion is pointless.

Example: user designed command and want it to be able also to apply migrations (and then make some decisions on exit-code results)

I can see only one appropriate solution for this: extract the migration functionality into the separated class (maybe application component), while the “MigrateCommand” should be only an interface wrapper around it. This will allow developer to reuse the migration mechanism in any place and any interface he wants.

Because if you want full HMVC then you need to isolate other input parameters as GET/POST

I am afraid it is unreachable with the current framework architecture: just see “Pagination::getPage()” for an instance. If some listing should be a part of “slave” controller, how we should handle such things?

Introducing a new method “call” does not make any sense. There is “run()” method already, it usage is not so complex as you think, while it give developer more clear understanding of the process.

Ragazzo commented 10 years ago

There is “run()” method already, it usage is not so complex as you think, while it give developer more clear understanding of the process.

not sure about it, and once again there was discussion also about $isolated param to. So for end-user it is not so simple and obvious to use run. I think that almost all users before this issues was created did not know that they can run sub-commands with controller-run.

samdark commented 10 years ago

Noone actually asked :)

Ragazzo commented 10 years ago

Yes, nobody actually knew about it i think.

klimov-paul commented 10 years ago

My opinion: call one controller from another is a bad practice of code reusage. It breaks the meaning of MVC. If you wish some piece of code to be used in 2 different controllers, then extract it into separated class: component, helper, model, whatever. I do not think framework should promote such practice, introducing special method for it.

there was discussion also about $isolated param

I don’t get it. We have “request” and “response” components already. Their usage ensures the particular controller returns its result as “Response” instance, while request parameters are fetched from “Request” instance. Console controller has a property “interactive”, which determines if it would try to fetch user input or not. What else are you planning to isolate? At the moment, this parameter just sounds like some magic trigger, which will suddenly resolve all possible problems with nested controller usage, while those problems should be avoided by wise application design (see above).

Ragazzo commented 10 years ago

It breaks the meaning of MVC. If you wish some piece of code to be used in 2 different controllers, then extract it into separated class: component, helper, model, whatever.

yes, i wish some piece of code to be used in 2 different controllers and yes as an example it already extract it into separated class, for example migration command. But now i need to call it via $this->run(). This is not good, for reasons mentioned above.

What else are you planning to isolate?

Environment in which new command will be executed.

klimov-paul commented 10 years ago

Environment in which new command will be executed.

Could you be more specifc?

Ragazzo commented 10 years ago

you call $this->call('migrate/apply')and it will be executed in separated environment (to be simple in new Application()). This was discussed above with Qiang. To avoid components mixed-up between commands calls. Anyway of course we may close this issue with answer like: useexec()` :)

klimov-paul commented 10 years ago

This requires creation of the new application instance, which should be configured initialized and so on. Such approach will impact the performance. For example: imagine I have created an application behavior, which will adjust “Application::params” with the values fetched from the database via SQL query. If I attach this behavior to my web and console application, using “call()”, will actually perform the opening of 2(!) database connections and execution of 2 SQL queries. Last time I have met such brutal approach was a project written on CakePHP, which came to me with task “improve the performance”, which I have resolved removing such “call()”.

Ragazzo commented 10 years ago

@klimov-paul i understand, but once again your use-case is specific, you only described drawbacks and no features. I said about some features, dont know why i am fighting to be true) In L4 you can easily call another command from command, if Yii architecture is not good enough for that, then what is the question? :)

klimov-paul commented 10 years ago

you call $this->call('migrate/apply') and it will be executed

What is wrong with Yii::$app->runAction('migrate/apply')?

Ragazzo commented 10 years ago

name is wrong, if we are not talking about isolation.

klimov-paul commented 10 years ago

name is wrong

Why?

klimov-paul commented 10 years ago

Method Module::runAction() accept route and action params, while returning Response instance. It should be just what you need.

Ragazzo commented 10 years ago

because end user is calling command and not running action. it is more intuitive to do $this->call() then $this->run()

Ragazzo commented 10 years ago

Method Module::runAction() accept route and action params, while returning Response instance. It should be just what you need.

nope, @qiangxue answered above about run() method.

adamaltman commented 10 years ago

http://www.google.com/trends/explore#q=run%20command%2C%20call%20command&cmpt=q

run command is more popular than call command.

Ragazzo commented 10 years ago

@adamaltman its not only about the name :D

samdark commented 10 years ago
$oldApp = \Yii::$app;
new \yii\console\Application([
    'id' => 'Command runner',
    'basePath' => '@app',
    'components' => [
        'db' => $oldApp->db,
    ],
);
\Yii::$app->runAction('migrate/up', ['migrationPath' => '@yii/rbac/migrations/', 'interactive' => false]);
\Yii:$app = $oldApp;

@qiangxue can it be done simpler?

qiangxue commented 10 years ago

Do you mean running a console command from a Web application?

samdark commented 10 years ago

From a web application or tests or another console application or whatever.

qiangxue commented 10 years ago

What about using system() or exec()?

samdark commented 10 years ago

Many hosts won't allow you using it.

samdark commented 10 years ago

@qiangxue so should we introduce shortcut method for it?

Ragazzo commented 10 years ago

i am fine with Yii::$app->runAction(), not sure for isolation level, i think this one has low priority )

stepanselyuk commented 10 years ago

Hi guys! So how to do it now? (Run specified console command from a web application or tests or another console application or whatever.)

I tried as suggested Samdark, but it's not worked:

class ConsoleAppHelper
{

    public static function runAction( $route, $params = [ ], $controllerNamespace = null )
    {

        $oldApp = \Yii::$app;

        // fcgi doesn't have STDIN and STDOUT defined by default
        defined( 'STDIN' ) or define( 'STDIN', fopen( 'php://stdin', 'r' ) );
        defined( 'STDOUT' ) or define( 'STDOUT', fopen( 'php://stdout', 'w' ) );

        /** @noinspection PhpIncludeInspection */
        $config = require( \Yii::getAlias( '@app/config/console.php' ) );

        $consoleApp = new ConsoleApplication( $config );

        if (!is_null( $controllerNamespace )) {
            $consoleApp->controllerNamespace = $controllerNamespace;
        }

        // use current connection to DB
        \Yii::$app->db = $oldApp->db;

        $exitCode = \Yii::$app->runAction( $route, array_merge( $params, [ 'interactive' => false ] ) );

        \Yii::$app = $oldApp;

        return $exitCode;
    }
} 

I didn't see results (it should be generate file in runtime path) of run action.