Skip to content

Conversation

@cebe
Copy link
Member

@cebe cebe commented Oct 29, 2013

  • Move commonly used functions of Query, ActiveQuery and ActiveRelation to traits
  • Make ActiveRelation class configurable in AR diff
  • where to put the SORT_ASC and SORT_DESC constants?
  • adjust class and trait documentation

I created a BaseQuery in yii\db that supports where, limit, offset, orderBy and indexBy. These are imo the basic operations that have to be implemented by a dbms. This is also everything that is needed by dataprovider and gridview.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 3130aad on db-traits into 155e9c6 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) when pulling 3130aad on db-traits into 155e9c6 on master.

@ghost ghost assigned qiangxue Oct 29, 2013
@cebe
Copy link
Member Author

cebe commented Nov 2, 2013

where to put the SORT_ASC and SORT_DESC constants?

Can't we just reuse the PHP constants SORT_ASC and SORT_DESC?
http://php.net/manual/en/array.constants.php
Not perfect because one can not use true and false anymore but imo using true and false is not good practice anyway as it is not clear which value is asc and which desc.

I tried to do it with namespaced constants that are accessable like yii\db\SORT_ASC but we become problems here with autoloading because it is not possible to use the constant before BaseQuery has been autoloaded.

@cebe
Copy link
Member Author

cebe commented Nov 2, 2013

defining constant in a trait is not possible btw.

@qiangxue
Copy link
Member

Moved to beta milestone. I'm still not quite sure about this...

@cebe
Copy link
Member Author

cebe commented Nov 13, 2013

Further Todo:

  • Rename traits and create interfaces: QueryTrait, QueryInterface, ActiveRelationInterface
  • Use PHP SORT_ASC and SORT_DESC

cebe added 2 commits November 13, 2013 16:50
Updated AR classes with the latest changes from master to go on with
separation of AR query classes in traits.

Conflicts:
	framework/yii/db/ActiveQuery.php
	framework/yii/db/ActiveRecord.php
	framework/yii/db/ActiveRelation.php
@cebe
Copy link
Member Author

cebe commented Nov 13, 2013

This is the current structure of db classes now:

db-class-diagram

The PHPdoc got duplicated now as it is once in the interface and once in the trait. need to clean that up later (best when we have api doc generator to see where @inheritdoc takes documentation from when we implement interface and use a trait at the same time).

this allows us to implement other activerecord implementations based on
NoSQL dbms
@cebe
Copy link
Member Author

cebe commented Nov 13, 2013

  • Need to add some tests for ActiveDataProvider to ensure it works as expected.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.16%) when pulling 6533897 on db-traits into 0eaafd7 on master.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...implemented by active query classes?

@qiangxue
Copy link
Member

What about ActiveRecord? Is it possible to extract an interface and/or a trait?

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 9448c3d on db-traits into 0eaafd7 on master.

@cebe
Copy link
Member Author

cebe commented Nov 13, 2013

What about ActiveRecord? Is it possible to extract an interface and/or a trait?

As I said in discussion yesterday I'd like to do that later when I implemented redis and elasticsearch to see what is the common part.

strict sql dbms like postgres would fail otherwise
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling 51faa62 on db-traits into 0eaafd7 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 1acdbb7 on db-traits into 0eaafd7 on master.

@cebe
Copy link
Member Author

cebe commented Nov 13, 2013

Should be all fine now except the count() signature issue.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 8250cfb on db-traits into 0eaafd7 on master.

qiangxue added a commit that referenced this pull request Nov 14, 2013
@qiangxue qiangxue merged commit b449211 into master Nov 14, 2013
@qiangxue
Copy link
Member

Everything looks good to me. Thanks for the great work!

@qiangxue
Copy link
Member

How about rename yii/ar to yii/query and move QueryInterface and QueryTrait into that directory too?

@qiangxue
Copy link
Member

Actually, perhaps we should put all these interfaces and traits under yii/db. They are all part of DB support in general. Having a separate directory such as yii\ar or yii\query could cause confusion.

@cebe
Copy link
Member Author

cebe commented Nov 14, 2013

I initially thought about having the db independend ActiveRecord classes in yii\ar and the SQL-DB specific things in yii\db. But as yii\ar\ActiveQueryInterface extends from yii\db\QueryInterface and ActiveQuery interface is then again implemented in yii\db it is better to have them all in yii\db imo. I also tend to putting redis elasticsearch and others also under yii/db. But that decision can be made when they are ready.

@cebe cebe deleted the db-traits branch November 14, 2013 22:19
@qiangxue
Copy link
Member

Yes, let's put these interfaces and traits under yii\db.

For elasticsearch and other DB solutions, I think we should put them directly under yii. If they have external dependencies, we could even consider putting them under yii/extensions.

@qiangxue
Copy link
Member

FYI, I have moved the traits/interfaces back to yii/db.

@klimov-paul
Copy link
Member

I have some concerns:

  • Shouldn’t “ActiveRelationInterface” have a “findWith()” method?
  • It would be nice, if we have “ActiveRecordInterface” (another possible name: “ActiveRelationContainerInterface”). In this case code “ActiveRelationTrait::findWith()” can be unified: see lines 108, 127.

qiansen1386 pushed a commit to qiansen1386/yii2 that referenced this pull request Mar 9, 2014
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

Successfully merging this pull request may close these issues.

5 participants