Improve exception message at ModelManager::batchDelete()
#1734
Conversation
phansys
force-pushed
the
batch_delete
branch
6 times, most recently
from
f33f699 to
ed03b55
Compare
phansys
force-pushed
the
batch_delete
branch
6 times, most recently
from
22bb576 to
4b66cc7
Compare
phansys
force-pushed
the
batch_delete
branch
9 times, most recently
from
15c3ae2 to
85fd814
Compare
| @@ -45,6 +45,8 @@ final class ModelManager implements ModelManagerInterface, LockInterface, ProxyR | |||
| { | |||
| public const ID_SEPARATOR = '~'; | |||
|
|
|||
| private const BATCH_SIZE = 20; | |||
There was a problem hiding this comment.
Could be useful to introduce the size as parameter
public function batchDelete(string $class, BaseProxyQueryInterface $query, int $batchSize = self::DEFAULT_BATCH_SIZE): void
f0004fb to
1ec6d3b
Compare
src/Model/ModelManager.php
Outdated
| { | ||
| if (!$query instanceof ProxyQueryInterface) { | ||
| throw new \TypeError(sprintf('The query MUST implement %s.', ProxyQueryInterface::class)); | ||
| } | ||
|
|
||
| // NEXT_MAJOR: Remove this assignment. |
There was a problem hiding this comment.
You don't need to wait next major.
There was a problem hiding this comment.
I forgot our classes are (finally 😆) final now 🚀
| @@ -345,7 +347,7 @@ public function addIdentifiersToQuery(string $class, BaseProxyQueryInterface $qu | |||
| $qb->andWhere(sprintf('( %s )', implode(' OR ', $sqls))); | |||
| } | |||
|
|
|||
| public function batchDelete(string $class, BaseProxyQueryInterface $query): void | |||
| public function batchDelete(string $class, BaseProxyQueryInterface $query, int $batchSize = self::BATCH_SIZE): void | |||
There was a problem hiding this comment.
shouldn't we add this to the interface? or at least with a next major comment?
There was a problem hiding this comment.
We cant add this to the interface since it wont be bc but yes we can add the next major constant and introduce the constant in the SonataAdminBundle instead
There was a problem hiding this comment.
I was thinking about the constant in SonataAdmin, but I think this must be kept in the scope of each model manager (the behavior regarding the batch size, the way each driver handles the transactions and the cost it demands is specific and not a common concern).
There was a problem hiding this comment.
If we can't add the constant to the SonataAdmin interface, we can't add the batchSize parameter to the interface too.
I'm fine with both solution.
There was a problem hiding this comment.
Then what is the use case for adding this parameter on the function? How are you suposed to pass it on an admin?
There was a problem hiding this comment.
It would be easier to add it to the interface, but even if we don't, people can still pass the batch size as third parameter after a instanceof check.
Subject
I am targeting this branch, because this change respects BC.
Related to sonata-project/SonataDoctrineMongoDBAdminBundle#811 (comment).
Closes #1735.
Changelog