Fix pagerfanta requirements #415

gseidel · 2022-04-08T10:15:23Z

Q	A
Bug fix?	yes
BC breaks?	no
Deprecations?	no
License	MIT

Since version 1.8.3 the requirement babdev/pagerfanta-bundle was upgraded to version ^3.0. But this version requires pagerfanta/core instead of pagerfanta/pagerfanta. So in the SyliusResourceBundle we have missing classes like Pagerfanta\Adapter\DoctrineORMAdapter which is not part of the pagerfanta core. This leads to no more working code if people don't install pagerfanta/pagerfanta their own. This PR will fix this requirements.

mbabker · 2022-04-08T12:13:13Z

If you’re going to add anything to the requirements, it really should just be the pagerfanta/doctrine-orm-adapter package and not the pagerfanta/pagerfanta monorepo (using the split packages installs a lot less code and has better dependency constraints than using the monorepo can ever have).

gseidel · 2022-04-08T12:18:35Z

Thanks mbabker. I was not aware of the pagerfanta packages. To just require the needed package make a lot more sense. PR is updated

gseidel · 2022-04-08T12:40:14Z

After checking the code I found other dependencies requiring speciefied subpackage of pagerfanta

pagerfanta/doctrine-mongodb-odm-adapter
pagerfanta/doctrine-phpcr-odm-adapter
pagerfanta/twig

For classes:

Pagerfanta\Doctrine\MongoDBODM\QueryAdapter
Pagerfanta\Doctrine\PHPCRODM\QueryAdapter
Pagerfanta\Twig\Extension\PagerfantaExtension

The main problem is that the adapter classes will also install further packages, which pagerfanta/pagerfanta doesn't

I would suggest to revert my commit and use pagerfanta/pagerfanta for the branch 1.8 and in other versions where we could make a BC break, we can use specified pagerfanta packages.

I can remeber that the resource-bundle will drop Mongo and CR support in higher versions. So we can easily add the orm package then. Otherwise a user have to install a pagerfanta package himself, when he will install the resource-bundle

loic425 · 2022-04-08T13:33:58Z

I think to avoid bc-break we can move pagerfanta/pagerfanta on main requirements on 1.8 & 1.9.
we can handle the "optional packages" on 1.10
WDTY @Zales0123?

on future grid bundle release, we already have this (not released yet)
https://github.com/Sylius/SyliusGridBundle/blob/master/src/Bundle/Doctrine/ORM/DataSource.php#L71

mbabker · 2022-04-08T14:00:12Z

The problem here is no different than Symfony itself with its optional dependencies. Actually, the Pagerfanta and Symfony monorepos are kind of the same in that they don’t install all of the optional dependencies and users are still forced to make sure the full dependency stacks are in place (the Pagerfanta monorepo doesn’t install any of the Doctrine packages or Twig, but the subsplits do have those dependencies declared).

Using the monorepo is fine, but from a downstream consumer perspective, it will cause issues by not installing all required transient dependencies and it won’t adequately lock version ranges (you can install the ORM 3.0 branch with the monorepo and it won’t raise a flag, but do the same with the ORM adapter package and you’ll get Composer warnings).

Short term, using the monorepo might “fix” any immediate issues, but long term folks really should stop using it.

mbabker · 2022-04-08T15:06:20Z

Hmm, I didn't realize another 1.8 release was made alongside 1.9 and that the Pagerfanta major version bump was included in that. #381 IMO should be reverted from 1.8, having a major version bump on a dependency in a patch release seems like it's asking for trouble. If anything, 4404859 (which was the initial version of #298 before it was changed to 3.0 only) should be used which allows both Pagerfanta 2.x and 3.x to be installed.

mbabker · 2022-04-08T16:02:07Z

src/Bundle/Tests/DependencyInjection/Compiler/PagerfantaBridgePassTest.php

@@ -26,7 +25,7 @@ final class PagerfantaBridgePassTest extends AbstractCompilerPassTestCase
     */
    public function it_creates_aliased_services_and_changes_the_view_factory_class(): void
    {
-        $this->registerService('pagerfanta.twig_extension', PagerfantaExtension::class);
+        $this->registerService('pagerfanta.twig_extension', class_exists('Pagerfanta\Twig\Extension\PagerfantaExtension') ? 'Pagerfanta\Twig\Extension\PagerfantaExtension' : 'BabDev\PagerfantaBundle\Twig\PagerfantaExtension');


You can always use Pagerfanta\Twig\Extension\PagerfantaExtension here. Since the bundle's 2.5 release, that's the class used for the pagerfanta.twig_extension service, and with the version constraints given, it'll always be available.

lchrusciel · 2022-04-11T07:22:00Z

Hey folks,

thanks a lot for your contribution @gseidel. Your first in this repository! I wish it would be in better circumstances ;)

But coming back to the implementation itself, I fail to see how this change would help you fix your repository. According to the change, a composer will try to solve the highest possible dependencies, so you still will have to define the proper pagerfanta version, or am I wrong?

it seems, that the only revert proposed by @mbabker will solve 1.8 compatibility issues. Next question is, how broke is the 1.9 release.

This PR was merged into the 1.8 branch. Discussion ---------- Reverts #381 Alternative solution for #415 Commits ------- 7d0f335 Revert "Bump Pagerfanta from 2.x to 3.x"

gseidel · 2022-04-11T10:48:20Z

Hello @lchrusciel. Don't worry about the circumstances, I using this bundle for a long time already and finally I can contribute ;)

This PR will fix two cases. My case is, that I can upgrade to pagerfanta-bundle version 3.* but if I upgrade a bug fix version I would not except to manully add a package. So I moved the pagerfanta/pagerfanta from dev to requirement.

On the other hand, their may be people who can't upgrade to version 3.*, because they have an other package which stick to version 2.* and the bump version will fail for them, because they can't resolve the dependency graph. So I follow @mbabker suggestion and allow version 2.* as well.

lchrusciel · 2022-04-11T12:00:10Z

Ah, now I see. So the first line of fix will be to revert our 1.8 change. From what I know, @loic425 is in favour of your solution as well for 1.9, but that can be done in a little bit slower pace :)

lchrusciel · 2022-04-11T12:24:40Z

@gseidel can you check your app with https://github.com/Sylius/SyliusResourceBundle/releases/tag/v1.8.4?

lchrusciel · 2022-04-11T12:33:52Z

this PR should be rebased to 1.9 after #417 merge

gseidel · 2022-04-12T08:38:22Z

@gseidel can you check your app with https://github.com/Sylius/SyliusResourceBundle/releases/tag/v1.8.4?

Yes it works, thank you!

gseidel · 2022-04-12T08:48:30Z

I close this PR and rebase it to branch 1.9. See #420

gseidel requested a review from a team as a code owner April 8, 2022 10:15

mbabker reviewed Apr 8, 2022

View reviewed changes

mbabker approved these changes Apr 8, 2022

View reviewed changes

lchrusciel mentioned this pull request Apr 11, 2022

Revert "Bump Pagerfanta from 2.x to 3.x" #416

Merged

gseidel added 4 commits April 11, 2022 12:35

Fix pagerfanta requirements

2afaf0b

Use doctrine-orm-adapter package instead of monorep

6f80ea7

Allow babdev/pagerfanta-bundle 2.5 version again to avoid bc breaks

d9233f8

Use always PagerfantaExtension from Pagerfanta

4d9082f

gseidel force-pushed the fix-pagerfanta-requirements branch from 4170a0a to 4d9082f Compare April 11, 2022 10:39

gseidel closed this Apr 12, 2022

gseidel mentioned this pull request Apr 12, 2022

Allow downgrading pagerfanta to version 2.* #420

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix pagerfanta requirements #415

Fix pagerfanta requirements #415

gseidel commented Apr 8, 2022

mbabker commented Apr 8, 2022

gseidel commented Apr 8, 2022

gseidel commented Apr 8, 2022 •

edited

Loading

loic425 commented Apr 8, 2022 •

edited

Loading

mbabker commented Apr 8, 2022

mbabker commented Apr 8, 2022 •

edited

Loading

mbabker Apr 8, 2022

lchrusciel commented Apr 11, 2022

gseidel commented Apr 11, 2022

lchrusciel commented Apr 11, 2022

lchrusciel commented Apr 11, 2022

lchrusciel commented Apr 11, 2022

gseidel commented Apr 12, 2022

gseidel commented Apr 12, 2022

Fix pagerfanta requirements #415

Fix pagerfanta requirements #415

Conversation

gseidel commented Apr 8, 2022

mbabker commented Apr 8, 2022

gseidel commented Apr 8, 2022

gseidel commented Apr 8, 2022 • edited Loading

loic425 commented Apr 8, 2022 • edited Loading

mbabker commented Apr 8, 2022

mbabker commented Apr 8, 2022 • edited Loading

mbabker Apr 8, 2022

Choose a reason for hiding this comment

lchrusciel commented Apr 11, 2022

gseidel commented Apr 11, 2022

lchrusciel commented Apr 11, 2022

lchrusciel commented Apr 11, 2022

lchrusciel commented Apr 11, 2022

gseidel commented Apr 12, 2022

gseidel commented Apr 12, 2022

gseidel commented Apr 8, 2022 •

edited

Loading

loic425 commented Apr 8, 2022 •

edited

Loading

mbabker commented Apr 8, 2022 •

edited

Loading