Basic support for asCollection

[samdark]

Q	A
Is bugfix?	no
New feature?	yes
Breaks BC?	no
Tests pass?	yes
Fixed issues	#10806

[SilverFire]

I'd suggest to move this line into a separate protected method. Then it could be overridden with custom logic.
Use case: I want to pass original Query object in my collection as a 2nd argument of the constructor.

[samdark]

Makes sense.

[SilverFire]

Are we going to implement CollectionInterface?

[samdark]

No since we aren't using collections.

[SilverFire]

IMO $models is enough. Method can access $this->collectionClass by itself

[klimov-paul]

Are we going to implement CollectionInterface?
No since we aren't using collections.

I suppose we should create a CollectionInterface and at least a one basic implementation for it.
This is necessary thing to do in order to call this feture 'complete'. Otherwise the entire change should be dropped.

[klimov-paul]

See #12304 (comment)

$className should be optional here, while default collection class is used.

[samdark]

Default is no collection at all. It doesn't make sense to introduce any default collection since the only thing you can achieve w/ collection that you can't achieve otherwise is type safety.

[samdark]

I suppose we should create a CollectionInterface and at least a one basic implementation for it.
This is necessary thing to do in order to call this feture 'complete'. Otherwise the entire change should be dropped.

Nope. First of all, it will prevent using external collection implementations such as Laravel's. Second, there's no requirements except that first argument of the constructor is an array of data.

[klimov-paul]

Default is no collection at all. It doesn't make sense to introduce any default collection since the only thing you can achieve w/ collection that you can't achieve otherwise is type safety.

As I already said then there is no sense to introduce the entire feature. What we are saving here?
Ability to write following code:

$collection = User::find()->asCollection('UserCollection')->all();

insead of:

$collection = new UserCollection(User::find()->all());

Make absolutely no sense to me.

Also what do you mean by type safety?

One more thing how this feature should behave for the Query::one()?

[klimov-paul]

Also collections should be introduced at the level of the Query instead of ActiveQuery.

[samdark]

The implementation here is incomplete proof of concept. You're absolutely correct that primary collection could be filled outside of AR and it's preferred to do so. The difference should be in handling relations...

By type safety I mean in an interface you'll be able to do:

interface X
{
    public function doit(PostCollection $posts);
}

// instead of

interface X
{
    public function doit(array $posts);
}

[samdark]

One more thing how this feature should behave for the Query::one()?

Query::one() should behave exactly the same way as before.

[klimov-paul]

The difference should be in handling relations

In which way it will be different?

By type safety I mean in an interface you'll be able to do:

So how the default collection class existance contradicts this ability? It is like telling the existance of the BaseActiveRecord class prevents you from specify type restiction like following:

interface X
{
    public function doit(PostActiveRecord $post);
}

while it provides more flexisbility to write following type restiction:

interface X
{
    public function doit(BaseActiveRecord $model);
}

Query::one() should behave exactly the same way as before.

In this case asCollection() method makes absolutely no sense, since it affects only single method all(). Another 'finish' method should be introduced instead like collection():

$postCollection = Post::find()->collection();

Note that asArray() method, which was used as prototype here, affects both all() and one() methods. - thus is is a separated method. For collection it make no sense.

[samdark]

In which way it will be different?

Well, relations could be automatically wrapped into collections.

[klimov-paul]

By the way, what about BatchQueryResult?
All thus fuss about collection reminds the concept of data readers. This is actually the only use case, which make sense for me.

I agree with @cebe here: #10806 (comment)

The collection functionality in Yii is split into different classes, providing all necessay support for most common features.

[klimov-paul]

relations could be automatically wrapped into collections.

For what purpose? Just to 'eat' more memory?

[klimov-paul]

For the linking improvement we should use ActiveQuery as I said at #10806 (comment)

$object->getRelationName()->link([$id1, $id2]);

[samdark]

OK. I agree with most of your arguments. Seems it's better to approach it differently via enhancing ActiveQuery and probably writing a recipe in Yii 2.0 cookbook about how to implement simple collections.

[samdark]

https://github.com/samdark/yii2-cookbook/blob/master/book/structure-collections.md

[cebe]

yiisoft/yii2-collection#6