[TASK] Rework chapters on reviewing and contributing patches and issues (#341)

* [TASK] Rework chapters on reviewing and contributing patches and issues
---------

Co-authored-by: Lina Wolf <[email protected]>
Co-authored-by: Stefan Frömken <[email protected]>

Author: 3 people

Documentation/BugfixingAZ/Index.rst

@@ -80,6 +80,10 @@ as described in :ref:`setup <setup>`. Especially the
    See :ref:`Testing the core <testing>` in TYPO3 Explained for more information
    about writing and running tests.
 
+   Once you have finalized your patch, check out the :ref:`common-review-checks`
+   for a list of what kind of review checks people may perform on your contribution.
+   Stay ahead of the game and address those yourself first.
+
 #. Commit your changes
 
    Please make sure that you read the :ref:`commitmessage` in the Appendix.
@@ -167,9 +171,10 @@ as described in :ref:`setup <setup>`. Especially the
 
    You can visit the link to https://review.typo3.org to see your patch in Gerrit.
 
-   If the automatically starting pre-merge build fails due to an error on Bamboo which
-   isn't caused by your patch (e.g. time out) you can restart it on
-   `Intercept <https://intercept.typo3.com/admin/bamboo/core>`__.
+   The Continuous Integration service, running on GitLab Pipelines, will automatically
+   be executed in the background and perform checks and tests on your patch.
+   Failing tests will be linked to that instance, where jobs can also be retried
+   (given sufficient permissions when being logged in to GitLab).
 
    Advanced users / core team only: See
    :ref:`cheat sheet: other branches <cheat-sheet-git-other-branches>`
@@ -193,6 +198,9 @@ dozens of requests each day, so expect a succinct response that is short and to
 You will get notified by email, if there is activity on your patch in Gerrit
 (e.g. votes, comments, new patchsets, merge etc.).
 
+Check out the section :ref:`reviewPatch` for more about this process, in which you can
+also be involved!
+
 It is not unusual for a patch to get comments requesting changes. If that happens,
 please respond in a timely fashion and improve your review. If things are unclear,
 ask in the **#typo3-cms-coredev** channel on https://typo3.slack.com.

Documentation/CoreMergers/Review.rst

@@ -13,7 +13,16 @@ Review a patch as a Core Merger
 .. include:: /_includes/CoreMergers.rst.txt
 
 Please see :ref:`reviewPatch` for general information on how to review a
-patch.
+patch, also the :ref:`cheat-sheet on common review issues <common-review-checks>`.
+
+As a Core Merger you have a huge responsibility because your vote (or misvote)
+has an impact on dedicated work of contributors or colleagues. This swings
+both ways: You can also greatly impact confidence and participation by being
+involved in a public voting and commenting process.
+
+Remember that you represent the TYPO3 project as a public-facing "Person with Power".
+Please try to always vote by explaining your reasoning, and feel free to
+correspond with your fellow Co-Mergers for impactful decisions.
 
 .. figure:: /Images/External/Gerrit/CoreMergers/VoteCoreMerger.png
    :class: with-shadow
@@ -69,7 +78,8 @@ Low brainers
 -------------
 
 A Core Merger can give a +2 and submit right away in case of "low-brainers"
-(what used to be called "FYI").
+(what used to be called "FYI" and relates to changes that are highly
+unlikely to negatively impact anything).
 
 A Core Merger can give a +2 and wait a bit before submitting
 (used to be called "FYI24", "FYI48", ...).

Documentation/HandlingAPatch/ChangeAPatch.rst

@@ -36,7 +36,9 @@ This chapter handles improving an existing patch. For creating a new patch, see
 
 4. Test your changes (optional)
 
-   Run the TYPO3 testsuite locally, as described under :ref:`testing`.
+   Run the TYPO3 testsuite locally, as described under :ref:`testing`. Otherwise,
+   don't worry, the automatic CI will do that for every committed patch set on the
+   TYPO3 infrastructure.
 
 5. Add files and amend to commit
 
@@ -54,6 +56,12 @@ This chapter handles improving an existing patch. For creating a new patch, see
 
    You can amend as often as you want.
 
+   Bear in mind that this kind of Git commit amending is a bit different than
+   when you do it with GitHub (and using `force-push`). For Gerrit, every push
+   will be a patch set. Previous patch sets can never be overwritten by amending,
+   so do not worry.
+
+.. _git-commit-with-message:
 6. Push your change to Gerrit
 
    Once you are satisfied, push your improved Patch Set to Gerrit:
@@ -62,3 +70,27 @@ This chapter handles improving an existing patch. For creating a new patch, see
       :caption: shell command
 
       git push origin HEAD:refs/for/main
+
+   Each pushed set will iterate a new patch set, and all previous patch sets
+   can always be compared and even checked out later on.
+
+   If you want, you can even provide an individual commit message to your push,
+   so that in gerrit your code change shows up with what usually a
+   "commit message subject" could do:
+
+   .. code-block:: bash
+      :caption: shell command
+
+      git push origin HEAD:refs/for/main%m="Some_String_Without_Whitespace"
+
+   The underscore characters will be replaced by whitespace. You can also
+   use the following bash script or shell alias to interactively do that:
+
+   .. code-block:: bash
+      :caption: push-with-message.sh
+
+      #!/bin/bash
+
+      read -p "Please enter pseudo-commit message: " answer
+      answer=$(echo "$answer" | tr -c '[:alnum:]' '_')
+      git push origin HEAD:refs/for/main%m=$answer

Documentation/HandlingAPatch/FindAReview.rst

@@ -91,6 +91,30 @@ Gerrit: Search
 
 Or use the search box on `Gerrit <https://review.typo3.org>`__
 
+GitHub: Links to Gerrit
+=======================
+
+Each commit to the TYPO3 Git repository comes with some metadata
+linking issue and patch reviews, for example:
+
+..  code-block::
+
+   [TASK] Provide PSR-7 Request in PolicyMutatedEvent
+   
+   For additional context does the PolicyMutatedEvent
+   now provide the current PSR-7 Request.
+
+   Resolves: #104141
+   Releases: main, 12.4
+   Change-Id: I1817366e77f20f6c43eef0ee209fbb419e7237e2
+   Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/84913
+   Tested-by: Lorem Ipsum <[email protected]>
+   Reviewed-by: Lorem Ipsum <[email protected]>
+   Tested-by: core-ci <[email protected]>
+
+The line `Resolves` contains the Forge issue id.
+The line `Reviewed-on` contains the link to the gerrit patch.
+
 Forge
 =====
 

Documentation/HandlingAPatch/ResolveMergeConflicts.rst

@@ -201,10 +201,11 @@ However, git status will show you:
 All files shown with "both modified" will need to be attended to.
 
 
-Resolve the conflicts
----------------------
+Resolve the conflicts manually
+------------------------------
 
-If you want to do it manually, look for all occurrences of `<<<<<<<`.
+If you want to do it manually (instead of using an IDE to do it visually), look for
+all occurrences of `<<<<<<<`.
 
 These are markers. They are used as follows (as in example above):
 
@@ -229,6 +230,33 @@ The merge conflict will show:
 
 There may be more than one conflict in a file!
 
+Special case: JavaScript/CSS merge conflicts
+--------------------------------------------
+
+JavaScript and CSS assets are build from sources in the monorepo, with the commands:
+
+.. code-block:: bash
+
+   Buid/Scripts/runTests.sh -s buildCss
+   Buid/Scripts/runTests.sh -s buildJavascript
+
+This "compiles" files with ".scss" and ".ts" extensions to their bundled ".css" and
+".js" variants. TYPO3 also versions these files inside the monorepo.
+
+Commiters need to be aware they need to maintain these asset versions, if they change
+any of the build source files, and commit them alongside their patch.
+
+Since the compiled files are merged on one single line, a merge conflict in these files
+will occur, if your patch works on anything CSS/JS related and other changes
+have been introduced meanwhile.
+
+The solution to resolve merge conflicts in these files is actually vers easy. Just
+re-perform the commands from above (`...buildCss/buildJavascript`), which will re-create
+the assets from your cherry-picked patchset. You may need to resolve conflicts in the
+`.ts/.scss` files beforehand, if there are any due to rebasing.
+
+Afterwards, you can include the generated `.css/.js` file your git amend commit like
+any other resolved conflict (see below).
 
 Resume command
 --------------
@@ -257,3 +285,14 @@ Resume git cherry-pick
 .. code-block:: bash
 
    git cherry-pick --continue
+
+Commit changes after rebase/cherry-pick
+---------------------------------------
+
+Once your git repository is in sync with `HEAD` and your cherry-picking was
+successsful, you can edit/add files, commit and push as usual:
+
+.. code-block:: bash
+
+   git commit --amend
+   git push

Documentation/HandlingAPatch/Review.rst

@@ -13,6 +13,9 @@ Reviewing a patch consists of two steps:
 
 You have the option to contribute to both stages or just a single stage in the review process.
 
+Both steps are free for everyone in the OpenSource community, you are not required to be
+a Core merger or Team member.
+
 If you're able to improve the patch yourself, your contribution would be very much appreciated.
 Visit :ref:`lifeOfAPatch-improve-patch` to find out more about how you can help improve patches.
 
@@ -35,6 +38,8 @@ Code Review
 
 A basic code review is possible by using the Gerrit web interface.
 
+For some tips on what to review, check our :ref:`common-review-checks`.
+
 .. rst-class:: bignums-xxl
 
 #. Select the latest patchset
@@ -127,6 +132,9 @@ Vote
 In order to comment or vote on a change you can click on the :guilabel:`Reply`
 and enter your comment. Here, you can also apply your votes.
 
+Remember, everyone with just a TYPO3 user account is able to vote, you do not
+need to be a team member or Core merger.
+
 .. figure:: /Images/External/Gerrit/CoreMergers/VoteUser.png
    :class: with-shadow
 
@@ -158,9 +166,11 @@ Policy for votes
 
 **Verified:** Needs :guilabel:`+1` of two reviewers, one of them being a Core Merger.
 
-Votes from the Bamboo build server (user *TYPO3com*) do not count. This means
+Votes from the CI GitLab Pipeline Server (user "Core CI") do not count. This means
 that a patch which is fully reviewed usually has at least 3 **Verified** :guilabel:`+1`
-votes, two from humans and one from Bamboo.
+votes, two from humans and one from "Core CI" ("Core CI is happy" for Verified+1, "Core CI is not happy"
+for Verified-1). Each comment by Core CI is linked to the log of the performed job,
+so that you can inspect the output.
 
 **Authors should not vote for their own patches**, unless the patch has been changed
 substantially by other developers.
@@ -194,7 +204,26 @@ be solved until this or that is fixed". Some hints on using -1 in reviews:
    a deeper knowledge of this subsystem, to take a look at it. I do not want
    this patch to be merged until this is sorted out and will vote -1 for now
    for this reason."
+*  If you are on the "receiving end" of a "-1" or even a "-2" vote, please do not
+   be afraid or feel bad. This is done as part of the process to make TYPO3
+   evolve as best as it can. Try to work out problems or negative feedback,
+   be kind to each other and remember you are contributing to OpenSource because
+   the experience of improving things together, and learning from each other
+   is what drives us.
+
+Vote resets ("Revote")
+----------------------
+
+Whenever a new patch is submitted, all votes are reset. So if previously
+a patch was voted to be merged, but then a last-minute issue is getting
+addressed, after the commit every vote will be back to zero.
 
+Contributors to a patch review will receive notifications about this and
+are encouraged to review the patch and re-apply their vote if they
+still feel confident.
+
+It is ok if the patch owner pings previous votes to please consider
+adding a revote, so that a patch can be pushed forwards.
 
 How to handle [WIP] patches
 ===========================
@@ -222,3 +251,24 @@ following this solution to its end."
 Having too many WIP patches in the review queue is not really helpful. Consider
 to fork the project in GitHub or somewhere else and push to gerrit again if
 your patches evolved.
+
+Stale reviews / Cleaning up
+===========================
+
+Since TYPO3 iterates on many, many patches and issues each day, the list
+of Gerrit reviews (as well as Forge issues) may feel intimidating.
+
+Due to this it is vital to "clean up" patches from time to time:
+
+*  If you are no longer planning to work on a patch, maybe better abandon
+   it, or ask other involved people to carry on.
+*  If your patch is not getting feedback for a long time, ask in the
+   #typo3-cms-coredev channel of the `TYPO3 slack workspace <https://typo3.slack.com>`__
+   if people may want to review or give feedback on. Or try to find people
+   working in a similar area of your patch and see if you can join forces.
+*  From time to time, check on patches you have voted on, to see if you can
+   push things forward to either get merged or abandoned.
+*  Sometimes just check all your own open patches and see if you might catch
+   interest in picking it up again.
+*  Please either update older patches in "Merge conflict" mode or state your
+   intent to abandon the patch.

Documentation/HandlingAPatch/Tips.rst

@@ -274,4 +274,44 @@ Voting
 
 Consider the :ref:`policies and tips for voting patches <gerrit-voting>`.
 
+..  _common-review-checks:
+
+Common code review checks
+-------------------------
+
+For reviewing and giving feedback, here's a couple of things that are often addressed. You can
+use this list both for checking other people's patches, as well as your own.
+
+*  Is the code-flow readable? Does it need more (or less) comments? Any code complexities
+   ("cognitive complexity") that could be easier to read when using different conditions/loops/sub-methods?
+*  Do breaking changes occur that need to be noticed? This can also apply to:
+   *  Type Hinting
+   *  using PHP features beyond the supported PHP version
+   *  Loss of existing functionality
+   *  Typos
+*  Are new class, method, function, variable names understandable
+*  Is there a need to add unit/functional testing for specific changes
+*  Are regression tests for a bugfix needed?
+*  Is the "Releases: " scope of a patch spanning the proper TYPO3 versions
+   (depending on the state of current LTS and priority-bugfix-only releases)
+*  Are the "final" and "private/protected" and "readonly" scopes of classes, methods and variables used properly?
+*  Is Dependency Injection used where applicable?
+*  Are possibilities for early code returns and reduced nesting levels addressed?
+*  If a change is Breaking or comes with a larger impact: Is a "Breaking.rst" or "Important.rst" document part of the patch?
+*  If a patch is a new feature: Is a "Feature"-rst part of the patch? Check out the `rst file generator <https://forger.typo3.com/utilities/rst>`
+   for help on creating files like this.
+*  Does the licensing of any foreign code introduced match the Core licensing?
+*  Is the commit message complete, clear and properly mentions all part of a patch?
+*  If a new Exception is added, is the timestamp-identifier unique and recent?
+*  When new PHP files are added, do they contain the TYPO3 License and a `declare strict_types` header?
+*  Is the provided commit message, reST files and the code itself aligned? Sometimes in the process
+   of reworking a patch multiple times, these three place of documentation can become out of sync.
+*  If SCSS/TS changes are involved, are the resulting build files included in the patch, and were built with the
+   right environment?
+*  Does your patch deprecate anything? If so, have you followed :ref:`deprecations`?
+
+If you feel this section is missing good things to watch out for for, please contribute to the
+documentation. This list does not claim to be complete, but should act as a kind of "Cheat-Sheet" for
+reviewing.
+