[FEATURE] Report exclude entries which did not take effect - #108
Draft
CybotTM wants to merge 1 commit into
Draft
Conversation
An exclude entry that quietly matches nothing is the failure the whole filter exists to prevent: a published archive carrying the very directory the configuration was supposed to keep out. `Resources/Private/Build/` written with a trailing slash, with a leading `./` or with backslashes as separators reads like a valid exclude and packages the directory anyway, without a warning and without an error. Report an entry when the created archive still contains what the entry names, together with a hint about the most likely reason. Comparing against the packaged paths rather than counting pattern matches keeps two cases quiet which are not a problem: an entry for a directory the extension does not have - exclude configurations are shared between extensions and the shipped default covers a lot of them - and an entry already covered by a shorter one next to it. Entries still written with escaped slashes are reported as well, since the escaping is only accepted for compatibility since TYPO3#107. Assisted-by: claude-code:claude-opus-5 Agent-Session: https://claude.ai/code/session_01CFdZJzsCjJ7rmT1u9snkiv Agent-Host: 0493f0 Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #106 and #107, which fixed two ways an exclude entry could silently fail. This adds the reporting that would have made both of them visible at release time instead of after the fact.
The problem
An exclude entry that matches nothing looks exactly like one that works: no error, no warning, exit code 0, and an archive that carries the directory the configuration was supposed to keep out. That is how the 36 build files ended up in the archive linked in #106, and how the escaped-slash notation broke in #107 without anyone noticing until Elias read the diff.
The notations that read as valid configuration and quietly do nothing today, all measured against
mainat 7faa2d2:Resources/Private/Build?Resources/Private/BuildResources\/Private\/BuildResources/Private/Build/./Resources/Private/BuildResources\Private\BuildNone of these is a regression - the first two were broken before #106 as well, with a
preg_match(): Unknown modifierwarning that at least showed something was wrong. Since #106 they fail in complete silence.What this does
After the tree has been walked, every configured entry is compared against the paths that actually went into the archive. An entry is reported when the archive still contains what the entry names, with a hint about the most likely reason:
It is advisory: the warnings go to both
create-artefactandter:publish, the exit code stays 0 and nothing about the resulting archive changes.Why report instead of accepting every notation
Normalising the input away -
trim('/'), strip./, translate backslashes - would fix the three rows in the table above and I would rather not, for four reasons.Normalisation only covers the variants someone already thought of. A
trim()fixes the trailing slash and does nothing for a directory renamed since the config was written, for afilesentry carrying a path, or for a typo. The comparison against the packaged paths caught all of those in the smoke run without knowing any of them in advance, because it measures the effect rather than the shape of the input.Every normalisation is a silent guess, and a wrong guess is the same silent failure again. #106 and #107 are the proof: quoting was right for the documented notation and broke the undocumented one; stripping
\/fixed that and would break a directory whose name really contains a backslash. Each rule quietly widens what counts as valid input without telling anyone which reading was picked.Accepted-but-undocumented variants become the contract. Once
Resources/Private/Build/just works, the README sentence "plain directory and file names" is no longer true and every future refactoring has to preserve the accident. A warning keeps one documented notation and makes the deviation visible where it is cheap to fix: in the release log, before the archive goes to TER.It is not either/or. Normalising the trailing slash later is a one-line change and the warning would still say that it happened. Reporting first is the smaller step and it closes the whole class, not the three cases in the table.
Why it compares against the packaged paths
The straightforward implementation - remember which entries matched during the walk, report the rest - is too noisy to survive contact with a real configuration. Custom configurations are curated copies of the shipped default plus a few extras, and most of their entries legitimately match nothing in any single extension.
Measured on
xima-media/xima-typo3-frontend-edit2.5.0 with its ownpackaging_exclude.php(35 entries): reporting every unmatched entry produces 24 warnings, including two false ones for entries that are perfectly fine. Comparing against what was packaged produces 2, both of them the escaped-slash notice this PR wants that extension to see:The same comparison also keeps a redundant entry quiet:
Resources/Privatenext toResources/Private/Buildleaves nothing of the second one in the archive, so there is nothing to report.Escaped slashes
Entries written as
Resources\/Private\/Buildstill work and are now pointed out once per release, because the escaping is only accepted for compatibility. The three extensions using it - xima-typo3-frontend-edit, t3x-nr-saml-auth, hide_sys_template - carry the same two lines, so this looks like one copied template rather than three independent decisions. Without the notice the double notation in the README stays forever; with it, the escaping can be dropped in a future major version.The README now documents one notation and mentions the escaped one as accepted, not as an alternative.
Tests
Nine new tests,
tests/Unit/Service/VersionServiceTest.phpand both packaging commands:filesentry containing a path is reportedcreate-artefactand ofter:publishEach of them was shown to fail: seven mutations of the new code (warnings suppressed, evidence check bypassed, packaged paths not collected, each hint removed, the notation notice removed, the command output removed) each redden exactly the tests that cover them, and every one of the nine appears in at least one of those runs.
composer tests:unitis green (163 tests, 449 assertions) andcomposer csreports no findings.Assisted by claude-code:claude-opus-5 — Session