Renaming `Pager::getNbResults` method

sonata-project / SonataAdminBundle

The missing Symfony Admin Generator

https://docs.sonata-project.org/projects/SonataAdminBundle

MIT License

2.11k stars 1.26k forks source link

Renaming `Pager::getNbResults` method #6710

Closed VincentLanglet closed 3 years ago

VincentLanglet commented 3 years ago

Cf https://github.com/sonata-project/SonataAdminBundle/pull/6541#discussion_r532515190 And https://github.com/sonata-project/SonataAdminBundle/pull/6541#discussion_r543306726

@sonata-project/contributors Let's discuss about the name, current options are

count() allows to implements Countable, but how is it clear to know if it's the count of pages or the count of results ?
countResults()

kirya-dev commented 3 years ago

Pager has bugs. Please use Doctrine Paginator

VincentLanglet commented 3 years ago

Pager has bugs. Please use Doctrine Paginator

If there are bugs, report them correctly in an issue. This message doesn't help.
This bundle works with multiple persistence bundle. Not only DoctrineORM.
There is already https://github.com/sonata-project/SonataDoctrineORMAdminBundle/pull/1217

dmaicher commented 3 years ago

count() allows to implements Countable, but how is it clear to know if it's the count of pages or the count of results ?

yeah I also find that confusing. I would vote for countResults.

VincentLanglet commented 3 years ago

count() allows to implements Countable, but how is it clear to know if it's the count of pages or the count of results ?

yeah I also find that confusing. I would vote for countResults.

Do you want to provide the PR @dmaicher ? :)

OskarStark commented 3 years ago

I would also vote 🗳 for countResults()

dmaicher commented 3 years ago

I checked a bit and this will not be sooo easy to deprecate in a clean way on 3.x.

Everywhere we call ->getNbResults() we need to check if the method countResults() exists and if not trigger a deprecation? We also call that method in twig templates :confused:

After all people could have their own PagerInterface implementations.

EDIT: ~~and this also affects setNbResults~~ (seems that is just some hack to test getNbResults()? its protected