Mark breaking change more clearly

[mjomble]

This is:

• a documentation change
• a bugfix
• a new feature
• refactoring
• additional unit tests

Checklist:

• Changes are covered by unit tests
  • Changes are covered by existing unit tests
  • New unit tests have been added
• Code style is respected
• Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
• CHANGELOG.md contains a short summary of the change and a link to the pull request if applicable
• Documentation is updated as necessary

Why this change is needed?

This change resulted in production code breaking in one of my projects. Marking it more clearly might help others avoid this problem.

[oleibman]

Thank you for the suggestion. I actually just merged something equivalent with PR #3787, so this is no longer needed. Closing.

[mjomble]

This PR is actually based on the result of merging the other PR.
#3787 mentioned the issue under "Changes", where it's not clear that it's a breaking change.
This PR moves it from "Changes" to "BREAKING CHANGE".

[oleibman]

Everything listed under "Changed" is a breaking change. We don't usually have a "Breaking Change" section. The pending release has one because the change to leverage native typing after eliminating Php7.4 was extremely broad (most breaking changes are narrow in scope). Before that, there has been no Breaking Change section since the very first release of PhpSpreadsheet in 2017; we have consistently used Changed ever since.

[mjomble]

Everything listed under "Changed" is a breaking change.

Ah, that is very surprising, given the top of the changelog states that "this project adheres to Semantic Versioning"

According to the Semantic Versioning conventions, any breaking change requires incrementing the major version, while minor version changes must be backwards compatible.

Would it be fair to say that this project doesn't actually adhere to Semantic Versioning?

[mjomble]

Or to add a clarification like "This project mostly adheres to Semantic Versioning, although we don't increment the major version for some breaking changes that are narrow in scope"?

[oleibman]

Interesting observation. Not my area of expertise. I want to defer to my colleagues, but I wouldn't mind continuing the discussion with you for a bit. For the change in question, it was not even reported as a problem for some time. It was a mistake to not document the change properly at the time (and, arguably, to change the constant in the first place), but, having made the mistake, I don't know what else we can do other than document it properly for both the release in which it happened and the pending release. We can't, or at least won't, retroactively make it a major release.

So, let's look at some other things in the Changed section.

• Php 7.4 could be used with release 1.29; it won't be usable with 1.30. For people still using 7.4, this will be a break. Even if the user manually overrides the Php release requirement, failure will follow due to inclusion of syntax not allowed under 7.4, including typing in the APIs (so this would be involve "incompatible API changes"). Does Semantic Versioning require a new release for this?
• RLM and NumberFormattingCurrency. The end user will eventually see changed behavior if we do nothing due to changes outside our control. Does providing a predictable path forward for our users require a new release?
• HTML Writer chart sizes. I could have documented it under Fixed rather than Changed. But, for most users, it doesn't need fixing - the code in question is unchanged since the beginning of time, and this is probably the first report we've had of this problem. Users will nevertheless see a difference in the chart size, so I think it's important to document it differently than, say, HTML omitting some charts, which originates with the same issue, and which is documented under Fixed, because, even though the behavior is changed, it was clearly wrong in the first place. So does the chart sizes change require a new release?

[mjomble]

It was a mistake to not document the change properly at the time (and, arguably, to change the constant in the first place), but, having made the mistake, I don't know what else we can do other than document it properly for both the release in which it happened and the pending release. We can't, or at least won't, retroactively make it a major release.

I agree - what's been released has been released.
And if your "Changes" sections have always included changes that may be breaking for some users, there's no need to move this change.

It is also true that the difference between a breaking change and a non-breaking one can be vague in many cases.
In my experience, most projects tend to err on the side of caution and bump the major version just in case, even if the number of affected users is expected to be fairly low. But bumping it too often can also have its downsides.
Some projects prefer to postpone breaking changes for a while and then release several of them in a single major version bump.

• Php 7.4 could be used with release 1.29; it won't be usable with 1.30. For people still using 7.4, this will be a break. Even if the user manually overrides the Php release requirement, failure will follow due to inclusion of syntax not allowed under 7.4, including typing in the APIs (so this would be involve "incompatible API changes"). Does Semantic Versioning require a new release for this?

In my opinion, this definitely calls for a new major version.

In the "Node.js universe" that I have more experience with, dropping support for older Node.js versions is almost always considered a breaking change that requires a major version increase. Even if the Node.js version in question is very old and hasn't been in active support for years.

• RLM and NumberFormattingCurrency. The end user will eventually see changed behavior if we do nothing due to changes outside our control. Does providing a predictable path forward for our users require a new release?

I'm not familiar enough with this change, but in general, my gut feeling says that:

• If behavior changes when a user switches their PHP version, but stays on the same version of PhpSpreadsheet, then it's not a breaking change in PhpSpreadsheet.
  • One should always test the entire system thoroughly if upgrading PHP.
• If behavior changes when a user stays on the same PHP version, but switches their PhpSpreadhseet version, then it is a breaking change in PhpSpreadsheet (though possibly one with narrow impact).
  • One of the goals of semver is that you should only have to test thoroughly when upgrading to a newer major version. While minor/patch version changes should be safe and only fix bugs and add features.

• HTML Writer chart sizes. I could have documented it under Fixed rather than Changed. But, for most users, it doesn't need fixing - the code in question is unchanged since the beginning of time, and this is probably the first report we've had of this problem. Users will nevertheless see a difference in the chart size, so I think it's important to document it differently than, say, HTML omitting some charts, which originates with the same issue, and which is documented under Fixed, because, even though the behavior is changed, it was clearly wrong in the first place. So does the chart sizes change require a new release?

This seems like a grey area. I can imagine it might mess up someone's layout, but it doesn't sound "catastrophic enough" for a major release 😄

The number format difference, however, changed our generated spreadsheets from displaying more accurate values to less accurate values (e.g. rounding € 5.32 to € 5), which caused quite a bit of confusion and problems for accountants using the system. If it had been known before the release, I would have considered it a breaking change.
At this point, we can't do much about this issue, but we might be able to clarify this project's interpretation of breaking changes at the top of the changelog document.

[oleibman]

I don't know how to embed new comments in a conversation like you did with your last reply. Please enlighten me.

That being so, I have only one additional comment for now, expanding on the RLM change. The behavior change would have come not with any release of PhpSpreadsheet, nor even with a specific release of Php. It was entirely dependent on the release of ICU which was incorporated in the Php release. So it could be triggered with a new point release of Php, or even running in a different environment with the same point release. I noticed it because I was getting different results between Windows and Linux.

[oleibman]

@MarkBaker @PowerKiKi I've taken this discussion about as far as I can, which is, hopefully, far enough to get you involved. Should we be changing how we number our releases? Should we instead offer some sort of clarification about "breaking changes" at the top of the change log?

[mjomble]

I don't know how to embed new comments in a conversation like you did with your last reply. Please enlighten me.

Here is more info on formatting GitHub comments. The quote button is also on the comment editor toolbar, after Bold and Italic.

That being so, I have only one additional comment for now, expanding on the RLM change. The behavior change would have come not with any release of PhpSpreadsheet, nor even with a specific release of Php. It was entirely dependent on the release of ICU which was incorporated in the Php release. So it could be triggered with a new point release of Php, or even running in a different environment with the same point release. I noticed it because I was getting different results between Windows and Linux.

I see. Even though the change is an improvement in most cases, I can imagine scenarios where someone has been using PhpSpreadsheet in an environment where the ICU doesn't change and then a minor version upgrade of PhpSpreadsheet breaks their application (by using a different number format than before, I assume).

So I guess a good baseline for deciding is imagining the worst-ish case scenario that a change might unexpectedly have for someone and figure if you'd want to protect them against that through a major version bump.

Of course, the worst case scenario might involve a person unaware of the semver convention who blindly updates to a new major version without testing, but we have to draw the line somewhere 😄

[PowerKiKi]

I'll have to side with @mjomble in general. We should definitely release major version when something breaks. This will be the case for the next version because we introduce typing that will break inheriting classes. In the future we should also be more careful as to not release a breaking change too often or in patch releases.

And Changed in the CHANGELOG never meant "breaking change" for me, on the contrary it's a very minor change with minimal impact, eg formating code, refactor tests, etc.

However, in PHP ecosystem, dropping the support for a PHP version is absolutely not a breaking change. Composer strongly enforce this constraint. If you use composer correctly and you have an old PHP, you just won't get the latest version of the library. But dropping a PHP version will never requires you to update your code, so it's not a breaking change.