Creating a pull request


(Christian Müller) #1

We rely on the regular github workflow of forking and pull requests (PR) which is described in the github help. If you don’t know how that works we recommend the github help that offers lots of guides to get you started working with github: https://help.github.com/

You can always ask the community for help while preparing a pull request if you are unsure about the github workflow, the following rules or any other aspect of contributing. We are happy to help!

All pull requests should stick to a single topic. Don’t work on more than one bug and/or feature in the same pull request unless the bugs all have the same cause and fix.

Which branch do I create the pull request for?
Features always go to the master branch unless there is a very good reason to put them into an already released version (usually to fix a bug that cannot be fixed without the feature).

Bugfixes on the other hand should go to the lowest supported version (that has the bug).
So for example if 1.2 and 2.0 are the maintained stable versions then a bugfix should go to 1.2 and will subsequently be merged into 2.0 and the development master by a team member.

We might ask you to change the branch your pull request would go to. That means you need to close the pull request and open another one to the requested branch. Avoid extra work by getting it right the first time.

If you’re unsure which branch to submit to, just submit it or ask in the development channels on Slack.

Commit Messages
We love it when at least your pull request message sticks to our commit message style but team members can help you with that after you created the pull request. The actual commit messages may be one-liners if the pull request message describes all changes inside sufficiently.

Labels
Please add labels (if you have the permission to do so) to your PR to help organize them. In any case, take note of labels added to your PR and act accordingly, if needed.

There are four label categories (Neos / Flow):

  • Component (Blue) – Neos, Content Repository, NodeTypes, Media, Kickstart / Flow, Fluid, Eel, Kickstart
  • Topic (Green) – PHP, JS, Documentation, Database, Testing, UX
  • Impediment (Orange) – WIP (work in progress), Needs feedback, Help wanted, Discussion (currently blocked by a discussion), Merge conflict

Additionally there’s a red label “Wrong branch” for indicating the PR was submitted for the wrong branch. See section above.

If assigning the WIP label, this means the PR should not be merged and usually people might not consider it for reviewing (because it is not done anyway). So, if you want to have feedback on a WIP pull request, also assign the Needs feedback label.

Automated tests and code style
All pull requests are automatically tested by https://travis-ci.org/ and cannot be merged if the tests fail. We expect that you ran the tests locally and made sure they completed successfully before creating the pull request.
Changes should be accompanied by tests that confirm the behavior to prevent bugs and to make the life of everyone easier.

Make sure you follow the coding style guidelines before opening the pull request as our style checker will block the pull request if the change doesn’t follow them.

Reviews
All PRs need to be reviewed before merging. Everyone is welcome to review and the more reviews the better. For merging there needs to be at least two :+1: votes before merging a change, for complex changes more votes can be necessary. Merging can be done by any team member with the needed permissions.

Discussion about a change happens on Github in the pull request and may result in the need to change the pull request, therefore please make sure you subscribe to notifications for your own pull requests.

Note that our time is limited and it might take a moment to check a pull request and get back to you. Don’t be discouraged if you don’t get quick answers. We will have a look at it!

Collaboration on PRs
If you want people to be be able to directly improve on a PR (e.g. to fix small things directly instead of simply leaving a comment asking you to do it), you can grant push access to your fork. A suggestion is to give all team members access to your fork, if you plan to contribute regularly.

Migrations
Flow and Neos bring multiple migration tools for separate areas:

If a change in code makes adaption of one of the above necessary you should provide a migration with your pull request. Again we can help you with that. Note that we currently ship migrations for MySQL and Postgresql so your pull request needs to provide both if a database migration is necessary.

Breaking Changes
Any change that needs manual work to keep existing code running is by definition a breaking change. This is automatically true for any changes in @api methods and interfaces. Those changes must be marked accordingly in the commit message (see details in the commit message styleguide) and should only go to master unless specific reasons make that necessary and it was discussed and agreed upon with active team members.

Code Quality
The Neos Project strifes for a high quality code base and thus we try to stick to principles like DRY, SOLID and Clean Code. Additionally we follow design patterns. All this shouldn’t block you from creating a pull request, we can help you to adjust the code if we feel improvements are possible.


Code contribution guideline
Release cycle / version numbers / breaking changes
[Doc] User interface language: How to set "system default"?
Code review process for pull requests
Code review process
Committers Guide
(Robert Lemke) #2

About breaking changes:

I think we should make it a rule that pull requests containing breaking changes can actually only be merged into master if it has been agreed upon in the project that the next version will actually be a major version.

It’s a non-trivial decision to release a major version and there must be a consensus about it. If the next version is clearly not a major version, breaking changes must not be merged under any circumstances.


Release cycle / version numbers / breaking changes
(Karsten Dambekalns) #3

Well, following SemVer features are definitely OK in a minor release, they must not be breaking, though.

But please, by all means, try to make even the one-liners explain their purpose.


(Christian Müller) #4

Suggestion:

I see we pile up pull requests that the original committer seems to not touch again, which are targeting master instead of the correct lower branch. As our adoption rate is quite low, I would rather see those changes merges in master now than waiting indefinitely for someone to backport it. Especially for edge case bugfixes, comments and documentation. So I would like to find a soft rule agreement to just merge those PRs to master after like 2 months of inactivity (so neither original commiter changed it nor anyone of us pledged to adopt the change).


(Aske Ertmann) #5

Yeah I agree, would suggest to add a soft rule of cherry-picking those bugfixes manually after merging if it makes sense and worth the effort.


(Johannes Steu) #6

Btw guys, as a “normal user” i am not able to set/add labels for my PR …


(Karsten Dambekalns) #7

Changed the wording slightly, to point out the possibility of not being able to assign labels…