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

Restored pagerfanta/pagerfanta dependency #1912

Merged
merged 2 commits into from May 28, 2023

Conversation

Arconian
Copy link
Contributor

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.

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.
@XWB
Copy link
Member

XWB commented May 26, 2023

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

@mbabker
Copy link
Contributor

mbabker commented May 26, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@Arconian Arconian changed the title Moved pagerfanta adapters to "required" section of the composer.json Restored pagerfanta/pagerfanta dependency May 26, 2023
@XWB XWB merged commit 6abea64 into FriendsOfSymfony:master May 28, 2023
19 checks passed
@XWB
Copy link
Member

XWB commented May 28, 2023

Thanks @Arconian

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

Successfully merging this pull request may close these issues.

Class QueryAdapter not found after updating to v6.3.0
3 participants