Discussion about renaming Yii::$app to Yii::getApp()

[yiisoft]

Discussion about renaming Yii::$app to Yii::getApp()

[lubosdz]

Please keep it simple, do not over-engineer.
Both Yii::$app-> and Yii::app()-> are OK - intuitive short syntax all developers are used to.
Personally I do not favour Yii::getApp()-> since it's just unnecessarily longer syntax.
Injecting app instance into all static helpers is not a good idea (increases API complexity) - just accessing thought global container Yii is performative, memory efficient and familiar to developers since Yii1.

[antonprogz]

@lubosdz Agree. I think Yii team should concentrate on new features (PSR compatibility, webpack support, using new php features) and make a release, not doing the meaningless refactoring of the di system (moving from SL to constructor DI, they both are ok) and renaming.

[rob006]

I assume one of those cases is AR?

Yes. And static helpers. Both cases could be fixed in some way, but I would like to know how it will look like before we start making it. We don't really need to make another Symfony.

What do you mean with "make it consistent"?

I tough that the same as you - eventually we want to use statics nowhere.

Isn't that a code smell?

Honestly, I don't see much difference between dependency on some global DB component and some global cache/urlManager - all smells the same :D.

[ddziaduch]

Why not move to https://www.doctrine-project.org ? 🙂

[SamMousa]

I agree with @rob006 that it should be injected in all places or all places should use statics. But is it possible?

I don't agree with this. This kind of strict consistency should imo not be enforced. Instead the goal should be eventual consistency; eventually we want to use statics nowhere, but in the meantime we make the application available for common cases X, Y and Z, if you have other scenarios you can still use \Yii::$app.

I agree with the goal, but requiring it immediately will make it a lot harder.

[rob006]

@SamMousa The problems are:

1. Right now we don't know how to make it consistent and how to handle some cases without use statics (without hurting convenience too much).
2. You will always have some statics and rely on some global state. The question is how far we can go with this "remove all statics" rush without loosing Yii spirit.

[schmunk42]

From reading this and those other issues:

• #11
• https://github.com/yiisoft/yii-core/issues/130

I think it is very important to REMOVE static from function Yii::getApp() before any stable release!

Either we live and encourage the fact, that this is the (handy) way to access it or do the clean way and remove that possibility completely.

It's very hard to remember in which places you can, should or should not use the application instance. If I want/need to access the application in a model ie. I need to think twice about the implementation design, if it is just not there; and this would save me from writing bad code.

[lubosdz]

Discussion about AR syntax should be moved elsewhere, not related to this topic.
BTW $this->query->newInstance(Product::class)-> ... is not really intuitive.

[samdark]

Doesn't matter anymore. We are trying to eliminate global access to application instance.

[SamMousa]

Honestly, I don't see much difference between dependency on some global DB component and some global cache/urlManager - all smells the same :D.

Well the code smell is mostly about optional dependencies. AR always needs a database, or more specific a storage backend. But it doesn't always need a UrlManager; so making your AR model dependent on that is a code smell ;-)

[Ziggizag]

@ddziaduch Oh, please - no DataMapper kind of stuff in Yii!

You must simply separate two "needs":

• one need is of package developer who is usually interested in the most relaxed coupling.
• but then there is app developer need who normally is looking for performance and development speed.

Doctrine adds to the first but deteriorates the latter. ActiveRecord is such a beauty - it is, actually, the very reason Yii was such a success.

[tecnologiaterabyte]

I support and agree with all these changes, I used yii 1, yii 2, and yii 3, before the separation if we read and look for many code quality applications like php_cs, scrutinizer, codeclimate, the first error that They are all static calls, should be done gradually, always from our comfort any change will have resistance, if those changes are improvements and adapt to the new architecture, semantic of php versions, I think it will be a breakthrough, It's just my two cents.

What if it would be ideal would be even a very detailed extension or a guide to the route to follow so that we can all move faster.

For me so far the important changes is that you can update the template with composer update, and with the hiqsol plugin each module or extension will have its own configuration without boostrap as boot.

Of course apart from all the cleaning of the code, its division and restructuring with di, this new approach that by the way many people requested, since it is easier to keep several packages divided, than a single monster.

And if you can inject all the code I do not see difference between:

yii :: $ app, yii :: getApp (), $ this-> app,

The only difference is that $ this->app will not give any error in any application of quality of code, something that we all look for, that we integrate our code, all these tests, looking for the quality of it, a normal cake is made by anyone, a cake of quality only by a few people, and the philosophy of yii has been slow with the changes, but it has been of quality, there are still things that have yii that has no framework.

And I congratulate all the team of yii, for the great work, as in every project there will always be discussions.

[antonprogz]

Here is my opinion about this.

Guys, I think you should definitely find answers to these questions before marking Yii::getApp() as deprecated:

1. How db Connection instance will be injected to AR instances?
2. How App instance will be injected to the static helpers?

[samdark]

@antonprogz good questions.

1. These could be accepted via constructor and auto-wired. But then we'll have to create ARs through a factory and AR would become less attractive in its syntax.
2. We can use Yii::$container. This solution is worse since it creates a dependency on container and implies that db is in there but, at least, AR syntax is kept in a good shape.

Overall, it has nothing to do with deprecating Yii::$app itself. There's no need for application instance for using AR.

[rob006]

These could be accepted via constructor and auto-wired. But then we'll have to create ARs through a factory and AR would become less attractive in its syntax.

This sounds much worse than dependency on static Yii::$container or Yii::$app, because then app becomes part of the AR, which is not true. App and AR instance should not be directly connected - you should be able to use the same AR instance in different application. If you store application instance as AR property, AR will use it even if actual application was changed (just imagine that you're storing AR in cache - after fetching AR instance from cache in different request, it will still use old application instead of current one).

[samdark]

@rob006 In point 1 I've meant connection is injected, not the app or container instance. Caching issue could be solved with custom serialization that excludes connection.

[rob006]

1. But you need connection before you create AR instance, right?
2. AR may use different components too (URL manager, cache), you want to inject everything in constructor?

[antonprogz]

Overall, it has nothing to do with deprecating Yii::$app itself. There's no need for application instance for using AR.

If I'm not mistaken you just want to deprecate Yii::$app and Yii::getApp(), but static calls Yii::createObject() and Yii::ensureObject() (previosly Instance::ensure()) will be still in place to get any dependency from the container?

[samdark]

@rob006 yes to both questions.

@antonprogz yes. These will still be there as well as referencing container explicitly. We should put a big warning that it should be avoided if possible though except special cases like AR instances.

[hiqsol]

@SamMousa Yes. This is the plan now.
Change gradually. Allow both variants.

[SamMousa]

These could be accepted via constructor and auto-wired. But then we'll have to create ARs through a factory and AR would become less attractive in its syntax.

Would it? When consuming AR models syntax won't change.
When constructing you would need a factory, this factory could be created via gii.
Usage would still be fairly fluent if the factory gets injected:

$car = $carFactory->create();

AR may use different components too (URL manager, cache), you want to inject everything in constructor?

Isn't that a code smell? I have AR records that need to sign URLs in certain methods, so these methods must provide a UrlSigner instance. Injecting dependencies is the way to go.

@rob006 yes to both questions.

Constructor injection is not ideal for optional dependencies though:

• Constructors will quickly accumulate a lot of parameters.
• The body of the constructor will be a lot of boilerplate to save the dependencies.
• Dependencies might be expensive to instantiate.

Of course in case of AR the question is if these things requiring optional dependencies should not be extracted to separate services. For example:

$car->getPreviewUrl(UrlManager $url)
// Could become
// This would be injected.
$carPreviewer = new CarPreviewer(UrlManager $url);
// This would be the syntax.
$carPreviewer->preview($car);

[SamMousa]

@rob006 I assume one of those cases is AR?

What do you mean with "make it consistent"? What inconsistency are you referring to?

[rob006]

But it doesn't always need a UrlManager

What is optional dependency or not depends on specific case and implementation. You can create Active Record implementation which will not require db component (or require it in 2 cases, so it could be passed as method argument), but will always require some kind of URL manager (like AR as abstraction to REST API).

There is a reason why AR is considered as anti-pattern, we can always find some code smell in it. ;]

[SamMousa]

Well that's why I changed db to storage. In your example you don't need an URL Manager, you need a connection component that uses a REST API.
The URL manager is about creating URL using application routing rules (thus it has configuration).
A connection to a database, whether it uses SQL + TCP or JSON + HTTP + TCP, is always required by AR.
The antipattern in AR (afaik) is that it violates the SRP since it models a domain object but also contains the knowledge of how to persist such object(s).

[antonprogz]

The antipattern in AR (afaik) is that it violates the SRP since it models a domain object but also contains the knowledge of how to persist such object(s).

This is a theory. What serious problems did you encounter from this violation in AR?
IMHO, AR is not an anti-pattern. It just as other patterns has it pros and cons. DataMapper has them too. For example:

class Cargo extends ActiveRecord
{
    const CAPACITY = 50;

    public function addItem(string $title, string $description)
    {
         return self::getDb()->transaction(function() use ($title, $description) {
             
             $id = self::getDb()->createCommand('SELECT id FROM cargo WHERE id=:id FOR UPDATE', [':id' => $this->id])->queryScalar();

             if ($id === null) {
                  throw new DomainException('Cargo is not found.');
             } 

             if ($this->item_count < self::CAPACITY) {
                 $item = new Item();
                 $item->title = $title;
                 $item->description = $description;
                 $item->cargo_id = $this->id;
                 if (!$item->save()) {
                      throw new DomainException();
                 }  
                 return $item;
             } else {
                throw new DomainException();
             }

         })

    }

}

I don't see any convenient way to implement this using DataMapper and save consistency in multitenant applications. DM enforces us to use a service for such a task.

[antonprogz]

Well the code smell is mostly about optional dependencies. AR always needs a database, or more specific a storage backend. But it doesn't always need a UrlManager; so making your AR model dependent on that is a code smell ;-)

Do you think that AR should have only one permanent dependency - the Connection instance?

[SamMousa]

@antonprogz I am not against AR, I just stated the arguments often used to call it an anti-pattern.

Do you think that AR should have only one permanent dependency - the Connection instance?

Not necessarily just one, although this is the only one I can think of at the moment.
We could even split that dependency into 2 smaller ones: TableSchema and RecordPersister.

[SamMousa]

I agree, but there is an issue regarding AR that has no solution yet.

How will AR get the connection if it cannot get the app instance?

[samdark]

Agree both about the need to remove it before 3.0.0 final release and about the lack of AR solution.

[antonprogz]

I can propose a solution for AR. Istead of:

$rec = new ActiveRecord();

$rec = ActiveRecord::findOne();

$rec = ActiveRecord::find()->scopeSmth()->one();

etc.

We could use a universal ActiveQuery

$rec = $this->query->newInstance(ActiveRecord::class);

$rec = $this->query->find(ActiveRecord::class)->one();

$product = $this->query->newInstance(Product::class);

$product = $this->query->find(Product::class)->one();

Using this instead of the new operator and static ::find() calls, we can inject anything we want into AR instances, including the db connection. Of course, when it is necessary, a custom ActiveQuery class could be created for each ActiveRecord class. Universal query should be injected into all places, where AR functionality is needed through the constructor.

Inserts and updates are performed as usual through AR instance calls, like ActiveRecord::save()

[SamMousa]

I considered something similar; but instead of overloading active query I used something like a repository.

Regardless of syntax this would be a very big change since AR internally assumes new objects can be created to inspect things like relations.

[antonprogz]

Repositories are widely used in the DataMapper implementations, using it with AR could confuse users.
I think ActiveQuery solution won't need lots of refactoring, since metadata could be retrieved as usual:

$this->query->newInstance(Product::class)->relations();

[rob006]

What is $this->query? Different AR may use different active queries. And different relations may use different database connections. It is not so easy when you want to support lazy loading...

[antonprogz]

Discussion about AR syntax should be moved elsewhere, not related to this topic.
BTW $this->query->newInstance(Product::class)-> ... is not really intuitive.

Ok, this is only a bare idea, it shoud be checked and discussed, syntax could be changed.

[antonprogz]

in my example, query is something like that:

use yii\db\ActiveQuery;
use yii\db\Connection;

class UniversalActiveQuery
{
     private $db;

     public function __construct(Connection $db)
     {
           $this->db = $db;
     }

     public function find(string $class)
     {
           return new ActiveQuery($class, $db)
     }

     public function newInstance(string $class)
     {
           return new $class($this->db, $this); //this is passed for the sake of relations management
     }
}

[antonprogz]

No, Doctrine is a mess =) (DataMapper has its drawbacks) I think yii core team will find an elegant way to solve AR connection injection problem. Maybe some sort of a brainstorm could help.

Fastest solution, i think, is to move all AR constructor calls into the factory class like above or something similar) and inject it everywhere instead of using new and static calls. Ok, overloading ActiveQuery could confuse and it was a bad idea (my fault), but in general it should be someting like ActiveRecordManager (like EntityManager in Doctrine), that will instantiate AR objects and inject Connection and all required dependencies to it. It should be also possible to create a custom factory class to customize AR object creation process. Some kind of a DI container for ARs.

[samdark]

@ddziaduch it is a separate topic. If you want to discuss it, proper place is the forum: https://forum.yiiframework.com/c/yii-3-0