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

Upgrade PagerfantaBundle to new version with B/C layer #175

Merged
merged 1 commit into from
Aug 31, 2020
Merged

Upgrade PagerfantaBundle to new version with B/C layer #175

merged 1 commit into from
Aug 31, 2020

Conversation

mbabker
Copy link
Contributor

@mbabker mbabker commented Jul 10, 2020

Q A
Bug fix? no
New feature? no
BC breaks? yes
Deprecations? yes
Related tickets Sylius/Sylius#10928
License MIT

This is an alternative version of #169 with a B/C layer to try and map the bundle configuration from WhiteOctoberPagerfantaBundle to BabDevPagerfantaBundle.

The B/C layer involves interfacing directly with the container in a few ways:

  • Using a custom container extension that can process the configuration under white_october_pagerfanta and map it to the babdev_pagerfanta configuration (since none of the child nodes were renamed, this part is pretty simple) and re-create the renamed white_october_pagerfanta.default_view parameter
  • Using a compiler pass to handle the white_october_pagerfanta.view_factory.class parameter that I fully dropped to change the Pagerfanta\View\ViewFactory class to something else if so desired and to alias the renamed twig.extension.pagerfanta and white_october_pagerfanta.view_factory service IDs (the aliases are deprecated in favor of the new bundle's service IDs)

The one part of #169 (comment) not handled is a class mapping layer for the namespace change. It'd be easy to throw together a file with a bunch of class_alias() calls, but as also noted in the comment, a bunch of classes were made final so if someone were extending them then even the aliasing wouldn't save them from that API break.

In addition to just upgrading to a supported bundle version that also removes one roadblock to full Symfony 5 support, there are a number of new features in the Pagerfanta library and/or PagerfantaBundle that could be interesting to Sylius users, including:

  • Customizable route generation logic (no longer hardcoded into the Twig extension)
  • Twig template support alongside the pre-existing PHP templates
  • Tailwind CSS support (I've shipped the Twig template for it, need to add the PHP version still)
  • At some point serializer support will land (at work we're manually building a lot of responses from our API with data out of the Pagerfanta\Pagerfanta class, at some point it's going to make sense to move that logic into the serializers (both Symfony's and JMS') and improve the pager's use in an API context)

@mbabker mbabker requested a review from a team as a code owner July 10, 2020 01:58
@loic425 loic425 mentioned this pull request Jul 10, 2020
Copy link
Member

@lchrusciel lchrusciel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build is green, tests are added and it seems to work 🎉 thanks a lot for your work, Michael!

I would like, however, someone else to take another look into this (or at least I need to check it out again next week)

@stloyd
Copy link
Contributor

stloyd commented Aug 18, 2020

@lchrusciel @pamil Is there anything blocking this? We would love to use Sylius + Symfony 5 but we're still locked due too few things :(

@lchrusciel
Copy link
Member

Hey Joseph, we are blocked mostly by our current commitments. We will enter a feature freeze next week, so probably we will put more effort into Symfony5 compatibility since then.

@lchrusciel lchrusciel merged commit 52df400 into Sylius:master Aug 31, 2020
@lchrusciel
Copy link
Member

Thank you, Michael! 🎉

@mbabker mbabker deleted the feature/pagerfanta-upgrade branch August 31, 2020 13:51
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.

3 participants