Closed zf2timo closed 3 years ago
kirya-dev
commented
4 years ago Please ensure that registered extension AdminEventExtension:
https://github.com/sonata-project/SonataAdminBundle/blob/be653e2d320717f958ba8653464e9732bf463aaa/src/Event/AdminEventExtension.php#L112-L118
Or use doctrine remove events
VincentLanglet
commented
4 years ago You're right,
The deleteAction is calling $this->admin->delete() which calls modelManager->delete and trigger the event.
The batchDeleteAction is calling directly $this->modelManager->batchDelete().
What did you have in mind to fix this ?
zf2timo
commented
4 years ago @VincentLanglet I would use the query to load all objects. These can then be passed to the delete method of the AbstractAdmin. From here also the pre and post delete event is fired.
VincentLanglet
commented
4 years ago @VincentLanglet I would use the query to load all objects. These can then be passed to the delete method of the AbstractAdmin. From here also the pre and post delete event is fired.
Then we would stop to use the modelManager batchDelete method https://github.com/sonata-project/SonataAdminBundle/blob/3.x/src/Controller/CRUDController.php#L177
I assume the bacthDelete method was a delete method optimized so I'm not sure it's a good idea to use multiple times the delete method: https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/3.x/src/Model/ModelManager.php#L221 instead of the batchDelete one: https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/3.x/src/Model/ModelManager.php#L537
Best would be to find another solution. cc @sonata-project/contributors
phansys
commented
4 years ago I agree that removing objects in a batch operation should behave like any single remove operation (including the extension logic, etc).
Maybe we could let ModelManager::batchDelete() to return a generator, including the models that are successfully removed within a batch cycle, in order to let the caller to iterate over these results and fire required events. I know this approach should require more elaboration, but I let it here for discussion.
kirya-dev
commented
4 years ago What about this?
public function batchActionDelete(ProxyQueryInterface $query)
{
$this->admin->checkAccess('batchDelete');
$modelManager = $this->admin->getModelManager();
try {
foreach($modelManager->getObjectsToBatchDeleting($query) as $object) {
$this->admin->delete($object);
}
$this->addFlash(
'sonata_flash_success',
$this->trans('flash_batch_delete_success', [], 'SonataAdminBundle')
);
} catch (ModelManagerException $e) {
$this->handleModelManagerException($e);
$this->addFlash(
'sonata_flash_error',
$this->trans('flash_batch_delete_error', [], 'SonataAdminBundle')
);
}
return $this->redirectToList();
}What about this?
What you're proposing doesn't seem to be a batch operation, since delete() is called individually for each object.
kirya-dev
commented
4 years ago is called individually for each object.
This is exactly what we need!
kirya-dev
commented
4 years ago @phansys says any time ago:
batch operation should behave like any single remove operation (including the extension logic, etc)
And now you say other solve.
phansys
commented
4 years ago This is exactly what we need!
AFAIK, we need to trigger the events associated to the remove operation for the removed objects, not commit the transaction for each single object.
kirya-dev
commented
4 years ago Run deleting transactions its task of ModelManager
phansys
commented
4 years ago Run deleting transactions its task of ModelManager
I agree, and I'm not going against that premise.
kirya-dev
commented
4 years ago ModelManager and now run deleting for each objects as single query (using remove & flush every 20 iteration)
No difference if we run flush every 20 iterations or every 1.
Difference in call clear of ObjectManager - free a memory
So, im think my code suggeston have a chance to live.
In my example getObjectsToBatchDeleting($query) has iterator & clear manager logic every 20 iterations.
Im would like take attention that if call postRemove when deletion transation not executed - wrong behavior
I, as an ordinary user, expect that deleting many objects is equivalent to deleting each one separately.
VincentLanglet
commented
4 years ago You can edit a message instead of posting 6 consecutive messages.
In my example
getObjectsToBatchDeleting($query)has iterator & clear manager logic every 20 iterations.
If we decide to lose the benefit of a batch operation, we can just do
$objects = $selectedModelQuery->execute();
foreach ($objects as $object) {
$this->admin->delete($object);
}And we could deprecate the BatchDelete method from ModelManagerInterface.
phansys
commented
4 years ago In case we keep the benefit of the batch operation, this is an example about what I've mentioned before: https://3v4l.org/HjjWH.
kirya-dev
commented
4 years ago Benefit in run flushing and crearing every 20 iterations? Hmm. You know that is useless? Database lock query results in your operation memory in any way? Calling clear clears only php application memory of extra entities.
Same problem here with exporting: https://github.com/sonata-project/exporter/issues/347
Im see one really benefit in clearing.
In case we keep the benefit of the batch operation, this is an example about what I've mentioned before: https://3v4l.org/HjjWH.
Im would like see that postRemove was executed after entity was realy deleted from DB. In can check here on rows count in my needs (for checking that left less than 100 rows and stop delition)
VincentLanglet
commented
4 years ago Benefit in run flushing and crearing every 20 iterations? Hmm. You know that is useless? Database lock query results in your operation memory in any way? Calling
clearclears only php application memory of extra entities.
I won't say it's useless when it's in the doctrine documentation: https://www.doctrine-project.org/projects/doctrine-orm/en/2.7/reference/batch-processing.html#bulk-deletes If you still think so, you should tell them directly.
PeterZhukov
commented
3 years ago Idea: may be batch operations should trigger other event, like "batchDelete" where you receive all ids, and then decice what to do with them.
github-actions[bot]
commented
3 years ago This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
Environment
PHP FPM Image with separated NGINX
Sonata packages
Symfony packages
PHP version
Subject
When a batch delete is executed, there is no event fired for the Document.
Steps to reproduce
Register an delete Event
Expected results
The
onImageDeletefunction should be executed - even in Batch mode.Actual results
Only the Document is deleted, but without executing the registered Event handler.
I can create a pull request with a fix, if you want.