yiisoft / yii-core

Yii Framework 3.0 core
https://www.yiiframework.com/
432 stars 75 forks source link

[2.1] Move `Module::runAction()` to `Application` #62

Closed SamMousa closed 5 years ago

SamMousa commented 7 years ago

Currently runAction is implemented in Module. However it is only ever called for root modules, which are Applications.

This won't break BC in most applications but since it doesn't functionally add anything I propose doing this in 2.1

samdark commented 7 years ago

Makes sense to me. @yiisoft/core-developers thoughts?

klimov-paul commented 7 years ago

It is need to be looked carefully. The actual solution should be developed in the scope of #13922. One thing can be said for certain: the threatment of the action result, like converting string or stream into yii\web\Response or integer to yii\console\Response should be perfromed at Application instead of Module.

SamMousa commented 7 years ago

@klimov-paul this is not about a functional change, just about a scope change. The current scope for runAction is Module, but it is never called other than by an Application.

klimov-paul commented 7 years ago

The current scope for runAction is Module, but it is never called other than by an Application.

That is unlikely to be possible, since runAction() includes instantiating controller and action instances, which search depends on the module. Module has the knoledge about controller path and may resolve route in his own way - such ability should not be dropped.

SamMousa commented 7 years ago

Actually that's createController.

klimov-paul commented 7 years ago

It is called inside runAction() and is just a particular implementation of route handling: https://github.com/yiisoft/yii2/blob/master/framework/base/Module.php#L522

Module for instance may handle all incoming routes and process them in some default manner instead of throwing InvalidRouteException or handle route without any controller or action involved.

SamMousa commented 7 years ago

Yes, but it's createController that calls the modules' createController, runAction is never called except for the root module. As far as I can tell runAction is never called for a module that is not the root module. All I'm proposing is that we move runAction to Application. I'm not proposing touching createController...

dynasource commented 6 years ago

rather than moving stuff away from Module.php, I would prefer the opposite. The more functionality is available in Module.php, the more can be reused in (module) extensions.

SamMousa commented 6 years ago

I feel there is a misconception; this specific part is not reusable at all...

Try overriding it in a module and see what happens...

On Oct 3, 2017 09:13, "Boudewijn Vahrmeijer" notifications@github.com wrote:

rather than moving stuff away from Module.php, I would prefer the opposite. The more functionality is available in Module.php, the more can be reused in (module) extensions.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/yiisoft/yii2/issues/14855#issuecomment-333892647, or mute the thread https://github.com/notifications/unsubscribe-auth/AAhYzUJGBSOvfCM6k4ZDpguSOzIwmWJ6ks5sol0ugaJpZM4Pce2m .

dynasource commented 6 years ago

this specific part is not reusable at all...

please elaborate

SamMousa commented 6 years ago

As I mentioned in the original post, the function is never called in a module only in an application.

On Oct 3, 2017 10:21, "Boudewijn Vahrmeijer" notifications@github.com wrote:

this specific part is not reusable at all...

please elaborate

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/yiisoft/yii2/issues/14855#issuecomment-333916564, or mute the thread https://github.com/notifications/unsubscribe-auth/AAhYzaQflydsIf4L3VOMsutMY77qzw7Cks5som0UgaJpZM4Pce2m .

samdark commented 5 years ago

Closing since implementation is too different now.