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

Dependency inversion and AR #16

Open
SamMousa opened this issue Nov 6, 2018 · 5 comments
Open

Dependency inversion and AR #16

SamMousa opened this issue Nov 6, 2018 · 5 comments

Comments

@SamMousa
Copy link
Contributor

SamMousa commented Nov 6, 2018

Historically in Yii2 one of the parts where it is really hard to inverse dependencies is AR.

The current AR implementation uses a static factory Car::find() which creates a query object.
The model depends on a database connection, which is found via:

    /**
     * Returns the database connection used by this AR class.
     * By default, the "db" application component is used as the database connection.
     * You may override this method if you want to use a different database connection.
     * @return Connection the database connection used by this AR class.
     */
    public static function getDb()
    {
        return Yii::getApp()->get('db');
    }

The follows directly from the AR pattern. An AR object needs to know how to persists itself (it essentially violates SRP by design).

Currently there's 2 main ways of using AR, creation and searching:

$carQuery = Car::find()
$car = new Car();

These things are primarily called inside controller actions, where we (should?) have DI available. This means that it could be feasible to remove the Yii::getApp() dependency as follows:

    class ActiveRecord {
        private $repo;
        public function __construct(Repository $repository) {
            $this->repo = $repository;
        }
       
        public function getRepository(): Repository
        {
            return $this->repo;
        }
        public static function getDbService():string {
            return 'db';
        }

        protected function saveInternal() {
            // In case we want to refactor more.
            $this->repo->saveModel($this);
            // In case we want to keep the database logic where it is
            $this->getDb()->schema->insert(self::tableName(), $vaues);
        }
        // Not sure this helper is needed / should be public.
        public function getDb(): Connection
        {
            return $this->repo->getDb(self::getDbService());
        }
    }
    class Repository {
        public function getDb(string $name): Connection;
    }

    public function actionView(Repository $repo, int $carId)
    {
        $carQuery = Car::find($repo);
        $car = new Car($repo);
    }

Now of course this means that we always need a Repository when working with AR, but since we usually have this at the start, it should not be hard to get these and then pass it on.
We make it available from the AR object so it can be reused when using relations or saving the object later.

Note this is a very raw idea, feedback, as always is welcomed.

@rob006
Copy link
Contributor

rob006 commented Nov 6, 2018

We make it available from the AR object so it can be reused when using relations

Relations may use different repository/connection.

In addition such dependency will make caching of AR really tricky...

@SamMousa
Copy link
Contributor Author

SamMousa commented Nov 6, 2018

Relations may use different repository/connection.

They won't use a different repository; think of the repository as a factory or service locator for just DB connections.
You would have 1 of those in your application. Specifically, if you use multiple you would indeed have to make sure that the whole model-chain for a query uses connections defined in that repository.

In addition such dependency will make caching of AR really tricky...

Why would it become trickier?

The only thing that changes is the way we retrieve the connection, you can still use caching in the exact same way?

--> I realize with AR you can never have it be totally pure, but this approach would allow us to use this library without having global contants like Yii::$container or similar constructs to do the same thing.

@rob006
Copy link
Contributor

rob006 commented Nov 6, 2018

Why would it become trickier?

It may depend on Repository implementation, but I'm seeing only two options:

  1. Repository takes Connection from global scope (same as Yii::$app->db), so you're not changing anything - Repository is just an abstraction for current getDb() and works in exactly the same way.
  2. Repository uses dependency injection to get Connection, then caching AR will cache Repository which will cache Connection.

Am I missing something obvious?

@SamMousa
Copy link
Contributor Author

SamMousa commented Nov 6, 2018

(I'm making this up as I go)
The repository could be configured in the DI container like this:

[
    'db1' => [normal db config],
    'db2' => [normal db config],
    'db3' => [normal db config]
    'mainRepo' => [
        'connections' => [
            Reference::to('db1'),
            Reference::to('db2'),
            Reference::to('db3'),
    ]
]

What's relevant here is the fact that the consuming code gets the repository injected and then manually injects it into AR.

public function actionX(Repository $repository)
{
    $car = new Car($repo);
}

Should we be caching the AR models themselves? Or just the underlying queries?
I've never had good experiences with serializing AR models and instead just cache queries.

If serialization of AR models is a requirement then you are right we have an issue, as anything with an injected dependency cannot simply be serialized / unserialized.

In case of AR, we could have the Repository do serialization / unserialization for us; but I have not considered or thought about this yet.

@rob006
Copy link
Contributor

rob006 commented Nov 6, 2018

To make this clean we would need to remove some magic from AR and remove direct dependency on connection:

  1. Class itself should contain all info about related DB schema - available columns/properties and its types. You should be able to do new Car() without touching DB connection or cache component.
  2. Require explicitly pass DB connection on saving the object to DB $model->save($connection).
  3. Require explicitly pass DB connection on fetching the object from DB Car::find()->one($connection).

But it makes using AR less convenient and more error-prone - IMO it will create more problems than it solves.

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

No branches or pull requests

2 participants