search

collective / dexterity.membrane

enables dexterity content items to be used as users and groups in Plone sites
3 stars 14 forks source link

Can we drop Plone 4.2 support? #21

Closed petri closed 8 years ago

petri commented 8 years ago

If so, is it sufficient to deactivate Travis run for Plone 4.2 and add a note in README?

agitator commented 8 years ago

+1

would that be a 2.0.x step?

hvelarde commented 8 years ago

dropping support for a version should be done only when really needed; please investigate further as tests are passing here: https://travis-ci.org/collective/dexterity.membrane/builds/126354703

mauritsvanrees commented 8 years ago

I don't mind dropping support for 4.2. But indeed: when it still works, and does not cause too much hassle, we should keep it. Note: I am not using this package at the moment.

gforcada commented 8 years ago

Well, the question is: are you planning to add a new feature that would mean that keeping backwards compatibility would be too much work? Then yes, drop 4.2 and make a new major release after that work is merged!

hvelarde commented 8 years ago

guys, this issue seems simple to fix, so please don't enforce Somebody Else's Problem behavior (breaking compatibility) as an easy (and false) solution.

gforcada commented 8 years ago

@hvelarde sorry but @petri just asked how to drop support, not why...

hvelarde commented 8 years ago

@gforcada no, he did ask if we can drop support, I say no, unless you have a good reason for it.

petri commented 8 years ago

I guess I did ask both up there. I've got enhancements coming up (first PR #20) and while the PR branch originally passed Travis runs, the PR itself failed the 4.2 run (with identical code) because of conflicting requirements for plone.app.locales:

I don't know what would be the right way to fix the Travis run for 4.2 or if there even can be a good solution with reasonable effort.

Given that, and that anyone on 4.2 probably should have upgraded to 4.3 long ago, I figured the best way forward would just drop the 4.2 Travis runs. Which would not mean breaking compatibility (sorry for the perhaps misleading issue title there).

But I'd gladly be proven wrong, if someone can come up with a fix to the buildout requirement conflict that'd work for sure? (without the p.a.l conflict the buildout runs ok)

petri commented 8 years ago

BTW, this issue is not just about the PR. Travis Plone 4.2 run fails on master as well.

@mauritsvanrees, given you originally merged dexterity.membrane #15 and recently authored the plone.app.locales requirement in plone.app.referenceablebehavior, what would you recommend?

OTOH, if plone.app.referenceablebehavior is used to just enable references from Archetypes content, could we stop forcing it on all dexterity.membrane users?

mauritsvanrees commented 8 years ago

You can pin plone.app.locales to 4.3.10. Preferably only when using Plone 4.2, so that other Plone versions can use their official version.

plone.app.contenttypes has plone.app.referenceablebehavior in a test and atrefs extra in setup.py. That might have been a good approach too. I guess that would be fine, though it would require testing to see if anything breaks for existing content when removing the dependency.

petri commented 8 years ago

Why pin to 4.3.10? @mauritsvanrees don't you mean pinning plone.app.locales to 4.2.5 when using Plone 4.2?

mauritsvanrees commented 8 years ago

No, because that is what is currently done by Plone itself, and latest plone.app.referenceablebehavior requires a newer plone.app.locales version, which is what causes the problem.

Actually, only since p.a.ref 0.7.4 does it require a minimum p.a.locales version, so we could pin it to 0.7.3. Simplest would be to do that for all versions in travis.cfg.

petri commented 8 years ago

Thanks Maurits. I will commit that fix to make Travis happy.