-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enhancement: Introduces PagerInterface::isDeterministic
(refs #8164)
#8205
base: 4.x
Are you sure you want to change the base?
Enhancement: Introduces PagerInterface::isDeterministic
(refs #8164)
#8205
Conversation
PagerInterface::isDeterministic
(refs #8164)
@@ -99,4 +99,6 @@ public function setMaxPageLinks(int $maxPageLinks): void; | |||
* Returns the maximum number of page numbers. | |||
*/ | |||
public function getMaxPageLinks(): int; | |||
|
|||
public function isDeterministic(): bool; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good idea but it's a BC break to add a method in an interface.
You have to do like this
SonataAdminBundle/src/Filter/FilterInterface.php
Lines 22 to 25 in 672a614
* @method array getFormOptions(); | |
* @method bool|null showFilter(); | |
* @method array getLabelTranslationParameters(); | |
* @method bool withAdvancedFilter(); |
By just adding a @method
and it will be mandatory in next major.
@@ -186,7 +186,7 @@ file that was distributed with this source code. | |||
<label class="checkbox" for="{{ admin.uniqid }}_all_elements"> | |||
<input type="checkbox" name="all_elements" id="{{ admin.uniqid }}_all_elements"> | |||
{{ 'all_elements'|trans({}, 'SonataAdminBundle') }} | |||
({{ admin.datagrid.pager.countResults() }}) | |||
({{ admin.datagrid.pager.countResults() }}{% if not admin.datagrid.pager.deterministic %}+{% endif %}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the method is not mandatory, we might need an internal twig extension
public function isDeterministic(PagerInterface $pager): bool
{
if (!method_exist($pager, 'isDeterministic')) {
return true;
}
return $pager->isDeterministic();
}
|
||
public function isDeterministic(): bool | ||
{ | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is maybe something to compute, because if we are on the last page and there is no next page, we know the exact number of result and we can return true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds legit. I'll return this dynamically depending on currently known information!
Hi, Do you encounter issue finishing this PR ? |
Hey, I wish to, yes. But I'm really busy at the moment, and I dunno when I have some spare time to fix it (maybe in Dec). |
Subject
This is my suggestion to indicate an enduser that the list doesn't know how many elements ("all elements") will be affected by now. I decided to use a universal
+
sign to indicate it.Default behavior:
Behavior
SimplePager
:I'm not sure if I should add this to
abstract Pager
class without finalizing it. I rather suggest to implementabstract function isDeterministic
and force any implementation of Pager to set it. WDYT? As this is not BC, I didn't choose this path (any custom implementation might be broken after a patch/minor update).This refs #8164 but does not finally closes it. I'll update the way
SimplePager
count results using thresholds in a separate MR. Together they will solve #8164.I am targeting this branch, because it's BC.
Refs #8164
Changelog