added a maintenance document #6412

fabpot · 2016-03-30T13:04:39Z

As the Symfony core team grows, more people merge PRs in old branches and I think we need to make it very clear what we can accept in versions under maintenance. This new reference document tries to list all possible cases.

fabpot · 2016-03-30T13:09:42Z

I've probably forgot some things, feel free to list them here. ping @symfony/mergers

javiereguiluz · 2016-03-30T13:16:09Z

What about refactorings which don't provide significant performance improvements? Example: https://github.com/symfony/symfony/pull/18227/files

javiereguiluz · 2016-03-30T13:19:46Z

Another use case: removal of legacy code which was left over by mistake. Example: https://github.com/symfony/symfony/pull/18299/files

javiereguiluz · 2016-03-30T13:23:26Z

Another use case: changes related to design. I'd like to redesign the exception page, the Symfony Requirements check webpage, etc. to match the new design of the toolbar, profiler, welcome page, etc. Would that be allowed?

fabpot · 2016-03-30T13:24:48Z

@javiereguiluz

Refactorings: I would say no
Removal of legacy code: yes, that's a bug
Design fixes, yes, design changes, no

javiereguiluz · 2016-03-30T13:25:21Z

Another one: adding tests for untested features present in old branches.

javiereguiluz · 2016-03-30T13:27:30Z

A question: changing the visibility of properties/methods is considered a new feature? Example: adding a getter for one property: https://github.com/symfony/symfony/pull/18143/files

Tobion · 2016-03-30T13:28:01Z

contributing/code/maintenance.rst

+* **Exception messages**: Exception messages must not be changed as some
+  automated systems might rely on them (even if this is not recommended);
+
+* **Adding new Composer dependencies**;


So the refactoring with polyfill would not be tolerated in maintainance anymore.

Yes, and in fact we introduced most polyfills in 2.8. The ones added in 2.3/2.7 were for security fixes and bug fixes only (which doesn't mean we were right regarding this doc :) )

javiereguiluz · 2016-03-30T13:31:22Z

Another question: where does "Upgrading ICU data" fit? It was merged on 2.3: symfony/symfony#18019

Tobion · 2016-03-30T13:31:42Z

contributing/code/maintenance.rst

+  it otherwise;
+
+* **Support for external platforms**: Adding support for new platforms (like
+  Google App Engine) cannot be done in patch versions;


symfony/symfony#18271 would violate this as well in retrospect.

which is precisely why we create this document

fabpot · 2016-03-30T13:32:51Z

Keep in mind that I'm not documenting the current way as it is inconsistent at times. That's why I want to write some rules for the future (what we've done in the past is irrelevant but useful to draw the line in the sand).

HeahDude · 2016-03-30T13:37:26Z

contributing/code/maintenance.rst

+
+* **Coding standard**: Coding standard fixes are not recommended but can be
+  accepted for consistency with the existing code base, if they are not too
+  invasive, and if merging them on master would lead to complex branch merging.


"would not lead" ?

I think this should also state unused variables: symfony/symfony#18293 which should be applied to the lowest possible maintained branch where applicable

"removing unused variables" is a coding standard fix. I don't think it's needed to state it explicitly

stof · 2016-03-30T13:45:11Z

A question: changing the visibility of properties/methods is considered a new feature? Example: adding a getter for one property

both are new features

stof · 2016-03-30T13:45:43Z

Another one: adding tests for untested features present in old branches.

I would accept this in maintenance branches, as it helps preventing regressions

HeahDude · 2016-03-30T13:50:26Z

Like symfony/symfony#12425 (comment) for example ?

nicolas-grekas · 2016-03-30T13:51:36Z

contributing/code/maintenance.rst

+During the lifetime of a minor version, new releases (patch versions) are
+published on a monthly basis. This document describes the boundaries of
+acceptable changes.
+


I suggest adding a general statement like:
This policy has one main goal: allow Symfony to keep getting better over time while avoiding the risk of breaking an existing and working application.

I don't think it adds much value. It also conveys the idea that as long as risk is low, new features could be added.

xabbuh · 2016-03-30T14:17:54Z

@fabpot We need to add a reference to the new file to /contributing/map.rst.inc too.

andrerom · 2016-03-30T15:56:14Z

contributing/code/maintenance.rst

+* **Translations**: Translation updates and additions are accepted;
+
+* **Version updates for Composer dependencies**: Changing the minimal version
+  of a dependency is possible, bumping to a major one is not;


bumping to a major one is not; => bumping to a major one or increasing php requirements is not; ?

nicolas-grekas · 2016-04-05T15:53:00Z

I propose also something like:

It is also designed to enable a continuous upgrade path that allows one to move forward with newest Symfony versions in the safest way. One should be able to move PHP versions, OS or Symfony versions almost independently. That's the reason why e.g. supporting latest PHP versions or OS features is considered as bug fixes.

alexislefebvre · 2016-04-06T12:13:26Z

contributing/code/maintenance.rst

+published on a monthly basis. This document describes the boundaries of
+acceptable changes.
+
+**Bug fixes** are accepted under the following conditions:


This may be unclear for a newbie. When someone add a PR, what makes it a bug fix? Does Bug fixes have a special meaning in the context of Symfony? Is it mandatory that a bug fix refer to an existing ticket?

@alexislefebvre bug fixes can be merged in patch versions. This means that now 3.0.0 was released, the 3.0 branch can only have bug/security fixes. New features must only be merged in the unstable branch (master/3.1 at the moment).

wouterj · 2016-04-10T09:08:54Z

contributing/code/maintenance.rst

+
+ * The change does not break valid unit tests;
+ * New unit tests cover the bug fix;
+ * The current buggy behavior is not widely used as a "feature".


lists should not be indented in reSt (it causes blockquotes)

wouterj · 2016-04-10T09:13:50Z

What about using a definition list instead of the unordered lists?

**Performance improvements**
    Performance improvements should [...]

fabpot · 2016-04-10T17:23:35Z

@wouterj because this is really a list.

fabpot · 2016-04-10T17:23:48Z

I've taken into account all comments, should be better now.

wouterj · 2016-05-21T08:42:19Z

@symfony/mergers can you please make a decision on this PR? From a doc point of view, it's ready to be merged but I'm not sure if this is already accepted by the core team.

javiereguiluz · 2016-05-21T17:10:57Z

I've re-read it again and I'm 👍

fabpot · 2016-05-23T08:55:47Z

I would merge this as we can always tweak the rules easily if we think they need to be refined based on our experience. Looks good to me as version 1

wouterj · 2016-05-27T08:44:21Z

Alright, let's merge this now then. Thanks for writing and updating this PR Fabien!

This PR was merged into the 2.3 branch. Discussion ---------- added a maintenance document As the Symfony core team grows, more people merge PRs in old branches and I think we need to make it very clear what we can accept in versions under maintenance. This new reference document tries to list all possible cases. Commits ------- 201de67 added a maintenance document

fabpot added the Status: Reviewed label Mar 30, 2016

Tobion reviewed Mar 30, 2016
View reviewed changes

HeahDude reviewed Mar 30, 2016
View reviewed changes

nicolas-grekas reviewed Mar 30, 2016
View reviewed changes

andrerom reviewed Mar 30, 2016
View reviewed changes

nicolas-grekas mentioned this pull request Apr 1, 2016

[WIP][FileSystem][Enhancement] Fallback to mklink for symlinks/junctions symfony/symfony#18324

Closed

9 tasks

linaori mentioned this pull request Apr 1, 2016

[Routing] Replace previously added preg_match with preg_replace_callback symfony/symfony#18379

Closed

alexislefebvre reviewed Apr 6, 2016
View reviewed changes

wouterj added Status: Needs Work and removed Status: Reviewed labels Apr 10, 2016

wouterj reviewed Apr 10, 2016
View reviewed changes

added a maintenance document

201de67

fabpot force-pushed the patch-version-docs branch from 016a49d to 201de67 Compare April 10, 2016 17:23

wouterj added Status: Reviewed and removed Status: Needs Work labels May 21, 2016

wouterj merged commit 201de67 into symfony:2.3 May 27, 2016

added a maintenance document #6412

added a maintenance document #6412

Conversation

fabpot commented Mar 30, 2016

fabpot commented Mar 30, 2016

javiereguiluz commented Mar 30, 2016

javiereguiluz commented Mar 30, 2016

javiereguiluz commented Mar 30, 2016

fabpot commented Mar 30, 2016

javiereguiluz commented Mar 30, 2016

javiereguiluz commented Mar 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiereguiluz commented Mar 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Mar 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Mar 30, 2016

stof commented Mar 30, 2016

HeahDude commented Mar 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh commented Mar 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Apr 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wouterj commented Apr 10, 2016

fabpot commented Apr 10, 2016

fabpot commented Apr 10, 2016

wouterj commented May 21, 2016 • edited Loading

javiereguiluz commented May 21, 2016

fabpot commented May 23, 2016

wouterj commented May 27, 2016

wouterj commented May 21, 2016 •

edited

Loading