search

maximilienGilet / notification-bundle

A simple Symfony bundle to notify user
https://packagist.org/packages/mgilet/notification-bundle
MIT License
107 stars 58 forks source link

Add ability to limit notification fetch #40

Closed emulienfou closed 6 years ago

emulienfou commented 6 years ago

Pull Request for issue #14

LeandroLuccerini commented 6 years ago

What about adding also an offset for pagination?

emulienfou commented 6 years ago

Offset added, suggested by @leandro980 Thanks for the idea

maximilienGilet commented 6 years ago

Great idea !

Thanks

LeandroLuccerini commented 6 years ago

Speaking of pagination, i could work on adding a controller that uses knp-paginator-bundle.

The steps that I will follow are:

  1. Adding a dependency on knp-paginator-bundle to composer;
  2. Adding a method to NotifiableNotificationRepository.php that returns the query needed to the paginator;
  3. Adding a controller (and a route) such as listPaginatedNotificationAction that renders the specific template
  4. Adding a template that renders the list paginated
  5. Adding a new case to the mgilet_notification_generate_path in NotificationExtension.php for the new route

Do you think it should be a good idea?

flavienaudin commented 6 years ago

Hi, I think this is a good idea. But I would like to warn you about a potential bug when using setFirstResult and setMaxResult with join query : http://www.christophe-meneses.fr/article/setmaxresults-limite-le-resultat-de-mes-jointures

Do you think it can occurred with your modification ?

By the way, Thank you for your work

LeandroLuccerini commented 6 years ago

Hi @flavienaudin , knp-paginator uses Doctrine Paginator so i think it's not a problem in this case.