Code quality and reviews

The main goal of your contribution to the foodsharing codebase should be to make the platform great for the users. Still, a very important aspect of that is to make the codebase great for developers as well so others can help with making it great for the users. That is why you should have a second goal with each commit: Make the code a little bit nicer than it was before. Below is a list of things that should be kept in mind when touching code but also when reviewing. When you think there is something touched that might break one of the points listed below, better delay the approval of the merge request or ask another person for a review.

What you should care about specifically

If you take responsibility it's okay to break master. Please try not to break it horribly 😄

We welcome new and beginner developers to contribute to foodsharing and understand that part of that might involve accidentally breaking bits of the site. And that is okay, as long as they stick around to fix what they broke. Still, try to be aware of what you are touching:

• Do not break things that affect non-beta users
  • Email notifications generated by actions of beta users are send to everybody
  • Modification of data, especially in stores, forums and walls, affect everybody as the content is shown on beta and production
  • An accidental loss of data is the worst case

Do not introduce security issues again

• Never write any new code using Foodsharing\Lib\Db class, always use Foodsharing\Modules\Core\Database with prepared statements
• When refactoring, take one step at a time. A lot of old code uses strip_tags as a basic Cross Site Scripting prevention method, it is hidden behind strval. Keep it when moving code.
• Always be aware what type of data is held in a variable: Plain text, HTML text, markdown? The old code does mostly not do this and is not even aware of the type when outputting it to the user. Still, when you want to change that behaviour, you must be aware of every single instance of that string used over the platform (e.g. it might be stored to the database or session and retrieved at other places). If in doubt, first try to leave that behaviour exactly as you found it and refactor as a separate step

REST API Endpoints

When new API endpoints are introduced or existing ones updated, reviewers need to pay special attention to these changes. First of all, make sure that the changes adhere to our API standards. But most importantly, you need to review permission checks. Make sure that only those who shall be permitted are actually able to use the API endpoint. You should also think about how the API can be used from user scripts, not only how it is actually used via the frontend.