Yii2: destroy() method in ActiveRecord

[MaksimKiselev]

How about add destroy() method in ActiveRecord?

Record::destroy(/* where condition */);

// OR

Record::destroy([1, 2, 3]);

instead:

$models = Record::find()->where(/* where condition */)->all();

foreach($models as $model) {
    $model->delete();
}

[allowing]

Good. I like it.
Laravel have a lot of method like this above.

[Locustv2]

Yii2 already have the function ActiveRecord::deleteAll($condition, $params)

http://www.yiiframework.com/doc-2.0/yii-db-activerecord.html#deleteAll()-detail

[MaksimKiselev]

@Locustv2 no, deleteAll() execute query without trigger events:

Note that this method will not trigger any events. If you need EVENT_BEFORE_DELETE or EVENT_AFTER_DELETE to be triggered, you need to find the models first and then call delete() on each of them.

[Locustv2]

You did not say anything about events. And any reason why you require events?
What is the scenario?

[MaksimKiselev]

@Locustv2

You did not say anything about events.

I pasted code, people with expirience understood what I'd mean.

And any reason why you require events?

O_o Events it's very popular practice.

What is the scenario?

For example after delete record delete some files in file storage.

Also many behaviors subscribe to Record events!

[Alex-Bond]

Maybe better to do like this: ActiveRecord::deleteAll($condition, $params,boolean $triggerEvents)?

[samdark]

@Alex-Bond no, it's not a good idea to do quite different things via single method.

[samdark]

How about naming it deleteEachByPks(array $pks)?

[Alex-Bond]

@samdark how about Record::find()->where(/* where condition */)->deleteAll(boolean $iterate);?

[MaksimKiselev]

@samdark hm... In most popular AR implements(RoR, Eloquent) method named and signature as destroy(by_id), destroy_all(by_condition) and destroy(by_pks).

I think that's good idea for use the same name and signature.

[samdark]

Destroy doesn't mean anything special to me. It's a synonym of "delete".

[rob006]

We could return models collection in all() instead of simple array. Then we could do

Model::find()->where(/**/)->all()->delete();

Or just:

Model::findAll()->delete();

[samdark]

That's a good argument for collections support.

[klimov-paul]

Why do not use ActiveRecord::deleteAll()?

[cebe]

Why do not use ActiveRecord::deleteAll()?

because:

no, deleteAll() execute query without trigger events:

[klimov-paul]

For me executing an SQL query (even such simple as delete) inside the cycle is a bad practice, which should be avoided.
I do not consider such rare use case to be worth additional methods or classes introduction.

[samdark]

It's not that rare. Deleting multiple models where each has its own cleanup sequence is quite common.

[klimov-paul]

Lets start from the beginning: you suggest following code:

Record::destroy(/* where condition */);

instead of following one:

foreach (Record::findAll(/* where condition */) as $record) {
    $record->delete();
}

So we are planning to introduce separated method or even a whole ActiveRecordCollection class to save 2 lines of code, one of which is closing bracket?

[klimov-paul]

It's not that rare. Deleting multiple models where each has its own cleanup sequence is quite common.

In the scope of all ActiveRecord methods invocation this sequence is rare - it can be used at some background clean-up script, but it is unlikely to be used at every page rendering. That is why I do not consider eliminating 2 lines of code worth the extra entities or methods creation.

[samdark]

So we are planning to introduce separated method or even a whole ActiveRecordCollection class to save 2 lines of code, one of which is closing bracket?

In fact, yes :) Got your point. Need more opinions.

[rob006]

It is actually more than 2 lines of code if you have more complex conditions. It is even more complicated if you want to use transactions or use number of removed models in some condition. And the goal is not to save 2 lines of code, but create more elegant and readable syntax for batch operations on models.

Collections can be useful not only for deleting records. Using collection is a natural way of handling tabular input including creating, validating and updating data.

[klimov-paul]

It is actually more than 2 lines of code if you have more complex conditions. It is even more complicated if you want to use transactions or use number of removed models in some condition.

So we are planning to create a sophisticated options paratemeter for destroy() method, which can determine whether to use transactions or not, specify callback, which will collect the number of processed records and so on?
Why such complex solution is better then a simple cycle statement, which can be adjusted by any additional logic at will?

[rob006]

I don't really see the problem (note that I'm talking about collections not ::destroy() method).

$removedCount = Model::findAll(['status' => Model::STATUS_TRASH])->useTransaction()->delete();

Or

while (Model::find()->each(100)->useTransaction()->delete() === 100) {
    // some stuff
}

Please propose alternative with max 2 extra lines of code.

[klimov-paul]

Here is a full code I use for the task:

class RemoveTempImageController extends \yii\console\Controller
{
    public function actionIndex()
    {
       $this->stdout("Removing temporary image files...\n");

        $query = TempImage::find()
            ->isExpired()
            ->notConfirmed();

        $recordCount = $query->count();

        if ($recordCount > 0) {
            $this->stdout("Total records to be processed: {$recordCount}\n");

            $processedRecordCount = 0;
            foreach ($query->batch(100) as $records) {
                foreach ($records as $record) {
                    $record->delete();
                    $processedRecordCount++;
                }

                gc_collect_cycles(); // ensure garbage collection of ActiveRecord+Behavior cycles

                $this->stdout("Records processed: {$processedRecordCount}/{$recordCount}\n");
            }
        } else {
            $this->stdout("There is no records to be processed.\n");
        }

        $this->stdout("...done\n");
    }
}

It includes batch processing, progress log and garbage collection.

I can not see how it could be wrapped into ActiveRecordCollection. There too many options and possibilities to be standartized. For example: what about your useTransaction() option - should it enclose the entire processing into a single transaction or just a one batch processing or each delete call?

[rob006]

For example: what about your useTransaction() option - should it enclose the entire processing into a single transaction or just a one batch processing or each delete call?

Transaction enclose Collection::delete() method (one batch).

Most of the problems can be solved by events and before*()/after*() methods, but obviously collections will not magically solve all you problems, same as AR will no solve all your problems with DB operations. There are always be a cases when foreach will be required, but even in you example collection and batch delete will save you 6-7 lines of code (more if you want transaction).

[klimov-paul]

Transaction enclose Collection::delete() method (one batch).

Are you so sure that noone will never need any other transaction level? I can see at least 3 different possibilities for transaction encosure.

Most of the problems can be solved by events and before()/after() methods

So now we are entroducing an events... And you are saying that this solution simplifies anything - I doubt it. How code with event handler specifications is better then obvious foreach operator?

Also implementing such feature will end up with endless improvement requests like 'I wish logging in collection batch processing' or 'I wish to use transactions at other level' and so on. Such solution can never be complete.

There are always be a cases when foreach will be required, but even in you example collection and batch delete will save you 6-7 lines of code (more if you want transaction).

Saving lines of code by any cost is not a healthy practice. Less lines by cost of less readability does not looks fine for me. Using foraech creates an obvious code, which is easy to comprehend, while using abstract class with event handlers looks like overkill.

There are always be a cases when foreach will be required

So we plan to provide solution even for less use cases then I suggested: if I wish to create clean-up console script - I should use foreach anyway. So which use case we are soling now? Why do you think it is common enough for all the trouble?

[rob006]

Are you so sure that noone will never need any other transaction level? I can see at least 3 different possibilities for transaction encosure.

What levels? If you want transaction for single model delete() you should implement it at model level. If you want transaction for all models processed in collection, you should use transaction at collection level (Collection::useTransaction()). If you want one transaction for all models processed in your command, you should implement it in your command.

if I wish to create clean-up console script - I should use foreach anyway

No, you don't. In your example you can use batch delete from collection, which will simplify your code.

And all your arguments you can also refer to Active Record and Query Builder - because "simple update query is more obvious and is easy to comprehend" and "implementing such feature will end up with endless improvement requests".
I don't really think this discussion is going anywhere. If you don't want use collections features, nobody will force you to do that - you can always iterate each collection as a regular array. Let's rest of the team and community decide whether it is worth the effort.

[klimov-paul]

And all your arguments you can also refer to Active Record and Query Builder - because "simple update query is more obvious and is easy to comprehend" and "implementing such feature will end up with endless improvement requests".

No, it can not: QueryBuilder encapsulates SQL composition solving problem of inconsistencies between different SQL dialects, while ActiveRecord provide the model abstraction level including populating from request and validation. Thier existance is necessary, while creation of destory method is just a wrapping of obvious code.

No, you don't. In your example you can use batch delete from collection, which will simplify your code.

Very well, then rewrite the code I provided with Collection usage in the way its original functionality reamins the same.

[rob006]

class RemoveTempImageController extends \yii\console\Controller
{
    public function actionIndex()
    {
        $this->stdout("Removing temporary image files...\n");

        $query = TempImage::find()
            ->isExpired()
            ->notConfirmed();

        $recordCount = $query->count();

        if ($recordCount > 0) {
            $this->stdout("Total records to be processed: {$recordCount}\n");

            $processedRecordCount = 0;
            while ($count = $query->batch(100)->delete()) {
                gc_collect_cycles(); // ensure garbage collection of ActiveRecord+Behavior cycles

                $processedRecordCount += $count;
                $this->stdout("Records processed: {$processedRecordCount}/{$recordCount}\n");
            }
        } else {
            $this->stdout("There is no records to be processed.\n");
        }

        $this->stdout("...done\n");
    }
}

}

ActiveRecord provide the model abstraction level including populating from request and validation

Collections will do the same for tabular data. It just give you similar syntax for processing multiple models as single model has.

[klimov-paul]

while ($count = $query->batch(100)->delete()) {

I would expect the ActiveRecordCollection iterate query as a batch internally to save performance -
otherwise it can not be used for large data set, which is meant to.
In collection scope I would expect following code:

$collection->batch(100)->delete()

To iterate all query results using batch size 100 and delete them instead of processing a single batch only.
In your example Collection::delete() is actually wrap a single foreach with delete() without much purpose. All improvent over it you propose is a transaction wrapping.

[rob006]

There is no $collection->batch(100)->delete() - it is $query->batch(100)->delete(). Collection is returned by batch().

I'm not really sure how do you want implement collections at query level.

[klimov-paul]

Because iterationg several thousands of records impossible in ActiveRecord scope.
I can justify batch (collection) ActiveRecord level processing only in one case - if it uses batch iteration allowing processing of any amount of objects. Only such work-flow can be considered complex enough to be encapsulated into separated entity.

[klimov-paul]

I'm not really sure how do you want implement collections at query level.

That is because you do not understand what is ActiveRecord collection in Laravel and ActiveQuery in Yii.
Collection functionality in Yii is split between ActiveQuery and DataProvider.
We already have a disscussion about ActiveRecord Collections at #10806.

You can easily create deleteAll() method in ActiveQuery related to your ActiveRecord to achieve your goal:

class TempFileQuery extends \yii\db\ActiveQuery
{
    public function deleteAll()
    {
         $recordCount = 0;
         $transaction = $this->getDb()->beginTransaction();
         try {
             foreach ($query->batch(100) as $records) {
                 foreach ($records as $record) {
                     $record->delete();
                     $recordCount++;
                 }
             }
         } catch (\Exception $e) {
             $transaction->rollback();
             throw $e;
         }
         return $recordCount;
    }
}

You may compose this code into a trait and use it at will.

There is no obstacle for you to use the syntax you wish within the framework:

TempFile::find()->whereExpired()->deleteAll();

Through all disscussions here and at #10806 there is no solid argument, which justifies introduction of ActiveRecordCollection class in Yii besides 'because Laravel is doing so'.

[Skinka]

Make ActiveRecordHelper :)

$models = ActiveRecord::find()->all();
ActiveRecordHelper::deleteRecords($models);

[cebe]

@Skinka There should not be the need for a framework method if it contains a simple foreach over all records calling a single method on them.

@ all In general I also do not really see the benefit of a method that limits the use case to a common case, while a manual foreach or a custom ActiveQuery method could be much more flexible.

foreach(TempFile::find()->whereExpired()->all() as $file) {
    $file->delete();
}

can be extended in many ways, e.g. transactions, custom error handling, etc... this all is often needed but not possible with a method that captures that foreach in the framework.
Here are some possible usages, that are easy when doing the foreach yourself but become hard to do if you want to use a method provided by the framework:

For being able to handle a lot of records:

foreach(TempFile::find()->whereExpired()->each() as $file) {
    $file->delete();
}

With transaction (could also be moved inside the foreach to cover what is done inside of delete() and events):

TempFile::getDb()->transaction(function($db) {
    foreach(TempFile::find()->whereExpired()->each(100, $db) as $file) {
        $file->delete();
    }
}

With error handling:

foreach(TempFile::find()->whereExpired()->each(100, $db) as $file) {
    try {
        $file->delete();
    } catch(\Exception $e) {
        Yii::error('failed to cleanup file:' . $file->name);
    }
}

...

[Skinka]

@cebe I understand, so a person is just too lazy to write 3 lines of code

[samdark]

Makes sense.

[cebe]

https://github.com/yiisoft/yii2-collection/issues/5