kiwilan / php-opds

PHP package to create OPDS feed (Open Publication Distribution System) for eBooks.
https://packagist.org/packages/kiwilan/php-opds
MIT License
6 stars 2 forks source link

[Bug]: OpdsPaginator is still created when usePaginator is false, and it checks that 'page' URL param is int #30

Closed mikespub closed 1 year ago

mikespub commented 1 year ago

What happened?

When you have your own URL scheme that includes an URL parameter called 'page', and you configure Opds with usePaginator = false, OpdsPaginator is still created, and it still checks the query parameters to see if 'page' is there and is an int.

Hint: it's not (always) for COPS :-)

I would expect php-opds not to create a paginator, or at least not to choke on any URL param it has no business checking out.

Or as a fall-back measure, to be able to specify 'this-param-does-not-exist' in OpdsConfig, as the fake page param it should check out instead.

Hint: COPS uses 'n' as actual page param, so someday it might be nice to be able to configure that in php-opds...

PHP Fatal error:  Uncaught TypeError: Kiwilan\\Opds\\Engine\\OpdsPaginator::__construct(): Argument #8 ($page) must be of type int, string given, called in ./vendor/kiwilan/php-opds/src/Engine/OpdsPaginator.php on line 48 and defined in ./vendor/kiwilan/php-opds/src/Engine/OpdsPaginator.php:15
Stack trace:
#0 ./vendor/kiwilan/php-opds/src/Engine/OpdsPaginator.php(48): Kiwilan\\Opds\\Engine\\OpdsPaginator->__construct()
#1 ./vendor/kiwilan/php-opds/src/Engine/OpdsEngine.php(245): Kiwilan\\Opds\\Engine\\OpdsPaginator::make()
#2 ./vendor/kiwilan/php-opds/src/Engine/OpdsJsonEngine.php(68): Kiwilan\\Opds\\Engine\\OpdsEngine->paginate()
#3 ./vendor/kiwilan/php-opds/src/Engine/OpdsJsonEngine.php(23): Kiwilan\\Opds\\Engine\\OpdsJsonEngine->feed()
#4 ./vendor/kiwilan/php-opds/src/Opds.php(122): Kiwilan\\Opds\\Engine\\OpdsJsonEngine::make()
#5 ./lib/Output/KiwilanOPDS.php(191): Kiwilan\\Opds\\Opds->get()
#6 ./opds.php(47): SebLucas\\Cops\\Output\\KiwilanOPDS->render()
#7 {main}
thrown in ./vendor/kiwilan/php-opds/src/Engine/OpdsPaginator.php on line 15

How to reproduce the bug

Try OPDS feed with URL parameter ?page=index

Package Version

1.0.1

PHP Version

8.2

Which operating systems does with happen with?

Linux

Notes

Not sure what it actually does with an 'int' page, but it doesn't seem to bother it after that...

ewilan-riviere commented 1 year ago

Can you tell me if it works now?

I add a new property to OpdsConfig with paginationQuery to override page default query parameter and I fix this bug for pagination.

Thanks for finding this bug!

mikespub commented 1 year ago

Yes, that fixed it all right - thanks!

I haven't tried out the paginationQuery yet, but I do appreciate it for later :-)

I haven't quite figured out yet if & how to use any built-in capability of php-opds on this, to be honest. Given that most COPS users seem to have somewhere around 5.000 - 10.000 books in their Calibre database, using limit on the database side is almost mandatory, so by the time I get to pass along the feeds to php-opds they're already pre-paginated.

So do I pass along pagination links myself too, or do I let php-opds take care of that for me, I haven't decided yet. It might generate another PR once I try it out though ;-)

ewilan-riviere commented 1 year ago

My database of books is smaller so I may lack the perspective of a larger database. If you see that php-opds is out of step with the needs of a large database, don't hesitate to suggest ideas ;)

mikespub commented 1 year ago

It's a nice idea to provide this capability out of the box, don't get me wrong. And for smaller data sets, it's certainly a nice feature to have. I just don't see how it would fit in cases where you need to pre-select or paginate data elsewhere in your app, before feeding it to php-opds.

Even if you used some kind of iterator, callable or async promise in your feeds() call, the actual selection of a "page" of data would still be done outside of php-opds, no? So how can php-opds make life easier - not sure yet...