Skip to content
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

deprecate PagerInterface::getNbResults() in favour of countResults() #6732

Merged
merged 10 commits into from
Jan 6, 2021

Conversation

dmaicher
Copy link
Contributor

@dmaicher dmaicher commented Jan 3, 2021

Subject

I am targeting this branch, because its BC.

Closes #6710

Changelog

### Added
- Added `PagerInterface::countResults`

### Deprecated
- `PagerInterface::getNbResults()` in favour of `countResults()`
- `PagerInterface::setNbResults(...)`
- `PagerInterface::$nbResults`

@dmaicher dmaicher force-pushed the issue-6710 branch 3 times, most recently from e6090d0 to 913b9b0 Compare January 3, 2021 14:38
@dmaicher dmaicher marked this pull request as ready for review January 3, 2021 14:42
Copy link
Member

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I saw the implementation of the getNbResult/SetNbResult I start to think that we should

  • Deprecate getNbResult, setNbResult like you did ; but also nbResult.
  • Not implementing countResults in the abstract Pager

Then, every Pager will have his own implementation.

  • SimplePager have a countResults without the need of the property or the setter.
  • DoctrineORM may use a property and a setter to cache the value
  • Another Persistence bundle can do it differently.

WDYT ?

src/Datagrid/PagerInterface.php Show resolved Hide resolved
src/Action/SearchAction.php Show resolved Hide resolved
src/Action/SearchAction.php Show resolved Hide resolved
src/Datagrid/Pager.php Show resolved Hide resolved
src/Datagrid/Pager.php Show resolved Hide resolved
src/Resources/views/Block/block_search_result.html.twig Outdated Show resolved Hide resolved
tests/Datagrid/PagerTest.php Show resolved Hide resolved
@VincentLanglet VincentLanglet requested a review from a team January 3, 2021 16:39
@dmaicher dmaicher requested review from VincentLanglet and removed request for a team January 4, 2021 17:26
return $this->getMaxPerPage() && $this->getNbResults() > $this->getMaxPerPage();
// NEXT_MAJOR: remove the existence check and the else part
if (method_exists($this, 'countResults')) {
$countResults = (int) $this->countResults();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$countResults = (int) $this->countResults();
$countResults = $this->countResults();

This is not needed, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/Datagrid/Pager.php Outdated Show resolved Hide resolved
tests/Datagrid/PagerTest.php Show resolved Hide resolved
tests/Datagrid/PagerTest.php Outdated Show resolved Hide resolved
@dmaicher dmaicher force-pushed the issue-6710 branch 3 times, most recently from 411f844 to c190a3d Compare January 4, 2021 20:59
Copy link
Member

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All we need now is to update UPGRADE-3.x.md to list the deprecated things and to explain that countResults should be implemented

VincentLanglet
VincentLanglet previously approved these changes Jan 5, 2021
@VincentLanglet VincentLanglet requested review from OskarStark and a team January 5, 2021 18:32
UPGRADE-3.x.md Outdated Show resolved Hide resolved
src/Action/SearchAction.php Show resolved Hide resolved
tests/Datagrid/PagerTest.php Outdated Show resolved Hide resolved
tests/Datagrid/PagerTest.php Show resolved Hide resolved
Co-authored-by: Javier Spagnoletti <[email protected]>
@VincentLanglet VincentLanglet requested a review from phansys January 5, 2021 20:30
Copy link
Member

@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @dmaicher 🤗

@OskarStark OskarStark merged commit 5f66180 into sonata-project:3.x Jan 6, 2021
@dmaicher dmaicher deleted the issue-6710 branch January 6, 2021 08:29
@franmomu
Copy link
Member

franmomu commented Jan 6, 2021

thanks @dmaicher, can you please take also a look at these calls? https://github.com/sonata-project/SonataAdminBundle/search?l=Twig&q=NbResults

@dmaicher
Copy link
Contributor Author

dmaicher commented Jan 6, 2021

thanks @dmaicher, can you please take also a look at these calls? https://github.com/sonata-project/SonataAdminBundle/search?l=Twig&q=NbResults

Ah I missed those 😕 Will check later today and create a follow-up PR 👍

@michanismus
Copy link

michanismus commented Jan 12, 2021

DoctrineORMAdminBundle Datagrid Pager has a missing method: countResult

@dmaicher
Copy link
Contributor Author

dmaicher commented Jan 12, 2021

DoctrineORMAdminBundle Datagrid Pager has a missing method: countResult

@michanismus this was fixed but not released yet: sonata-project/SonataDoctrineORMAdminBundle#1257

@michanismus
Copy link

@dmaicher I saw, but not merged into 4.x-dev yet...
I also opened an issue in ORM repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Renaming Pager::getNbResults method
6 participants