Actions

Difference between revisions of "How to contribute new features"

From LimeSurvey Manual

 
(26 intermediate revisions by 4 users not shown)
Line 27: Line 27:
 
* New features are always on feature branches
 
* New features are always on feature branches
 
* New features are never merged into develop branch before QA
 
* New features are never merged into develop branch before QA
* Bug fixes are in bug branches and merged after they are tested by another developer
+
* Bug fixes are in bug branches and should always have a '''pull request''' on GitHub for code review and comments
 +
* Pull requests with a single author are squashed and merged
 
* Trivial fixes can be applied in the development branch directly (trivial fixes are e.g. one line or changing a localization typo in multiple lines)
 
* Trivial fixes can be applied in the development branch directly (trivial fixes are e.g. one line or changing a localization typo in multiple lines)
 +
 +
Most importantly, nothing is ever merged into master or develop branches without:
 +
 +
# QA
 +
# Code review
 +
 +
=How to create a pull request?=
 +
* Develop your feature or bug fix in a separate branch
 +
* Push your branch to the repository (internal developer) or push it to your personal fork (external)
 +
* Visit https://github.com/LimeSurvey/LimeSurvey and navigate to '''Pull requests''' tab.
 +
* Click on the '''Create pull request''' button
 +
* Select the base branch (new features should go to 'develop', bug fixes should go to 'master') and add your branch with your bug fix/feature.
 +
* The header of the pull request should be the same text as required in our [[Standard for Git commit messages]]. Please also enter a meaningful description of what you fixed.
 +
 +
=How to merge a pull request?=
 +
After a PR has been reviewed and tested, it will be merged by the responsible person.
 +
Please note the following rules:
 +
* When merging the PR on GitHub always squash it (there is an option in the merge button dropdown).
 +
[[File:Github_squash_merge.png]]
 +
* Always make sure that the squashed merge has a [[Standard_for_Git_commit_messages|proper commit message]].
 +
* If you fixed several issues in the same PR, they should all be part of the commit message text.
  
 
=Branch naming=
 
=Branch naming=
Line 37: Line 59:
  
 
[https://gitversion.readthedocs.io/en/latest/git-branching-strategies/gitflow-examples GitFlow examples].
 
[https://gitversion.readthedocs.io/en/latest/git-branching-strategies/gitflow-examples GitFlow examples].
 
=Rebase and merge=
 
 
When a branch is merged into master or develop branch, use the following procedure (using bug/15469-submit-invalid-part as an example bug branch):
 
 
[https://www.atlassian.com/git/tutorials/merging-vs-rebasing Source].
 
 
[[File:rebaseexample.png|rebaseexample]]
 
 
git checkout bug/15469-submit-invalid-part
 
git rebase develop
 
(Already OK in this case)
 
git checkout develop
 
git merge bug/15469-submit-invalid-part
 
(Fast-forward happening)
 
git push
 
 
Then delete the branch with:
 
 
git push -d origin bug/15469-submit-invalid-part
 
git branch -d bug/15469-submit-invalid-part
 
  
 
=Specification=
 
=Specification=
Line 66: Line 67:
  
 
New features must be tested by a QA-team or tester before being merged. The tester should not be part of the same sprint team that developed the feature.
 
New features must be tested by a QA-team or tester before being merged. The tester should not be part of the same sprint team that developed the feature.
 +
 +
=Documentation=
 +
 +
Make sure all PHPDoc blocks are correct.
 +
 +
All relevant manual pages should be updated before merge. Include information about from which version the new feature is available.
  
 
=Acceptance tests=
 
=Acceptance tests=
Line 89: Line 96:
 
</syntaxhighlight>
 
</syntaxhighlight>
  
This will all dev tools. Make sure to ''not'' commit any changes to the autoloader. The autoloader shoulder always be commited with the --no-dev switch on.
+
This will install all dev tools. Make sure to ''not'' commit any changes to the autoloader. The autoloader should always be commited with the --no-dev switch on.
  
 
==Code sniffer==
 
==Code sniffer==
Line 100: Line 107:
  
 
<syntaxhighlight lang="bash">
 
<syntaxhighlight lang="bash">
$ ./third_party/bin/phpcs tests/rulesets/phpcs_ruleset.xml <file to check>
+
$ ./third_party/bin/phpcs --standard=psr12 <file to check>
 
</syntaxhighlight>
 
</syntaxhighlight>
 +
 +
Example output:
 +
 +
<syntaxhighlight lang="text">
 +
---------------------------------------------------------------------------------------------------
 +
FOUND 273 ERRORS AND 98 WARNINGS AFFECTING 260 LINES
 +
---------------------------------------------------------------------------------------------------
 +
    2 | ERROR  | [ ] Missing file doc comment
 +
    3 | ERROR  | [ ] Missing doc comment for class QuestionEditorController
 +
    5 | ERROR  | [ ] Missing doc comment for function accessRules()
 +
  25 | ERROR  | [ ] Missing parameter comment
 +
  25 | ERROR  | [x] Tag value for @param tag indented incorrectly; expected 2 spaces but found 1
 +
  26 | ERROR  | [ ] Tag @return cannot be grouped with parameter tags in a doc comment
 +
  31 | WARNING | [ ] Line exceeds 85 characters; contains 90 characters
 +
</syntaxhighlight>
 +
 +
Setup in PHPStorm: https://confluence.jetbrains.com/display/PhpStorm/PHP+Code+Sniffer+in+PhpStorm
  
 
==Mess detector==
 
==Mess detector==
  
todo
+
Home page: https://phpmd.org/
 +
 
 +
Mess detector can detect unused variables, and code complexity.
 +
 
 +
Run it with
 +
 
 +
<syntaxhighlight lang="bash">
 +
$ ./third_party/bin/phpmd <file to test> text tests/rulesets/phpmd_ruleset.xml
 +
</syntaxhighlight>
 +
 
 +
Example output:
 +
 
 +
<syntaxhighlight lang="bash">
 +
application/controllers/QuestionEditorController.php:240    The method actionSaveQuestionData() has an NPath complexity of 8652. The configured NPath complexity threshold is 200.
 +
application/controllers/QuestionEditorController.php:240    The method actionSaveQuestionData() has 142 lines of code. Current threshold is set to 100. Avoid really long methods.
 +
application/controllers/QuestionEditorController.php:444    Avoid unused parameters such as '$returnArray'.
 +
application/controllers/QuestionEditorController.php:468    Avoid unused parameters such as '$returnArray'.
 +
</syntaxhighlight>
 +
 
 +
Setup in PHPStorm: https://confluence.jetbrains.com/display/PhpStorm/PHP+Code+Sniffer+in+PhpStorm
  
 
==Psalm==
 
==Psalm==
  
todo
+
Home page: https://psalm.dev/
 +
 
 +
Psalm makes sure your docblocks are correct, and that you never access anything that could be null.
 +
 
 +
Run it with
 +
 
 +
<syntaxhighlight lang="bash">
 +
$ ./third_party/bin/psalm <file to check>
 +
</syntaxhighlight>
 +
 
 +
Example output:
 +
 
 +
<syntaxhighlight lang="text">
 +
INFO: MissingReturnType - application/controllers/QuestionEditorController.php:54:21 - Method QuestionEditorController::actionView does not have a return type, expecting void
 +
    public function actionView($surveyid, $gid = null, $qid = null, $landOnSideMenuTab = 'structure'){
 +
ERROR: PossiblyNullPropertyFetch - application/controllers/QuestionEditorController.php:58:24 - Cannot get property on possibly null variable $oSurvey of type Survey|null
 +
        $gid = $gid ?? $oSurvey->groups[0]->gid;
 +
ERROR: PossiblyNullArrayAccess - application/controllers/QuestionEditorController.php:58:24 - Cannot access array value on possibly null variable $oSurvey->groups of type array<array-key, QuestionGroup>|null
 +
        $gid = $gid ?? $oSurvey->groups[0]->gid;
 +
</syntaxhighlight>
  
=Release process=
+
==Release process==
  
 
[[File:releaseprocess.png|Release process]]
 
[[File:releaseprocess.png|Release process]]

Latest revision as of 11:23, 25 February 2022

Contribution process

We are always encouraging users to contribute new features or fix bug they have developed. If you are planning to code a stunning new feature and you want it to be implemented at the core so you are future save when updating later, please proceed as follows:

  1. Use the latest Github version for your development
  2. Open a new ticket at our bugtracker at the good project part:
    1. Bug report for a bug
    2. Feature request for a new feature
    3. Development is more for fix the internal system
  3. If you already have fix : make a pull request at github. Remember the Standard for Git commit messages
  4. Describe your feature as detailed as possible and tell us about the implementation details (what code you want to add, which files you are planning to change, which GUI parts have to be extended, ...)
  5. We will then assign the ticket to one of our developers and discuss your approach. It's important to tell us about your plans before you start coding because otherwise you might take the wrong way when implementing new features so we might have to say "Sorry, we can't take that one (because of security problems or what so ever)".
  6. Once implementation details were discussed just start coding and create a Github pull request afterwards and ask for a developer to review your coding.
  7. Make sure to write unit or functional tests that tests your new functionality. If you don't know how to set it up, please read this manual page or ask in the #limesurvey IRC channel for help.
  8. There might be some further discussions about implementation details but once this was solved, your feature will be available in the next new version.

Please remember that new features should as much as possible be in new files.

Internal contribution process

(This section is only relevant for employees at LimeSurvey GmbH.)

As above, but using gitflow instead of pull-requests. Especially:

  • New features are always on feature branches
  • New features are never merged into develop branch before QA
  • Bug fixes are in bug branches and should always have a pull request on GitHub for code review and comments
  • Pull requests with a single author are squashed and merged
  • Trivial fixes can be applied in the development branch directly (trivial fixes are e.g. one line or changing a localization typo in multiple lines)

Most importantly, nothing is ever merged into master or develop branches without:

  1. QA
  2. Code review

How to create a pull request?

  • Develop your feature or bug fix in a separate branch
  • Push your branch to the repository (internal developer) or push it to your personal fork (external)
  • Visit https://github.com/LimeSurvey/LimeSurvey and navigate to Pull requests tab.
  • Click on the Create pull request button
  • Select the base branch (new features should go to 'develop', bug fixes should go to 'master') and add your branch with your bug fix/feature.
  • The header of the pull request should be the same text as required in our Standard for Git commit messages. Please also enter a meaningful description of what you fixed.

How to merge a pull request?

After a PR has been reviewed and tested, it will be merged by the responsible person. Please note the following rules:

  • When merging the PR on GitHub always squash it (there is an option in the merge button dropdown).

Github squash merge.png

  • Always make sure that the squashed merge has a proper commit message.
  • If you fixed several issues in the same PR, they should all be part of the commit message text.

Branch naming

All bug branches should be prefixed with bug/12345-short-description, where the number points to relevant Mantis issue on bugs.limesurvey.org.

All feature branches should be prefixed with feature/12345-short-description, where the number points to relevant Mantis issue on bugs.limesurvey.org.

GitFlow examples.

Specification

All new features should be specified using scenarios or user-stories.

QA

New features must be tested by a QA-team or tester before being merged. The tester should not be part of the same sprint team that developed the feature.

Documentation

Make sure all PHPDoc blocks are correct.

All relevant manual pages should be updated before merge. Include information about from which version the new feature is available.

Acceptance tests

Each new feature should have a folder with acceptance test. The folder name should include the ticket number, like

tests/functional/acceptance/15532-em-warning/<test classes>

The acceptance tests correspond to the user-stories written in the specification and guarantees that the functionality keeps working as intended even after further changes are applied to the code-base.

TODO: How to organize unit tests for new functionality?

Static analysis

(New in 4.3 )

It's a good idea to run some analysis on your code to make sure you have the correct docblocks, no unused variable, and apply PSR-2.

Run

$ ./composer.phar install

This will install all dev tools. Make sure to not commit any changes to the autoloader. The autoloader should always be commited with the --no-dev switch on.

Code sniffer

Home page: https://github.com/squizlabs/PHP_CodeSniffer

PHP code sniffer checks your code for style violations.

Run it with

$ ./third_party/bin/phpcs --standard=psr12 <file to check>

Example output:

---------------------------------------------------------------------------------------------------
FOUND 273 ERRORS AND 98 WARNINGS AFFECTING 260 LINES
---------------------------------------------------------------------------------------------------
    2 | ERROR   | [ ] Missing file doc comment
    3 | ERROR   | [ ] Missing doc comment for class QuestionEditorController
    5 | ERROR   | [ ] Missing doc comment for function accessRules()
   25 | ERROR   | [ ] Missing parameter comment
   25 | ERROR   | [x] Tag value for @param tag indented incorrectly; expected 2 spaces but found 1
   26 | ERROR   | [ ] Tag @return cannot be grouped with parameter tags in a doc comment
   31 | WARNING | [ ] Line exceeds 85 characters; contains 90 characters

Setup in PHPStorm: https://confluence.jetbrains.com/display/PhpStorm/PHP+Code+Sniffer+in+PhpStorm

Mess detector

Home page: https://phpmd.org/

Mess detector can detect unused variables, and code complexity.

Run it with

$ ./third_party/bin/phpmd <file to test> text tests/rulesets/phpmd_ruleset.xml

Example output:

application/controllers/QuestionEditorController.php:240     The method actionSaveQuestionData() has an NPath complexity of 8652. The configured NPath complexity threshold is 200.
application/controllers/QuestionEditorController.php:240     The method actionSaveQuestionData() has 142 lines of code. Current threshold is set to 100. Avoid really long methods.
application/controllers/QuestionEditorController.php:444     Avoid unused parameters such as '$returnArray'.
application/controllers/QuestionEditorController.php:468     Avoid unused parameters such as '$returnArray'.

Setup in PHPStorm: https://confluence.jetbrains.com/display/PhpStorm/PHP+Code+Sniffer+in+PhpStorm

Psalm

Home page: https://psalm.dev/

Psalm makes sure your docblocks are correct, and that you never access anything that could be null.

Run it with

$ ./third_party/bin/psalm <file to check>

Example output:

INFO: MissingReturnType - application/controllers/QuestionEditorController.php:54:21 - Method QuestionEditorController::actionView does not have a return type, expecting void
    public function actionView($surveyid, $gid = null, $qid = null, $landOnSideMenuTab = 'structure'){
ERROR: PossiblyNullPropertyFetch - application/controllers/QuestionEditorController.php:58:24 - Cannot get property on possibly null variable $oSurvey of type Survey|null
        $gid = $gid ?? $oSurvey->groups[0]->gid;
ERROR: PossiblyNullArrayAccess - application/controllers/QuestionEditorController.php:58:24 - Cannot access array value on possibly null variable $oSurvey->groups of type array<array-key, QuestionGroup>|null
        $gid = $gid ?? $oSurvey->groups[0]->gid;

Release process

Release process

The release cycle consists of two phases:

  • Merge window
  • Feature freeze

During the merge window, new features are accepted into the develop branch.

During the feature freeze, only bug fixes are accepted.

At the end of the feature freeze, the develop branch is merged into master and a new version is released. After that, a new release schedule is written and announced.

Also see current release schedule.

Useful links