You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Review this pull request and provide inline feedback using the GitHub review system. Follow these steps:
47
-
48
-
1. Start a review: Use `mcp__github__create_pending_pull_request_review` to start a pending review.
49
-
2. Get diff information: Use `mcp__github__get_pull_request_diff` to see the code changes and line numbers.
50
-
- If the pull request has many changes, paginate the diff using the `page` and `per_page` parameters.
51
-
- Example: To get the first 100 lines, call `mcp__github__get_pull_request_diff` with `per_page=100` and `page=1`. For the next 100 lines, use `page=2`, and so on.
52
-
- Continue fetching pages until all changes are retrieved.
53
-
3. Add inline comments: Use `mcp__github__add_comment_to_pending_review` to provide feedback on specific lines of code snippets.
54
-
- **When suggesting code changes, format them as GitHub suggestion blocks so they can be committed directly:**
55
-
- For single-line suggestions, use:
56
-
```suggestion
57
-
suggested code here
58
-
```
59
-
- For multi-line suggestions, use:
60
-
```suggestion:-0+1
61
-
suggested code here
62
-
```
63
-
(Adjust the numbers to match how many lines to remove and add)
64
-
- Only provide suggestion blocks when you have a concrete, actionable fix
65
-
- Include explanation text before or after the suggestion block
66
-
4. Submit for review: Use `mcp__github__submit_pending_pull_request_review` with the event type set to "COMMENT" (not "REQUEST_CHANGES") to publish all comments for non-blocking review.
67
-
68
-
Focus on the following focus areas:
69
-
70
-
1. **Code Quality**
71
-
- Clean code principles and best practices
72
-
- Proper error handling and edge cases
73
-
- Code readability and maintainability
74
-
- **Provide suggestion blocks for improvements when possible**
75
-
76
-
2. **Security**
77
-
- Check for potential security vulnerabilities
78
-
- Validate input sanitization
79
-
- Review authentication/authorization logic
80
-
- **Suggest secure alternatives with suggestion blocks**
81
-
82
-
3. **Performance**
83
-
- Identify potential performance bottlenecks
84
-
- Review database queries for efficiency
85
-
- Check for memory leaks or resource issues
86
-
- **Offer optimized code via suggestion blocks**
87
-
88
-
4. **Testing**
89
-
- Verify adequate test coverage
90
-
- Review test quality and edge cases
91
-
- Check for missing test scenarios
92
-
93
-
5. **Documentation**
94
-
- Ensure code is properly documented
95
-
- Verify README updates for new features
96
-
- Check API documentation accuracy
97
-
98
-
Provide detailed feedback using inline comments for specific issues.
99
-
Use suggestion blocks liberally to make it easy for developers to apply fixes.
100
-
Use top-level comments for general observations or praise.
101
33
102
-
# Tools for comprehensive PR review
34
+
Review this pull request following the guidelines in docs/review-guidelines.md.
35
+
Provide detailed inline feedback using suggestion blocks where applicable.
- Follow PSR-4 autoloading under `CultuurNet\UDB3\` namespace
7
+
- Keep classes small and focused (Single Responsibility)
8
+
9
+
## PHP 8.1 Features
10
+
11
+
Use **readonly properties** with constructor promotion for immutable class properties.
12
+
13
+
See `src/Doctrine/DBALDatabaseConnectionChecker.php` for an example.
14
+
15
+
> **Note**: Do NOT use PHP 8.2 `readonly class` syntax - we're on PHP 8.1.
16
+
17
+
## Comments & Self-Documenting Code
18
+
19
+
Avoid comments in favor of self-documenting code:
20
+
21
+
-**Type hints**: Use typed parameters, typed properties and return types instead of docblock descriptions
22
+
-**Method names**: Use descriptive, self-explaining method and function names
23
+
-**Variable names**: Choose clear, meaningful variable names that convey purpose
24
+
-**Class names**: Use descriptive class names that explain their responsibility
25
+
-**File names**: Mirror class names for easy discoverability
26
+
27
+
Comments are acceptable when explaining **why** something is done (not **what**), such as workarounds for external limitations or non-obvious business rules.
28
+
29
+
## Naming
30
+
31
+
### Interfaces
32
+
33
+
- Descriptive name without `Interface` suffix (e.g., `DatabaseConnectionChecker`)
34
+
- Implementations: Prefix with technology (e.g., `DBALDatabaseConnectionChecker`)
35
+
36
+
### Exceptions
37
+
38
+
- Descriptive name without `Exception` suffix
39
+
- Name should describe what went wrong (e.g., `DocumentDoesNotExist`, `NewsArticleNotFound`)
40
+
41
+
## Testing
42
+
43
+
- Use `@test` annotation style for test methods
44
+
- Mock objects use intersection types: `Connection&MockObject`
45
+
- Prefer separate test methods over data providers for clarity
46
+
- Test file location mirrors source: `src/Foo/Bar.php` → `tests/Foo/BarTest.php`
47
+
48
+
## Gotchas & Pitfalls
49
+
50
+
| Issue | Solution |
51
+
|-------|----------|
52
+
| Commands fail outside Docker | Use `make` commands, not direct `composer` calls |
53
+
|`withConsecutive` deprecation | Avoid in new tests - use separate test methods |
54
+
| Uppercase `String` type hint | Legacy code - don't "fix" these |
55
+
| Tests fail after changes | Run `make ci` before committing |
56
+
57
+
## Restrictions
58
+
59
+
-**Never** modify files in `vendor/`
60
+
-**Never** commit `.env` or credentials files
61
+
-**Avoid** creating new service providers - extend existing ones in `app/`
62
+
-**Avoid** adding dependencies without team discussion
63
+
-**Always** run `make ci` before considering work complete
64
+
65
+
## AI Restrictions
66
+
67
+
-**Never** create commits - only humans commit code
68
+
-**Plan** larger changes but implement step by step, waiting for human review between steps
69
+
70
+
## Dependency Injection
71
+
72
+
- Service providers in `app/` wire all dependencies
73
+
- Use **constructor injection**, not service locators
74
+
- Extract testable logic into separate classes with **interfaces**
75
+
76
+
See `src/Doctrine/DatabaseConnectionChecker.php` and `src/Doctrine/DBALDatabaseConnectionChecker.php` for an example of interface + implementation pattern.
77
+
78
+
## Workflow
79
+
80
+
-**Small commits**: One logical change per commit
81
+
- A logical change can span multiple files (e.g., source code + tests, or a refactor touching several related files)
82
+
- All changes in the commit should belong together and serve a single purpose
83
+
- The goal is to make commits easy to review and understand
84
+
-**Small pull requests**: Easier to review, faster to merge
85
+
-**Feature flags**: Deploy often, enable features when ready
86
+
87
+
## Preferences
88
+
89
+
When working on this codebase, prefer:
90
+
91
+
- Extracting logic into testable classes with interfaces over inline implementation
92
+
- Constructor injection over service locator patterns
93
+
- Explicit code over clever abstractions
94
+
- Integration with existing patterns over introducing new ones
95
+
96
+
## Security
97
+
98
+
### Never Commit
99
+
100
+
-`.env` files or environment-specific configurations
101
+
- API keys, tokens, or credentials
102
+
- Private keys or certificates
103
+
- Database connection strings with passwords
104
+
- Any hardcoded secrets
105
+
106
+
### Configuration
107
+
108
+
- Secrets belong in environment variables, not in code
109
+
- Use `.env.example` or `.env.dist` for documenting required env vars (without actual values)
110
+
- Configuration files with secrets should be in `.gitignore`
111
+
112
+
### Secure Coding
113
+
114
+
- No hardcoded credentials or secrets in code
115
+
- No sensitive data in log statements
116
+
- Validate input on user-provided data
117
+
- SQL queries use parameterized statements (handled by Doctrine DBAL)
0 commit comments