FriendsOfSymfony / FOSElasticaBundle

Elasticsearch PHP integration for your Symfony project using Elastica.
http://friendsofsymfony.github.io
MIT License
1.24k stars 795 forks source link

Restored pagerfanta/pagerfanta dependency #1912

Closed Arconian closed 1 year ago

Arconian commented 1 year ago

Since adapters are needed for the functionality they should be installed automatically otherwise exceptions are thrown.

Feel free to close the PR if that was intentional and we have to specify the adapters we need from now on manually. This should be mentioned in the documentation and in the CHANGELOG though.

Fixes #1911.

XWB commented 1 year ago

Alternatively we could restore the pagerfanta/pagerfanta dependency and make the split in the next major release?

mbabker commented 1 year ago

Either go back to requiring the mono-package or do nothing.

Making these packages hard requirements also means their transient dependencies become hard requirements, meaning all of doctrine/orm, doctrine/mongodb-odm, and doctrine/phpcr-odm would be required to be installed.

The mono-package doesn't require anything but ext-json, and the Pagerfanta code has no runtime checks to make sure the sub-package dependencies are met (so someone could composer require pagerfanta/pagerfanta and try to use Pagerfanta\Doctrine\ORM\QueryAdapter without installing doctrine/orm). Using the sub-packages gives proper Composer version constraints to prevent installing incompatible things, the mono-package only enforces lower boundaries with no upper boundaries (so unsupported next majors are installable).

So it's really a matter of trade-offs I guess, install a mono-package with no guarantee that its transient dependencies are met or improve the documentation and runtime checks to inform users how to handle dependencies for optional features (since this package doesn't require doctrine/orm either, yet there's nothing in FOS\ElasticaBundle\Doctrine\ORMPagerProvider warning about any of its missing dependencies).

Arconian commented 1 year ago

Alternatively we could restore the pagerfanta/pagerfanta dependency and make the split in the next major release?

This is also a possibility. I personally very much prefer to reduce the amount of dependencies when possible and from my understanding these three adapters are the only ones that needed. So why not get rid of other 5? This won't be a breaking change.


 The following first party packages are available to include additional functionality:

pagerfanta/doctrine-collections-adapter: Provides a pagination adapter for [Doctrine\Common\Collections\Collection](https://www.doctrine-project.org/projects/collections.html) and Doctrine\Common\Collections\Selectable implementations
pagerfanta/doctrine-dbal-adapter: Provides support for the [Doctrine DBAL](https://www.doctrine-project.org/projects/dbal.html) package
pagerfanta/doctrine-mongodb-odm-adapter: Provides support for the [Doctrine MongoDB ODM](https://www.doctrine-project.org/projects/mongodb-odm.html) package
pagerfanta/doctrine-orm-adapter: Provides support for the [Doctrine ORM](https://www.doctrine-project.org/projects/orm.html) package
pagerfanta/doctrine-phpcr-odm-adapter: Provides support for the [Doctrine PHPCR ODM](https://www.doctrine-project.org/projects/phpcr-odm.html) package
pagerfanta/elastica-adapter: Provides support for [Elastica](https://elastica.io/) (an ElasticSearch PHP client)
pagerfanta/solarium-adapter: Provides support for [Solarium](https://github.com/solariumphp/solarium) (a Solr search client)
pagerfanta/twig: Provides support for [Twig](https://twig.symfony.com/)```
Arconian commented 1 year ago

Oh yes valid point by @mbabker. Then mono-package is the way to go and a split in the next major.

XWB commented 1 year ago

Thanks @Arconian