BC breaks in minor releases

[1ed]

If "This project adheres to Semantic Versioning." as it's said in the changelog, then it should not break BC in minor releases.

[p365labs]

Hi @1ed can u explain better what do u mean and of what bc in minor are you talking about ? Tnx a lot!

[p365labs]

not sure but I think we can close this.

[1ed]

From the changelog:

5.3.0

Backward Compatibility Breaks

Removed Query\NumericRange, use Query\Range instead #1334

...

5.1.0

Backward Compatibility Breaks

\Elastica\Script\AbstractScript added the script language as constructor argument and sub-classes must implement getScriptTypeArray
...
3.2.0

Backward Compatibility Breaks

Method \Elastica\ResultSet::create and property \Elastica\ResultSet::$class were removed. To change the ResultSet class, implement your own ResultSet Builder. #1065
Properties on \Elastica\ResultSet _totalHits, _maxScore, _took and _timedOut that were originally set on object construction are now accessed by the getters on the ResultSet. #1065
...

If this project adheres to Semantic Versioning these BC breaks should only happen in major releases. If not, then the users of this library are prevented to use permissive version constraints and are in danger of version lock (or at least it is very hard to support multiple versions).

[ruflin]

If we do BC breaks in Minors, there is normally a good reason for it. For example for #1334 this was deprecated since 0.90 in elasticsearch and removed in 5.0. As Elastica states it's only compatible with 5.0 it was more an oversight to have the function still in. So this only broke code if someone used Elastica with an unsupported version of Elasticsearch as with 5.x this would not work from the Elasticsearch side.

If I remember the BC break in 5.1.0 correctly it was needed to support some new features and only applied to users which used the AbstractScript type and extended it. As we tie Elastic release to the elasticsearch release, this was a trade off to way 12 month for the 6.0 release or do a small BC break. Did we break semantic versioning with this? Probably yes but I think here the benefits outweighted the potential downsides.

[merk]

But why are you tying Elastica releases to ES versions? Theres no need for it? You'd simply have to publish your supported ES version range in README.md and when you decide to drop support for a specific ES version you can just bump a major.

You may have discussed this at some point recently and sorry if its already been debated to death :)

[p365labs]

In my opinion I prefer having Elastica tyed to Elasticsearch, as Elasticsearch-php client do, this way users should have less problem in terms of understanding compatibility. The only thing I would consider would be to apply hotfixes, security patches, etc, to old releases.

[1ed]

I think, that is not a choice. There are rules, if we do not respect the rules we should not advertise to follow them, I think. I know it's easier on the library maintainer side, but it's a PITA on the consumer side. We can choose to not follow SemVer, but please do not advertise to adhere it if do not.

[ruflin]

@merk For Elastica tying the ES version. I went forth and back on this. Initially Elastica was tied to it, then I decoupled it with 3.0 but then brought it in line again for 5.0. Having it in sync with the ES version makes explaining which versions are compatible, tested against etc. much simpler and I'm trying to keep the project as lightweight as possible. Having it in sync made my life easier :-)

@1ed I'm happy to change the line on top of Changelog to "tries to adhere" to Semver.

So far I'm not aware that there are users which got affected by our breaking changes in minor. As this is popping up now I assume @1ed you were affected by one of these?

[greg0ire]

this way users should have less problem in terms of understanding compatibility

This sounds wrong, and will be more and more wrong over time, as more and more libs use semver (it should go from 99.9% to 99.99% very quickly IMO). I think the best way to solve this issue would be to ask user to first install elasticsearch/elasticsearch and carefully read their compatibility matrix, and then ask them to let Composer pick the right elastica version by just typing composer require ruflin/elastica. I think users are far more disgruntled by BC-breaks than by what not pinning might imply. cc @curry684

[ruflin]

I think there are 3 discussions here:

1. Should Elastica be tied to the Elasticsearch version and the elasticsearch-php client
2. If Elastica is tied to ES / ES-php, which versions is it tied to
3. Should Elastica fully confirm to Semver.

For 1. the answer for me is yes, at least for the major versions. For minor / bugfixes happy to have a deeper discussion on what the implications are.

For 3. as stated before, I'm ok breaking semver in some cases if really needed. TBH as we follow Elasticsearch major versions, Elastica has become more stable and there are more Elasticsearch major releases now, I think it's becoming more and more unlikely that we actually have to break semver in the future. The comment here (#1432 (comment)) from @curry684 is interesting related to this. If I understand this correctly, removing features is ok if the method stays around but potentially throws an exception? For me so far I would have treated that as a breaking change.

[greg0ire]

IMO, if you have a series of libs A → B → C, and C is a bindings lib for an external tool "D" that follows semver, only C should be affected and synced to D, but A and B should just leverage Composer to do version constraints resolving. Users of A and B should just have to read one compatibility matrix, in C's README, and that's all. If we are in version 6, and 7 comes out, and there is no BC-break in it's API, then C version 7 should come out too, but B should only have a new minor version because of the new methods it exposes. People still using D v6 will be able to benefit from bugfixes after that version is out. People upgrading D from 6 to 7 will know that they need not change any piece of code / read B's changelog if the new version is minor.

So to sum up:

• issuing a new major when there should not be one is bad because it creates fragmentation in your user base, because people don't notice they need to bump the major number or fear to do it "right now"
• not issuing a new major when there is a BC-break is very, very bad
• syncing is needed when you have an external tool that composer doesn't know about, but here, syncinc is already there in elasticsearch/elasticsearch. That's bad but necessary. Don't use it in your lib, because that's bad and unnecessary.

UPDATE: changed from numbered items to bullet points to avoid confusion

[curry684]

From my perspective and experience:

1. Yes, keeping majors together is good practice and clear to the end users. Also, as Semver implies upstream should release a new major when it breaks BC this is also the time when you can break BC here.
2. 1 on 1 for the majors. Minor and patch are irrelevant from Semver perspective.
3. Yes, but pragmatically. Given the case we discussed, if you forgot to remove a method that is no longer supported by upstream it was crashing anyway. Giving it a clear exception instead is not breaking BC, and therefore acceptable even in patch releases. It's a bugfix.

There is a misconception that Semver's implied BC promise would automatically imply it's forbidden to change any kind of historic behavior. It's only meant to imply my application will not break when using composer update. It's fine to change the way it was already broken, or to fix a bug (although if it was usable before it's good practice to save that at least for a minor, or perhaps even a major).

[greg0ire]

One point that I forgot to mention: not syncing will mean that you might end up with one version of this library being compatible with several versions of ES. This is desirable because it makes migrations easy: people can choose to upgrade ES and elasticsearch/elasticsearch, and be confident that their application will not break, because they do not use elasticsearch/elasticsearch directly in their app (they do not reference that namespace anywhere in it). Later on, they can upgrade ruflin/elastica, because they want to use features that are specific to the latest version of ES, that weren't exposed before. Ideally, your users should only have to upgrade one thing at a time, not both their infrastructure and their code.

[ruflin]

There is definitively the chicken / egg problem about which you should update first, server or client. Unfortunately the past has shown each major had breaking changes which had do be fixed in Elastica to make it working. The nice part of Elastica that it has a proxy for most things in Elasticsearch is also the biggest problem, as every small breaking change in ES is a breaking change in Elastica. The elasticsearch-php client only provides the raw functionality and lets it up to the user to create for example all the queries. Like this the change on the user side is breaking but not on the library side which is nice, but the users will have to figure out himself that something broke in his query. So my past experience is that there are potentially more major Elastica releases then major elasticsearch releases. @greg0ire Still, it would be great if we could have this with a reasonable effort.

The reason I state very early in the discussion that I'm willing to break semver sometimes as I think the decisions we make must be on the one hand best for the user but also for the library and it's dev to make sure it stays maintainable long term. @curry684 I really appreciate your approach here and your suggestion on how we should have solved it better.

My suggestion to move forward on this that in case we have a future PR which would break BC without being a major again we come together and find a solution for this specific case.

[curry684]

Cheers, thank you for the good and productive discussion!

[ruflin]

@curry684 @greg0ire @1ed Looking at #1422 I was wondering what your take on this one is? It removes some code which was already deprecated in 5.x but we forgot to remove it for 6.0. I think most of it will not even work anymore with Elasticsearch 6.0. If we merge this into master does that mean it we could not make any 6.x release with it anymore?

[greg0ire]

If you think no one can use the removed feature and have something that works in 6.x, then there is no BC-break, so merging is fine, you can release in 6.x. But if you think there are some features that could be used and work, then yeah, merging that would mean you would have to release 7.x IMO (if you want to follow semver).

[ruflin]

We have now one of these issue where the question is if it's a break or not. Would be great to get some comments on it: https://github.com/ruflin/Elastica/pull/1506/files#r197741395

[thePanz]

Can we close this issue? @ruflin
I guess this was open for v5.x (now unmaintained)