Allow >= 1.1.0 & 2.x for Pagerfanta #194
Conversation
|
Some tests here are failing because of translation string changes in Pagerfanta. They'll be fixed by getting #189 complete and merged (assuming we then rebase this branch off that). The 404 error in tests is probably something else, but I'll look into that once the translation ones are fixed and it's clearer where we're at! |
|
2.x should be allowed too :) |
|
Thanks for the input, @emodric. Since Pagerfanta is following Semantic Versioning, 2.x could include backwards-incompatible changes, whereas it's guaranteed that 1.x won't. Therefore, it makes sense to leave this as it is - if a 2.x is released, we'd then need to ensure that this bundle worked correctly with it before updating the version constraint. |
|
@sampart As far I can see, there's no breaking changes in Pagerfanta 2.0 compared to 1.1, so allowing it should not break anything. |
|
Sorry, I forgot I'd already done a 2.0.0 release of Pagerfanta! Whoops. However, since that bumps Pagerfanta to be PHP 7, I wouldn't want to make this bundle use that until we also moved the bundle to PHP 7 (and bumped its major version too). We have issue #193 for that. |
|
Well, you don't have to bump the bundle to PHP 7 only. It can remain on PHP 5 & 7, and allow Pagerfanta 1.x and 2.x. My point is, to allow Pagerfanta 2, this bundle doesn't need a new major version. |
|
This is a tricky one to call. However, Pagerfanta 2.x is effectively stating that whilst it may work with PHP < 7 (the code hasn't changed), it's no longer officially supported there - it's not tested against PHP < 7, and Given that, I wouldn't want this library to claim support for PHP < 7 whilst including in its core functionality a library which doesn't support that. Hence the need for a version number bump. |
|
I see your point, yes, but I'm not sure I agree. If this bundle is compatible with Pagerfanta 2.x, and it is, it should allow installing it. People using PHP 5, will not be able to install Pagerfanta 2.x anyhow and they will fallback automatically to 1.x, while PHP 7 users will get 2.x. I don't see a problem with that. Nice sideeffect too would be easy and automatic upgrade to Pagerfanta 2.x for projects which depend on the bundle and not on the lib. But I will leave the decision to you, you're the boss :) |
That's a really good point. I hadn't thought about the automatic fallback. I think you're probably right that we don't need a major version jump, then. Thanks for taking the time to work this one through with me. I'll update the PR. |
The latest 1.x release is https://github.com/whiteoctober/Pagerfanta/releases/tag/v1.1.0, which the previous constraint wouldn't have allowed.
See discussion at #194
|
Thanks for listening! :) |
|
I've resolved the merge conflicts in the initial commit, and added an "allow 2.x" commit too now. The branch name is now out-of-date therefore, but I can live with that! 😉 Just waiting on #189 now. I'll ping them for an update in a week or two. |
These changes were originally in #189, but I've brought them across into this branch as they require Pagerfanta >= 1.1.0
|
I was slightly mistaken - to bring this bundle and Pagerfanta into line on using Given that, I've brought the changes from #189 across into this PR and closed #189. Once @olorton has re-reviewed this PR, I'll merge it. |
|
The failing tests here are (now) unrelated to this work - they're because of known issue #182 with PHP 5.3(!) |
The reason for this is that older versions of Pagerfanta still (incorrectly) use the option previous_message rather than prev_message in the Default template (whiteoctober/Pagerfanta#230), but this branch now uses prev_message everywhere. In order to keep the two in-step, we need to ensure that this code is running against Pagerfanta >= 1.1.0
sampart
changed the title
|
I've created release https://github.com/whiteoctober/WhiteOctoberPagerfantaBundle/releases/tag/v1.2.1 for this. |
|
@sampart What happened to 1.2.0 ? :) |
|
@emodric, whoops - that was a Monday morning kind of mistake on my part! I've updated that release to have the right name and tag name: https://github.com/whiteoctober/WhiteOctoberPagerfantaBundle/releases/tag/v1.2.0 |
|
Thank you so much for pointing out my mistake so quickly! |
|
You're welcome! Thank you for allowing Pagerfanta 2.x :) |
|
Also, you shoud probably delete 1.2.1 from packagist.org too: https://packagist.org/packages/white-october/pagerfanta-bundle#v1.2.1 |
|
Good point, thanks. Done now. |
The latest 1.x release is https://github.com/whiteoctober/Pagerfanta/releases/tag/v1.1.0, which the previous constraint wouldn't have allowed.