Skip to content
This repository has been archived by the owner on Sep 9, 2019. It is now read-only.

Doubt Yii 3.0 ($this->app or Yii::getApp()). #11

Closed
terabytesoftw opened this issue Aug 21, 2018 · 7 comments
Closed

Doubt Yii 3.0 ($this->app or Yii::getApp()). #11

terabytesoftw opened this issue Aug 21, 2018 · 7 comments

Comments

@terabytesoftw
Copy link
Member

terabytesoftw commented Aug 21, 2018

Hi, I have a question revising the Yii 3.0 code when I can use $this>app or Yii::getApp(), I see that in yiisoft/app/controller/sitecontroller.php using $this->app and in the Yii::getApp() in view,

I would like to always use $this->app to avoid static calls.

Thanks.

Q A
Yii version 3.0
PHP version 7.1.17
Operating system Centos7
@SilverFire
Copy link
Member

We are working on reducing static access to application as well.
We have already injected application in objects where it was easier to do it terms of architecture, but in some places we have not reached a mutual agreement on how to redo it, so you still see static calls.

@hiqsol
Copy link
Member

hiqsol commented Aug 22, 2018

I would like to always use $this->app to avoid static calls.

That's the good approach :) We would like too!
But it requires substantial rework of many of yii core classes.
This work is in progress and meanwhile we've presented deprecated Yii::getApp() to have framework functional.

So general idea is simple: don't use deprecated methods, find better solutions, ask when in doubt.

@hiqsol hiqsol closed this as completed Aug 22, 2018
@antonprogz
Copy link

antonprogz commented Sep 5, 2018

What a terrible thing you are doing! Yii was originally built on top of static service locator pattern, and getting any dependency in any place gived us great level of flexibility. For people who are against this there is the Symfony framework. Please, do not restrict developers like Symfony does. Besides, huge amount of rewriting and redesigning should be done to remove Yii::getApp() (Yii::$app) from the framework completely and getting this job done will change the whole philosophy, do you admit that?

And how about Yii::ensureObject(), and Yii::createObject()? Do you want to deprecate them too? If not, I don't see any reason for the Yii::getApp() deprecation, because any dependency is still available statically from Yii::ensureObject().

@antonprogz
Copy link

Injecting the whole Application instance doesn't differ a lot from getting it statically, because the client class is still tightly coupled to yii's Application class. I wonder, if you could explain the benefits.

@hiqsol
Copy link
Member

hiqsol commented Sep 5, 2018

@antonprogz You have a very strong opinion but few arguments.
On the other side arguments for changes being done are well known.
You can google for:

  • service locator is anti-pattern
  • DI container should not be used explicitly
  • static calls are considered bad
  • global variables (Yii::$app) considered harmful

Please, do not restrict developers

This is a good argument. The idea was to empower developer, not constrain :)
At the 3.0 release all the old methods will still be available (possibly renamed). But deprecated - not recommended to use.
And I think at some time in future old methods will be moved to yii-legacy package to have the framework core clean but old stuff available for conservative developpers.

change the whole philosophy, do you admit that

What philosophy do you think is changed?
Could you please collect your thoughts for we could discuss it in a constructive manner.

@antonprogz
Copy link

antonprogz commented Sep 5, 2018

Could you please collect your thoughts for we could discuss it in a constructive manner.

service locator is anti-pattern

I don't agree with that. Service Locator was described by Martin Fowler, and who proclaimed it an anti-pattern? I consider SL as a one of the DI patterns with it's pros and cons. Constructor injection has it's own drawbacks too, for example:

  1. Constructors, polluted by so-called 'transitional dependencies':
class Item 
{
    private $dep;
    private $data;

    public function __construct(Foo $dep, array $data)
    {
        $this->dep = $dep;
        $this->data = $data;
    }
}

class ItemIterator 
{
    private $dep;
    private $data_source;

    public function __construct(Foo $dep, Iterator $data_source)
    {
        $this->dep = $dep;
        $this->data_source = $data_source;
    }

    public function current(): Item
    {
         $data = $this->data_source->current();
         return new Item($this->dep, $data);
    }
}

class Inventory
{
     private $api;
     private $dep;

     public function __construct(Foo $dep, $api)
     {
          $this->dep = $dep;
          $this->api = $api;
     }

     public function items(): Iterator
     {
          return new ItermIterator($this->dep, $this->api->fetchInventoryData());
     }
}

Item class has the Foo dependency to meet it's own needs. In order to create new Item objects, Inventory and ItemIterator should pass the Foo object through them. With SL it's not an issue:

class Item 
{
     private $dep;

     public function __construct()
     {
          $this->dep = DI::instance()->get('foo');
          Assert::isInstanceOf($this->dep, Foo::class);
     }
}
  1. Creation of not requred instances.

In the first example above, the Foo oject will be always created, no matter if the Inventory::items() method is called.
"Yii creates objects that are in use only" - is the one of the Yii's key exclamations. SL supports that, Constructor Injection - doesn't. In Java and C# it is not a problem, but PHP dies after each request and can't pre-create and reuse dependency graphs. I think Constructor DI could reduce perfomance and increase memory consumption in large PHP web apps.

DI container should not be used explicitly

If it is used as a service locator, that's not a bad stuff.

static calls are considered bad

In general, its true, but for one singleton (SL) static methods are ok.

global variables (Yii::$app) considered harmful

Global variables - yes. Singletons - no. Singletons could be mocked with ease. Signletons could not be replaced suddently: Yii::app() and YII::getApp() are not global variables. Why should they be marked as deprecated?

That's my opinion about your arguments). Here are some additional questions about the new 3.0 ROADMAP:

  1. If Yii team wants to avoid static calls and SL, how a db connection instance will be injected into new ActiveRecord instances?
$product = new Product($this->db);

I think this is not an option, passing the Connection instance around could pollute the code and decrease readability. The same is actual for the other dependencies that are widely used, like logger, urlManager, authManager etc.

  1. How to inject custom dependencies into ActiveRecord instances and its behaviors without Service Locator and static calls (Yii::createObject(), Yii::ensureObject())? Destroying this way of DI could totally reduce encapsulation and disavow the plain old "that models, thin controllers" recomendation.

P.S. @qiangxue, the creator of Yii (and the first core team members, of course), invented an outstanding PHP framework. They carefully reconsidered all the OOP principles and patterns and created very productive toolkit. SL is the one of the main features this productivity was built on.

@terabytesoftw
Copy link
Member Author

terabytesoftw commented Dec 26, 2018

I would like to always use $this->app to avoid static calls.

That's the good approach :) We would like too!
But it requires substantial rework of many of yii core classes.
This work is in progress and meanwhile we've presented deprecated Yii::getApp() to have framework functional.

So general idea is simple: don't use deprecated methods, find better solutions, ask when in doubt.

How to use di instead for $this->app in models, and do not use Yii::getApp(). @hiqsol

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants