Add changes to the release process #19
Conversation
|
The RFC for this is: https://wiki.php.net/rfc/policy-release-process-update |
bukka
force-pushed
the
release-process-update
branch
from
71e342f to
79facd3
Compare
This is to mostly match the current process and set some rules for it
bukka
force-pushed
the
release-process-update
branch
from
79facd3 to
5e4a08c
Compare
release-process.rst
Outdated
|
|
||
| - Backwards compatibility breaks in minor versions MUST NOT result in silent | ||
| behavioral differences. Instead any breaking change MUST be "obvious" when | ||
| executing the program. |
There was a problem hiding this comment.
Can you clarify what this looks like in practice? This makes sense when introducing new error cases, i.e. things that now throw an exception. But it doesn't make much sense to change the behavior of non-error cases while then emitting some notice or warning, if the code isn't otherwise considered problematic. If it is problematic, behavior should not change and a deprecation should be emitted instead, so it can later be removed.
There was a problem hiding this comment.
I think the point here is that it should never change for non error cases. For such cases, the deprecation would be used which is not considered as a BC break.
There was a problem hiding this comment.
I noted the error bit there. Let me know if it's ok. Maybe it should be SHOULD?
There was a problem hiding this comment.
@iluuu1994 Given I was the one who suggested this, to give an example: https://wiki.php.net/rfc/static_variable_inheritance would have been an illegal BC break with this policy.
There was a problem hiding this comment.
I changed to SHOULD as it reflects more what we do. It's more a recommendation in that sense.
|
@iluuu1994 @TimWolla @jorgsowa @derickr I went through it again today and added notes about SAPI support. Otherwise it looks ready to me unless you have some further points? |
There was a problem hiding this comment.
Sorry, dropped the ball on the mailing list, but unfortunately no one else replied there either. I'm mostly okay with the contents, but feel that the “what are acceptable BC breaks” section could still get some love. Perhaps we should discuss this in foundation Slack to get at least some additional opinions?
release-process.rst
Outdated
| - Syntax backward compatibility SHOULD be preserved - every PHP program that | ||
| compiles SHOULD continue to compile. | ||
|
|
||
| It is critical to understand the consequences of breaking BC, APIs or ABIs (only | ||
| internals related). It should not be done for the sake of doing it. RFCs | ||
| explaining the reasoning behind a breakage and the consequences along with test | ||
| cases and patch(es) should help. | ||
| - Backward compatibility breaks in minor versions MUST NOT result in silent | ||
| behavioral differences. Instead any breaking change MUST be "obvious" when | ||
| executing the program. It means it SHOULD either throw exception or | ||
| trigger error. | ||
|
|
||
| See the following links for explanation about API and ABI: | ||
| - Userland API backward compatibility breaks SHOULD be limited to | ||
| extensions, or the API of functions within an extension. |
There was a problem hiding this comment.
I'd like to see something about “BC breaks must be simple to work-around in a cross-version way”, e.g. what I mentioned in https://externals.io/message/127328#127335.
There was a problem hiding this comment.
Sorry, I must've missed the notification about this. Will try to take another look tomorrow.
|
Just for the record, I slightly updated the rules for type of fixes in security branches to allow exceptionally merge fixes for some crashes. Let me know if anything about it does not make sense. Specifically it's all in this section: https://github.com/php/policies/blob/97833c8a412fc91b9c4aaa900b57139f6450c2fa/release-process.rst#bug-fix-and-security-releases |
release-process.rst
Outdated
| A backward compatibility (BC) break is defined as any change that prevents | ||
| existing, valid, userland code from continuing to behave as it did in a previous | ||
| version within the same major release. |
There was a problem hiding this comment.
So breaking userland in major versions is not considered a BC break?
I do feel the guidelines in "On Breaking Compatibility" would also apply there?
There was a problem hiding this comment.
Good point! It doesn't make much sense to mention version here because that's specified elsewhere. I changed it.
release-process.rst
Outdated
| A backward compatibility (BC) break is defined as any change that prevents | ||
| existing, valid, userland code from continuing to behave as it did in a previous | ||
| version within the same major release. |
There was a problem hiding this comment.
What does valid mean in this case and in the rest of the document?
Compiling and executing without (fatal) errors?
There was a problem hiding this comment.
I added valid definition below.
release-process.rst
Outdated
| In this context, valid userland code refers to code that compiled and executed | ||
| in a previous version without fatal errors. It may or may not have relied on | ||
| buggy or unintended behavior. The key consideration is whether the change alters | ||
| the behavior in a way that existing code might have come to depend on. |
There was a problem hiding this comment.
As discussed in php/php-src#18962, big 👎 for this definition of BC from my side. This disqualifies the majority of bug fixes from being fixes in patches, and potentially forces deprecations and RFCs for things that could previously just be fixed. This helps no one, quite the opposite.
The key consideration is whether the change alters the behavior in a way that existing code might have come to depend on.
I realize you added this to allow some code fixes even if it changes behavior. But how would you know no code possibly relies on buggy behavior? There's no way to know. This makes this whole clause meaningless.
I would ask what issue we're solving here. Has there been a surge of complaints from bug fixes? I know there have been complaints about the increasing amounts of intentional BC breaks from new major versions. But that's not what we're talking about here.
I will vote against this RFC with this clause.
There was a problem hiding this comment.
But those sort of BC breaks are allowed in minor version. There are just SHOULD for some silent behaviour. Would you want to allow such things in patch releases?
I'm happy to modify it if you have some suggestion. This really tries to specify the current process. If you have feeling, it's different I will be happy to chang it.
There was a problem hiding this comment.
btw the last change actually relaxed things that were already approved by you so I'm a bit confused why it's suddenly big -1...?
There was a problem hiding this comment.
and btw. if you vote against it, it means that you are for disallowing any BC breaks in minor versions as well. That's what the current policy specifies. This PR significantly relaxing this rule and actually allows all those sort of BC breaks happen in the minor version. You should really re-read this before doing such comments.
There was a problem hiding this comment.
One thing that I should actually change is the need for RFC for any BC break. That should be a bit more relaxed so we don't need RFC for each such bug fix change.
There was a problem hiding this comment.
Oh actually there is no such requirement because RFC requirement for BC breaking changes is only specified for features (it's in Feature selection and development) so no need for RFC for bug fixes.
There was a problem hiding this comment.
Ok, just pushed updated version:
In this context, valid userland code refers to code that compiled and executed
without fatal errors in a previous version. Changes that alter behavior,
including bug fixes, MAY still be considered backward compatibility breaks if it
is reasonable to assume that existing code could have come to depend on the
previous behavior. Whether such a change constitutes a significant BC break is a
matter of interpretation and should take into account factors such as
documentation, the expected behavior, the severity of the original issue, and
the likelihood of real-world usage relying on the old behavior.
That should make it pretty generic and just adds MAY for bug fixes... Is that ok?
There was a problem hiding this comment.
Just to clarify why it's all here. I was initiallay asked to define Backward compatibility so that section was added. Then I was asked to define valid here: #19 (comment) . So I added this valid definition... :)
There was a problem hiding this comment.
I just rewrote it and defined a clear exception for bugs. This section was kept but points to the exception part to make clear that not all things are considered as BC under that definition. Please check it out and let me know if it's ok.
There was a problem hiding this comment.
Thank you for the changes. I will re-read the whole RFC to refresh my memory. Today's a holiday in my country, so I might not get around to it until tomorrow.
|
I did hopefully the last update into this to better clarify backward compatibility section and added also note about the current rules and some recommendation for major version bump. |
| - It SHOULD include bug fixes and security patches | ||
| - New features MUST NOT be added. | ||
| - Extensions support MUST NOT be removed (like move them to PECL) | ||
| - Backward compatibility MUST be kept (internals and userland) |
There was a problem hiding this comment.
This wording is contradictory with allowing semantic bug fixes
| - Syntax backward compatibility SHOULD be preserved - every PHP program that | ||
| compiles SHOULD continue to compile. | ||
|
|
||
| - Backward compatibility breaks in minor versions SHOULD NOT result in |
There was a problem hiding this comment.
This wording is also contradictory with allowing semantic bug fixes
There was a problem hiding this comment.
This depends on whether semantic bug fixes are considered BC breaks. The text now says:
The following are not considered BC breaks:
- Fixing clearly incorrect or unintended behavior, even if it changes the output or side effects of a function, is not automatically considered a BC break.
So, that should be ok. I think it's worth specifying that such bug fixes may be delayed to the next minor version if the possible impact is not considered negligible (those would go to patches), but small. How small exactly may be hard to define.
| - ABI backward compatibility MAY be broken. | ||
|
|
||
| - Syntax backward compatibility SHOULD be preserved - every PHP program that | ||
| compiles SHOULD continue to compile. |
There was a problem hiding this comment.
Does this include keywords? These are added in minor versions quite frequently. I guess I missed this on my last read-through.
| - Syntax backward compatibility SHOULD be preserved - every PHP program that | ||
| compiles SHOULD continue to compile. | ||
|
|
||
| - Backward compatibility breaks in minor versions SHOULD NOT result in |
There was a problem hiding this comment.
This depends on whether semantic bug fixes are considered BC breaks. The text now says:
The following are not considered BC breaks:
- Fixing clearly incorrect or unintended behavior, even if it changes the output or side effects of a function, is not automatically considered a BC break.
So, that should be ok. I think it's worth specifying that such bug fixes may be delayed to the next minor version if the possible impact is not considered negligible (those would go to patches), but small. How small exactly may be hard to define.
| - Backward compatibility breaks in minor versions SHOULD NOT result in | ||
| silent behavioral differences. Instead any breaking change SHOULD be | ||
| "obvious" when executing the program. It means it SHOULD either throw | ||
| exception or trigger error. |
There was a problem hiding this comment.
Is there some room for small BC breaks in minor versions? One example I can think of:
https://externals.io/message/117466
This added proper support for enums to var_export(). The risk of breakage was very small, but it was a user facing change.
Furthermore, some features are difficult to introduce via migration path. In this case, we would have needed some opt-in flag, be that an argument, ini setting or similar. Often, these can make the API more convoluted and worse in the long run.
IMO, small BC incompatibilities should be ok if accepted through a community vote. The current draft technically already allows for this through the looser "SHOULD" requirement. But might still make sense to write it down.
This may have been a compromise for php/php-src#18962, deciding on whether this theoretical BC break is worth the risk, fixing it earlier and avoiding users having to change their code (assuming it was correct, but misbehaved due to the bug).
| - Security fix and regression releases SHOULD occur on the same date as | ||
| bug fix releases for the other branches. Exceptions can be made for | ||
| high risk security issues or high profile regressions. | ||
| - Security fix, compatibility build fix, and regression fix releases |
There was a problem hiding this comment.
Nit: The wording above is "compatibility fixes", use the same for both?
This is to mostly match the current process and set some rules for it. The will be accompianed by the RFC.