---
title: "TYPO3 Extension Patch Review"
description: "Skill: typo3-extension-patch-review"
canonical: index.html
navigation-title: "TYPO3 Extension Patch Review"
---
# TYPO3 Extension Patch Review
- [Markdown source](#markdown-source)
- [References](#references)
- [Where every task starts](#where-every-task-starts)
- [Incoming change review checklist](#incoming-change-review-checklist)
**Skill:** `typo3-extension-patch-review`
Judge one incoming change against a TYPO3 extension, sitepackage or project
package — a GitHub pull request, a patch, a branch somebody proposes — and say
what stops it being merged, from its versions to its commit message and its
checks. It stops at the verdict; the whole repository is
typo3-extension-health's.
## Markdown source
```markdown
---
name: typo3-extension-patch-review
description: 'Judge one incoming change against a TYPO3 extension, sitepackage or project package — a GitHub pull request, a patch, a branch somebody proposes — and say what stops it being merged, from its versions to its commit message and its checks. It stops at the verdict; the whole repository is typo3-extension-health''s.'
compatibility: Needs the typo3-dev-companion MCP server, which owns every lookup this workflow routes to and publishes this skill together with the references/base.md it opens on. Install it from github.com/TYPO3/dev-companion and run typo3-dev-companion install in the project. A copy taken out of that repository's skills directory alone has neither the tools nor that base file.
---
# TYPO3 Extension Patch Review
Judge one change proposed against a package that is not the core. Report what
stops the merge. Keep this skill as routing and review method. The conventions,
the version boundaries and the commit rules are lookups. A copy of them here is
one nobody can correct.
## Establish the change, then the range you judge it in
1. Work through [references/base.md](references/base.md). It fixes the order
every task here starts in, and a review is where that order decides the
result. A rule you fetch after you read the diff confirms a reading instead
of a test of it.
2. Read [references/checklist.md](references/checklist.md) for the review
surfaces, what a finding owes, what a dropped candidate owes, and the
severity rubric.
3. Establish the change itself. This server reads neither your git state nor the
forge the change arrived on. What it needs from you is exactly what the
change is. That is **the changed paths**, **the diff**, **the branch it
targets** and **the commit messages**. It is also **whether it merges and
what the repository's own pipeline said**.
One reading produces all of them. `git fetch [` and
`git diff ...` give the first four. The forge's own client or its
web page gives the last. Every lookup below takes one of them as its
argument.
The constraint the package declares for the core is the axis every answer below
turns on. The installation the review runs against supplies one point in it. A
finding you establish there holds for that point. The base's first step
discharges `typo3_project_describe`, because its answer already carries that
constraint and the commands this repository declares.
## Whether the change is right where it lands
The changed paths are the argument, not the subject. Pass them to
`typo3_hint_lookup`, one call per subsystem the diff touches. Do that before you
form a view of whether the code is right. A query that describes the review
reaches nothing.
The hints match by path and by subsystem. A sentence about how you judge a
change comes back as the index of ids it did not return.
Enumerate what the diff **removes or renames** before you ask. A package has its
own public surface, and it is not only PHP. It is a TCA field, a TypoScript
path, a Fluid partial or section, a ViewHelper argument, a label key. It is a
signature somebody else's template or override calls.
That is the class of finding this review exists for. A read of the new code does
not surface it. The evidence is in what is gone.
Whether a member that stays is public API, and what its signature is, is the
checkout's answer, not a lookup's. That reading belongs in the base's step after
the lookups. A finding built on it names the version you read it at.
## Whether it holds on every version the package declares
A change you confirm against the installation holds on one of the declared
majors. You settle the others by reading. The procedure exists whole rather than
as sections a search would cut it into:
- `typo3_rule_lookup` with
`documentId="extension/compatibility/a-declared-major-that-is-not-installed"`.
It says which majors the question is about, what the changelog settles and
where it stops. It gives the invocations that read one symbol off the branch
that carries the other major. The question is per symbol rather than per
package.
- `typo3_rule_lookup` with
`documentId="extension/compatibility/running-on-a-declared-major-that-is-not-installed"`.
That is for a claim you have to run rather than read. It says what the
repository's own pipeline already covers. It says how a Composer root of its
own stands the other major up beside the installation.
`typo3_changelog_lookup` narrows that question and does not close it. It answers
change events. So a member added later to a class that was already there leaves
no entry. Neither does a widened signature.
It also reads the changelog off the installed core. So a bare clone of the
package under review has none to read. The answer then says it could not answer
there, which is not the same as an empty one. Say which of the two you got.
`typo3_documentation_lookup` with `targetVersion` per declared major answers
"does this still work there" for a documented surface. The silence of a
changelog does not.
## Whether a core API already does what the change hand-rolls
The conventions lookup above answers most of this by itself. Where the core has
an API for the construct, the hints for that subsystem carry it and its majors.
A change that reaches past it is the finding.
Two things settle the rest, and neither is a recollection:
- **Sweep the package and the installed core for the call sites before you
propose an alternative.** Whether an idiom has a foothold is a count, not a
taste, and no lookup here holds it. The answer is the call sites at their
paths and lines, and one of them is a coincidence.
- `typo3_system_extension_lookup` before you recommend an API that lives in
another extension. It says whether that extension is part of the core on the
majors the package declares. Where it is not, the recommendation adds a
dependency, and the finding has to say that.
## The commit message, against the convention this repository writes
Which convention that is comes out of the repository's own log, which this
server does not read. Establish it before you check anything.
Where the repository writes TYPO3 keywords, call `typo3_commit_message_guide`
with `workflow="project"` and the message under review. It says whether the
subject, its length and the body wrap hold. Where the repository writes another
convention, that guide answers about TYPO3's instead. It reports a subject
without a keyword as an error. To pass that on is a finding about a repository
nobody reviews. Say which convention you judged the message against.
## What is green, and what that is worth
Whether the branch merges and whether the pipeline passed are the forge's
answers. Read them there and report them as read. Then run what this repository
declares as checks. The base's first step marked each declared command a check,
a change or unknown.
A check hands the code back as it was, so a review told to change nothing runs
it. A green pipeline covers the entries its own commands cover and says nothing
about the ones it has none for. **Write out, by name, the checks you did not
run.**
## Report
Order by what stops the merge, and say why each one stops it:
1. what blocks the merge;
2. what the maintainer would send it back for;
3. what is worth a change and would not block it;
4. what you checked and found correct, briefly, so a reader can tell a silent
surface from a verified one;
5. what you raised while you read and dropped, with what dropped it.
Every finding names the changed path it is about. A statement about the package
that ties to no line in this diff belongs in an issue. Close on the checklist's
surfaces with each one marked assessed, unassessed or not applicable to this
diff. A reader cannot tell a review that reports only findings from one that
looked at less.
**The report is markdown the reader can copy, and the answer is where it goes.**
Everything this section asks for makes it long, and length makes the form
matter. Somebody carries a review into a thread, an issue or a chat, and
rendered output does not survive the move. Write it to a file only where the
caller asks for one, at a path outside the checkout under review. A modified or
untracked file beside the change is a surface this review reports on.
## Where the review ends
**When the user asks you to make the change, invoke `typo3-extension-health` and
work from it.** That includes the commit. An instruction to change the package
asks for it: "fix it", "do the first three", "push that".
It arrives in the middle of a session that goes well. That makes it easy to
carry on under review rules while you rewrite what the review was about. The
same skill takes the request when it widens past this change. That is "audit the
package", "what else is wrong in here".
It owns the surface list you read a whole repository against. To run that list
on one diff is what this workflow exists not to do. What crosses over either way
is the paths and what you already established about them.
**A remark about a finding's weight is not that instruction.** "That one blocks
it", "are you sure", "I would reject it for that": each reaffirms a finding and
commissions nothing. Where the sentence could be either, ask which the user
meant.
This skill owns the judgement of one change proposed against a package that is
not the core. It stops at the verdict. The repository around that change belongs
to `typo3-extension-health`. That is what else is wrong with it, and the
committed changes that answer a finding.
```
## References
### Where every task starts
```markdown
# Where every task starts
## Nothing starts until the server answers
A skill is a file the installer left behind. It loads and reads the same whether
the tools behind it are there or not, and neither side notices. So the first
call below is also the check.
- A client may carry this server's name in each tool's name:
`mcp____typo3_project_describe`. So a search for the bare name comes
back empty where the server is there. A search for a tool's schema needs the
same form. A `select:` on the bare names returns nothing where the tools are
there. Look for the qualified form before you read an empty result as an
answer about the server.
- No `typo3_` tool in this session, or a first call that errors: stop. Say that
this workflow needs the server and it is not there, and name what came back.
- Do not fall back to general TYPO3 knowledge, and do not start to read the
checkout. That answer carries this workflow's order and confidence and none of
its evidence. Nothing in it says which of the two it is.
- Continue only when the user asks you to after you said so. Repeat it in the
answer and in every finding a lookup would have carried.
## The order
This is an order rather than a list. Each step decides what the next one is
worth. Where a step below carries a condition to skip it, that condition is
narrow on purpose. A skipped prescription teaches the next reader to skip the
ones that matter too.
1. **`typo3_project_describe`** — the repository and whether it holds an
installation yet. It reports the TYPO3 and PHP version, the project's own
extensions, its sites, and the commands this repository declares. That
version filters every later answer. A check the repository does not declare
is a wrong answer however sensible it sounds.
The answer ends with the whole procedures this server carries, as ids. That
list is the only place a client that renders no resource list sees their
names. Each one is a `typo3_rule_lookup` with that `documentId` rather than a
search.
2. **`typo3_extension_describe`** for each extension in scope. It says what the
extension registers, and what it ships beside that. That is its manual, its
README, its test layers, and its XLF files with the source language each one
declares. It also says what the extension does *not* ship, and that is the
half no file listing gives you.
Where step 1 reported no extension, that answer is this step, and there is
nothing to call. Say so. A core checkout is that case, because step 1 names
the project's own extensions and not TYPO3's.
3. **`typo3_task_guide`** with a short English task, the paths it touches, the
target version and the change type. It answers the workflow this task belongs
to and the checks that come with it.
Run it in every session, this skill's own tasks included. The guide builds
the brief from the paths as well as the task text. No skill knows which paths
the caller holds.
A skill that covers the task is not that brief. A skipped step costs the
hints and the core checks those paths match. Where the guide's own answer
named this skill, this is one call for an answer already in the session. The
price of a step there is nothing to decide about.
4. **`typo3_hint_lookup`** for each subsystem in scope, with its concrete paths.
One query per subsystem. A single broad query is not subsystem evidence.
Where step 3 ran with those paths, its answer says whether you still owe this
step. A brief that carried everything the lookup matched says so: "these are
everything typo3_hint_lookup matches for these paths". There the guide made
the call, and the same query returns the same hints. A brief that stopped
short says that instead and names the ids it left. You owe those: fetch them
by id rather than repeat the query.
Read the sentence rather than the populated `hints` key. That key is present
either way and does not tell the two apart. `omittedHints` is that sentence
as data. It is empty where the brief carried everything, and it holds the ids
the brief left where it stopped short.
5. **`typo3_changelog_lookup` with `type: deprecation`**, at each major the
package declares. Omit the query and raise `limit` to carry that major whole.
Those two are the changelog's own axes, and the extension's vocabulary is not
among them. An entry carries a query only when its title carries every word
of it at once. The core titled those entries about its own code.
That is one call per declared major, and what comes back is the major. Every
entry carries its own index tags. `ext:core`, `ext:frontend`, `ext:form` and
the rest name the system extension a change is **in**. `TCA`, `TypoScript`,
`Fluid`, `YAML`, `Backend`, `Frontend` name the surface.
Step 2 picks the package's entries out of that answer by those tags. The tags
are the system extensions it requires, renders through or registers into, and
the kinds of file it ships. That costs no further call. An extension key of
your own is not among them and matches nothing. `tag` narrows one question
inside a major rather than composes the sweep out of eleven.
You check the answers against step 2, which is the other half the words did.
Verify each identifier that comes back in the checkout. A deprecation nothing
here calls is not a finding.
Carry the `FullyScanned` / `PartiallyScanned` tag into the answer. It says
whether the Extension Scanner can find the remaining call sites or whether
that reading is yours. Bounded this way, you can write the sweep before you
open a file. That is why it is a step of the order rather than something the
reading stumbles into.
**What its silence is worth.** A changelog records change events. So a
pattern nothing has touched for ten majors has no entry at all. An empty
sweep is therefore not an answer about what still works. "Does this still
work in version N" goes to `typo3_documentation_lookup` at that version. Ask
it here, and whenever the reading raises it again.
That is a question for a documented surface: a ViewHelper, a TCA type, a
TypoScript setting. The manual matches page titles, section paths and what
each manual declares by name, never the text of a page. Declared is a
property, a class or method the manual documents, a console command. You
reach one by its own name where the query writes that name the way code
does. You also reach it where the query is nothing but the name. A PHP
identifier the manual does not declare has no page named after it.
An identifier goes to `typo3_changelog_lookup` under its own name. That
reaches the entries that write it, however the core titled the change. Then
it goes to the class below. Where the manual has no page for a surface
either, that is a result and not an answer. Undocumented is not unsupported.
**A second declared major.** A package that declares more than one asks a
second question of every deprecation the sweep returns. Is the replacement on
the lower one? The entry's `issue` is a query of its own, and it reaches
every entry filed under that number. The Feature the core announced the
replacement in is among them.
The version the core released it in settles that question. Where the number
reaches no sibling, nobody wrote an entry for the replacement.
`typo3_rule_lookup` with
`documentId="extension/compatibility/a-declared-major-that-is-not-installed"`
is the reading that closes it.
**Where you do not owe the sweep.** A task that produces no change does not
reach this step at all. The property is what the task produces. A triage, a
reproduction and a review illustrate it; they are not the list you read it
off. The sweep asks what a package will have to stop calling. A task that
writes nothing is not going to call anything.
The exemption ends where the workflow produces a change. A review asked to
make the change is that other workflow. It starts this order again with the
files it is about to write. To carry somebody else's patch onto current code
is on the same side. It writes commits. The sweep says whether the code that
moved under the patch deprecated something the patch calls.
Skip the sweep only where the change touches no TYPO3 API: a code style
fixer, a CI file, an `.editorconfig`. A deprecation is a statement about API
the package calls. So a change that calls none has nothing for the sweep to
land on. The sweep is empty before it runs.
That condition is worth a statement, because this step is the largest answer
the order asks for. It is one call per declared major, with that major's
deprecations whole. You read which side a change falls on off the files it
touches, never off the task it started as. One PHP file edited along the way
puts it back among the ordinary ones.
A skip there costs the deprecation no finding would have walked into. How
small the change is decides nothing either. Three statements can call a
deprecated API as easily as three hundred.
A test file is one of those wherever it sits. It calls the API it exercises
and the framework around it, and both deprecate. A fixture is exempt where it
is data the suite reads, and not where it is a class.
**Before the reading**, write down what the order established. That is the
version that filters every later answer, the packages in scope, and the commands
this repository declares. Write down which steps what discharged. Those are
answers already in the session rather than a second reading. What the files show
belongs to the report at the other end. A caller who cannot see what an answer
rests on cannot tell it from one that rests on nothing.
**Then** read the checkout. Not before. A file list first makes everything after
the list look optional. The conventions then arrive as a footnote to a verdict
that has already formed.
**Before the first edit**, name the files this change will create, change or
delete. A deletion is the caller's to ask for, and this is somebody else's
checkout. It is the one act nothing here can put back.
**Last**, the report names every step of this order it did not reach, and what
stood in for it. That is an answer already in the session, a condition that made
the step empty, or an exemption. A reader cannot tell a step passed over in
silence from one somebody dropped.
## When the lookups run out
A behaviour question that survives the lookups above is one you read out of the
installed source. Do not guess at it. The class that implements the behaviour
and the one it inherits from answer it. That reading is the step after the
lookups. It replaces a change to the code until it works.
A first change that did not work is evidence about the reading. So the second
attempt at one failure reads the source rather than changes the code again.
What it settles is what this installation does and never what TYPO3 supports. So
a finding says that you could not settle the question beyond the version
installed. An answer built on the reading names the version it holds for.
## What each runtime lookup adds after the extension answer
`typo3_extension_describe` in step 2 says what one package registers. The
lookups below say what the installation resolved. That is a different fact even
where the words are the same. So step 2 has made none of these calls:
- `typo3_backend_module_lookup` — the tree position, the labels, the access
level, the routes and the navigation component the parent module supplies.
Step 2 lists the modules the package declares. A declaration cannot show that
inheritance.
- `typo3_icon_lookup` — whether any installed package registers an identifier.
That validates the ones a template uses. Step 2 lists the identifiers this
package contributes.
- `typo3_label_lookup` — the labels as the installation resolves them, with its
overrides applied. Step 2 lists the package's XLF files and the source
language each declares, never what a unit says here.
- `typo3_fluid_namespace_list` — the prefixes any template may use without a
declaration, from every package at once. Step 2 lists the package's own
declarations. So an empty list there is no evidence that no package registers
a prefix globally.
- `typo3_configuration_lookup` — the resolved configuration value, after every
extension has had its say. For a form data group it gives the order the
providers really run in. Step 2 answers nothing about that surface at all.
What a registration declares is not what the installation resolves.
- `typo3_service_lookup` — the class the container really injects for a service
id, an interface or a tag. Decorations and overrides count. Step 2 lists what
the package's own `Services.yaml` declares, never what won.
- `typo3_schema_lookup` — the columns TYPO3 derives for a table from its TCA. It
gives the type, the nullability and the default each one gets. Step 2 lists
the tables the package registers and nothing about their shape.
- `typo3_flexform_lookup` — the data structure the installation resolves a
`type=flex` field to, sheet by sheet, with listeners and migrations applied.
Step 2 lists the content elements a package registers, never the structure
each one's form builds.
- `typo3_record_lookup` — the rows of any table the installation has TCA for. It
says how many there are and where they sit. It says what one column holds
across them and which rows depart from its default. Step 2 has no row in its
answer at all.
None of these says whether what it reports is right. `typo3_hint_lookup` and
`typo3_documentation_lookup` do. A subsystem its own runtime lookup confirmed
can still break every rule that governs it. So it is not established until you
asked both.
## A rule reads in both directions
It says what new code should do, and it says what this checkout already does
wrong. A file that settled into the opposite of a rule is a finding, not a local
style to preserve. Consistency with a project's own habit establishes nothing
about whether the habit is right.
## What the code is for is evidence, and the repository states it
A mechanism that costs something is not a defect because it costs. Before you
report one, find what it is there for and say so. That is the manual, the
README, the changelog, the setting that drives it, or the declared versions.
Where the documentation states a purpose, what you have is a trade-off, not a
defect. Name it with its cost and its alternative. Where you cannot find one,
the finding says that you could not establish one, not that none exists. If you
skip this, your review is a list of everything the author did on purpose.
## What a finding rests on is part of the finding
Three things carry one. A file you read, at its path and its line. A command you
ran, with what it printed. A mechanism you traced into an installed package. Say
which of the three it is. If you leave it unsaid, a finding from a CI file
weighs as much as one with a verified line.
Where one of the project's own commands would settle it, run it.
`typo3_project_describe` marks each command it lists **check**, **change** or
**unknown**, read off the declared body. A check reports and hands the code back
as it was. So even a task told not to change files runs it. The linter the
repository already declares is the cheapest evidence in it.
Do not run a change under that instruction. Name an unknown in the answer as
evidence that is available, and do not run it unasked. An unknown is a test
suite, a shell pipeline, a console command.
What a check prints is not the finding. The configuration that makes it fail is
still what the finding is about. The run takes that finding from derived to
established.
## What this server does not know
It does not read your working tree. You establish which files changed, which
branch you are on, and whether a path or an identifier still exists there. Then
pass the concrete paths back, because that turns a general convention into an
answer about this code.
## Query it in English
The knowledge is English and the match is lexical. So a query in another
language reaches the loanwords the two happen to share and nothing else.
Translate the subject before the call and the answer back afterwards, whatever
language you speak with the user.
```
### Incoming change review checklist
```markdown
# Incoming change review checklist
The surfaces below are the work list. The coverage the report closes on is this
same list with every entry answered. You write it from the diff. An entry the
diff does not touch is not applicable and costs the line it costs here. One it
touches and nobody read has no assessment, which is not clean.
## Review surfaces
- **The change itself.** What it claims to do, what it does, and whether the two
are the same thing. Where it changes what the frontend or the backend form
renders, the diff says what it sets. It says nothing about what comes out.
- **What it removes or renames.** The package's own public surface: PHP members,
TCA fields and types, TypoScript paths, Fluid templates, partials and
sections. It is also ViewHelper arguments, label keys, site set settings,
database columns. Whoever installed the package calls them.
- **The declared range.** Every TYPO3 and PHP version the package's manifest
claims, not the one the installation happens to be. A change you verify on one
point of the range holds there.
- **The API it reaches for.** Whether the core already offers the construct, on
every declared major. Whether the form the change uses survives the next one.
- **Persisted data and editors.** Where the diff touches TCA, the schema or a
migration. What happens to rows that already exist, and what the editor sees.
- **Labels and icons.** A user-facing string or an identifier the diff adds.
Whether it resolves in the installation rather than only in the file.
- **Security.** A user-controlled value the diff moves, and the sink it reaches.
- **Coverage.** A test that fails before the change and passes after it. Where
the package's suite has no layer that could carry one, that absence.
- **The commit message**, against the convention this repository writes.
- **Merge state and checks.** Whether the branch merges, what the pipeline ran,
and which of the repository's own checks you ran here.
## Severity
The bands are the merge decision, because that is what the review is for:
- **Blocks the merge** — the change is wrong, breaks a version the package
declares, or loses data. It opens a security boundary, or removes a public
surface without a migration path.
- **Send it back** — the change works and is not ready. That is a defect in a
case it covers, or a missing test for risky behaviour. It is a hand-rolled
construct where a core API exists, or a message that does not meet the
convention.
- **Worth a change** — a concrete cost that does not stop the merge.
- **Recommendation** — a beneficial improvement with no verified violation.
Severity follows the demonstrated consequence and not the size of the diff. A
one-line change that breaks a declared major blocks. A large change that only
moves code does not.
## What a finding owes
A concrete location in the diff, and what the code there does. The rule,
documentation or reading that says it is wrong. The consequence, and what would
remediate it. Short of those it is a question rather than a violation. A
question reported as a violation costs the author exactly the reading the review
skipped.
Say what the finding rests on. That is a line you read, a command and its
output, or a mechanism you traced into an installed package. A finding from a
pipeline configuration and one with a verified line are not worth the same. A
report that does not separate them says they are.
Say also whether **this change** introduced it. Report a defect the diff only
stands next to as pre-existing, with that word. To ask an author to fix what
they did not break is a different request. It is theirs to decline.
Where nobody here can settle a claim on a version nobody can run, the finding
says so. It names the reading or the run that would settle it. Unverified is a
result. A confident sentence in its place is not.
## What a dropped candidate owes
A review drops more than it reports, and nothing records the drop. Name each
candidate you raised while you read and then let go, with what let it go. That
is the guard that was there after all, the default that was the core's, the
class you read. One sentence each, beside the findings.
The two directions do not meet the same bar. To raise a candidate costs a
reading. To drop one costs the maintainer a finding, silently. So drop a
candidate only where something concretely disproves it. Report one you can
neither establish nor disprove as open, with the reading that would settle it
named beside it.
Two dismissals go wrong reliably. One drops a candidate because a comment or a
docblock says the code behaves that way. That is a sentence somebody wrote
rather than the behaviour. The other drops it because the case looks unlikely,
which is not disproved. What disproves a path is what makes it impossible. That
is a guard nobody can pass or a caller that cannot exist, at a line.
```
]