Skip to content

Commit 879d4db

Browse files
authored
Introduce ADR's to the project (#1345)
* Introduce ADR's to the project * Added an ADR about extension points in the projects, implemented CR changes * Create a list to accepted/proposed/rejected ADs
1 parent 14ba687 commit 879d4db

File tree

8 files changed

+229
-0
lines changed

8 files changed

+229
-0
lines changed

documentation/adrs.md

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
# Architecture Decision Record
2+
3+
# What are ADRs?
4+
Architecture Decision Records (ADRs) are structured documents that capture significant architectural decisions made during the development of this project.
5+
6+
Each ADR explains:
7+
- **The context**: Why the decision was necessary.
8+
- **The decision**: What was decided and why.
9+
- **The consequences**: The impact of the decision, both positive and negative.
10+
11+
ADRs act as a single source of truth for important design and architectural choices, ensuring that everyone involved in the project has a shared understanding of why things are the way they are.
12+
13+
## How to Use ADRs in This Project
14+
**Follow Existing ADRs**: ADRs are mandatory to follow unless explicitly overridden by a new ADR. This ensures consistency in decision-making across the project.
15+
16+
> Whenever ADR is merged into the main branch, it is considered accepted and must be followed by all contributors.
17+
18+
**Propose New ADRs for Significant Decisions**:
19+
20+
- If you encounter a situation that requires a significant architectural or design change, you must create a new ADR.
21+
- Use the provided [ADR Template](/documentation/adrs/template.md) to create your proposal.
22+
- Submit the ADR via a pull request and engage in a discussion with maintainers and contributors.
23+
- **Document Decisions Transparently**: All significant decisions must be documented. This includes not only what was decided but also the alternatives that were considered and why they were rejected.
24+
25+
## Index of ADRs
26+
27+
### [Accepted AD](https://github.com/flow-php/flow/pulls?q=is%3Apr+is%3Aclosed+is%3Amerged+label%3AAD+)
28+
- [2025-01-07: Static Analysis Baseline](/documentation/adrs/static-analysis-baseline.md)
29+
- [2025-01-09: Extension Points](/documentation/adrs/extension-points.md)
30+
31+
### [Proposed AD](https://github.com/flow-php/flow/pulls?q=is%3Apr+is%3Aopen+label%3AAD+)
32+
33+
### [Rejected AD](https://github.com/flow-php/flow/pulls?q=is%3Apr+is%3Aclosed+is%3Aunmerged+label%3AAD+)
Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
# Extension Points
2+
3+
Proposed by: @norberttech
4+
Date: 2025-01-09
5+
6+
## Context
7+
---
8+
9+
Flow is a framework, and as all frameworks, it should provide extension points for developers to hook into the framework and extend its functionality.
10+
11+
How to find an extension point?
12+
13+
Easy, look for interfaces! For example take a look at `\Flow\ETL\Cache` which is
14+
an interface with few implementations. Thanks to the interface, you can easily
15+
create your own implementation and use it in the framework.
16+
17+
Most of the extension points exposed through `\Flow\ETL\Config` where you can
18+
replace a default implementations of various interfaces with those that better
19+
fit your needs.
20+
21+
Those extension points are there by design and should follow the backward compatibility policy.
22+
23+
Are there any other extension points that we should consider?
24+
25+
Yes... Unfortunately, each class that is not marked final and that is being somehow
26+
injected into the DataFrame must be considered as an extension point.
27+
28+
Why? Because nothing prevents developers from extending those classes and injecting
29+
their custom version into the DataFrame.
30+
31+
Those extension points can't be covered by our backward compatibility policy.
32+
33+
## Decision
34+
---
35+
36+
**Closed by default**
37+
38+
All classes should be marked as `final` and whenever we need to expose an extension point,
39+
we should do it through an interface.
40+
41+
**Always possible to open later**
42+
43+
In a justifiable cases, we can expose extension points through abstract classes or
44+
simply allowing people to extend the class. Those must always be approved by one of
45+
the core contributors.
46+
47+
**No guarantee and support for workarounds**
48+
49+
Developers can still use reflections or dedicated tools to "unfinalize" Flow classes,
50+
but we do not guarantee backward compatibility for those cases and everyone should
51+
do it at their own risk. As a Flow PHP maintainers, we also reserve the right to
52+
not provide any help or support for those cases.
53+
54+
## Pros & Cons
55+
---
56+
57+
- zero risk of breaking accidentally backward compatibility promise
58+
- more predictable behavior
59+
- reduced cost of maintaining backward compatibility
60+
- easier to find an actual extension points
61+
- impossible to mock classes that aren’t marked as `final` in tests (which is a good thing, users shouldn’t mock Flow classes in their test suites)
62+
63+
## Alternatives Considered (optional)
64+
---
65+
66+
Alternatively we could take similar approach to Symfony and mark classes as `final`
67+
[when it makes sense](https://github.com/symfony/symfony/issues/15233#issuecomment-1566682054)
68+
but this approach is too easy to get abused.
69+
70+
Plus, changing non-final class into a final one is much bigger BC break
71+
then the other way around.
72+
73+
## Links and References (optional)
74+
---
75+
76+
- [When to declare classes final](https://ocramius.github.io/blog/when-to-declare-classes-final/)
77+
- [Final Classes by default, why?](https://matthiasnoback.nl/2018/09/final-classes-by-default-why/)
Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
# Static Analysis Baseline
2+
3+
Proposed by: @norberttech
4+
Date: 2025-01-07
5+
6+
## Context
7+
---
8+
9+
After removal of Psalm and making PHPStan the main static analysis tool,
10+
we looked again into hardening the static analysis configuration.
11+
12+
We had three globally ignored errors in phpstan configuration
13+
14+
- identifier: argument.type
15+
- identifier: missingType.iterableValue
16+
- identifier: missingType.generics
17+
18+
All of the above were significantly reducing the value of static analysis and
19+
code quality.
20+
21+
One of the proposed approaches was to use a baseline file to suppress those errors
22+
in the existing codebase and gradually remove them.
23+
24+
There are few problems with this approach, but the most significant one is that once the baseline is introduced,
25+
it needs to be maintained.
26+
Whenever the code is changed, the baseline needs to be updated, which is an opportunity to also suppress new errors.
27+
This means that maintainers would not only need to go through the code changes
28+
but also through the baseline file, which is not the best use of their very limited and valuable time anyway.
29+
30+
## Decision
31+
---
32+
33+
We **must not** use baseline for static analysis.
34+
Instead, errors can be suppressed by annotations in the codebase or globally
35+
in the static analysis tool configuration.
36+
37+
Error suppression should be considered an edge case and should be used sparingly.
38+
Core contributors should review and approve all suppression annotations.
39+
40+
## Pros & Cons
41+
---
42+
43+
- The codebase will be cleaner and more maintainable.
44+
- More predictable and stricter types.
45+
- Less maintenance overhead related to managing the baseline.
46+
47+
## Links and References
48+
---
49+
50+
- #1329

documentation/adrs/template.md

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
# [Decision Title]
2+
3+
Proposed by: @author_github_username
4+
Date: YYYY-MM-DD
5+
6+
## Context
7+
---
8+
9+
`// Todo: Describe context here`
10+
11+
> What is the current situation or issue that needs addressing?
12+
> Why is this decision necessary?
13+
> Any relevant background or constraints.
14+
15+
## Decision
16+
---
17+
18+
`// Todo: Describe the decision here`
19+
20+
> What is the decision being made?
21+
> State it clearly and concisely.
22+
23+
## Pros & Cons
24+
---
25+
26+
`// Todo: Describe the consequences here`
27+
28+
> What are the outcomes or implications of this decision?
29+
> Highlight both positive and negative impacts.
30+
31+
32+
## Alternatives Considered (optional)
33+
---
34+
35+
`// Todo: Describe the alternatives here`
36+
37+
> Briefly describe any alternatives that were considered.
38+
> Why were they not chosen?
39+
40+
## Links and References (optional)
41+
---
42+
43+
> Include any supporting documents, links to issues, pull requests, or discussions that provide context.

documentation/contributing.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,11 @@ Even that we are supporting 3 PHP versions at time, we are using the lowest supp
66

77
For the code coverage, please install [pcov](https://pecl.php.net/package/pcov).
88

9+
### Before you change anything
10+
11+
Please make sure that you are aware of our [Architecture Decision Records](/documentation/adrs.md).
12+
It's mandatory to follow all of them without any exceptions unless explicitly overridden by a new ADR.
13+
914
### Prepare Project:
1015

1116
```shell

web/landing/assets/styles/app.css

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,4 +186,8 @@ code {
186186

187187
#documentation-page code {
188188
@apply text-orange-100;
189+
}
190+
191+
#documentation-page blockquote {
192+
@apply border-l-4 border-orange-100 p-2 bg-blue-300 mb-3;
189193
}

web/landing/src/Flow/Website/Service/Markdown/LeagueCommonMarkConverterFactory.php

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
use League\CommonMark\Extension\CommonMark\Node\Inline\Link;
1010
use League\CommonMark\Extension\ExternalLink\ExternalLinkExtension;
1111
use League\CommonMark\Extension\FrontMatter\FrontMatterExtension;
12+
use League\CommonMark\Extension\Mention\MentionExtension;
1213
use League\CommonMark\Extension\Table\TableExtension;
1314

1415
final class LeagueCommonMarkConverterFactory
@@ -20,13 +21,26 @@ public function __invoke() : CommonMarkConverter
2021
'open_in_new_window' => true,
2122
'noreferrer' => 'all',
2223
],
24+
'mentions' => [
25+
'github_handle' => [
26+
'prefix' => '@',
27+
'pattern' => '[a-z\d](?:[a-z\d]|-(?=[a-z\d])){0,38}(?!\w)',
28+
'generator' => 'https://github.com/%s',
29+
],
30+
'github_issue' => [
31+
'prefix' => '#',
32+
'pattern' => '\d+',
33+
'generator' => 'https://github.com/flow-php/flow/issues/%d',
34+
],
35+
],
2336
];
2437

2538
$converter = new CommonMarkConverter($config);
2639

2740
$converter->getEnvironment()
2841
->addExtension(new ExternalLinkExtension())
2942
->addExtension(new FrontMatterExtension())
43+
->addExtension(new MentionExtension())
3044
->addExtension(new TableExtension())
3145
->addRenderer(FencedCode::class, new FlowCodeRenderer(), 0)
3246
->addRenderer(Link::class, new FlowLinkRenderer(), 0);

web/landing/templates/documentation/navigation.html.twig

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,9 @@
2525
<li>
2626
<a href="/documentation/dsl#dsl-functions">DSL References</a>
2727
</li>
28+
<li>
29+
<a href="/documentation/adrs">Architecture Decision Records</a>
30+
</li>
2831
<li>
2932
<a rel="noopener noreferrer" target="_blank" href="https://github.com/orgs/flow-php/projects/1">Project Roadmap</a>
3033
</li>

0 commit comments

Comments
 (0)