Add test infrastructure and PHPUnit suite - #4
Merged
Merged
Conversation
The constructor accessed $this->logger inside a catch block, but Silverstripe DI assigns properties from $dependencies *after* the constructor returns. Any malformed SVG would have thrown 'property accessed before initialization' on load. Parsing is now deferred until first access to getWidth/getHeight, by which point logger is wired up. Also tightens types so static analysis at level max with 100% type-coverage passes: handle nullable filename in onBeforeWrite, and switch manipulate() parameters to mixed (parent is untyped, narrowing isn't allowed).
Narrows composer constraints to silverstripe/framework ^6 and php ^8.3 so the test matrix and dev environment can target a single supported stack. This matches the SilverStripe 6 system requirements and the conventions used by other recent WeDevelop modules (e.g. silverstripe-menustructure). Also drops friendsofphp/php-cs-fixer in favour of Rector for code-style automation, and removes silverstripe/mimevalidator from dev deps (it stays as an optional consumer dependency via "suggest").
The previous implementation was incompatible with SilverStripe 6: BuildTask now declares `protected string $title` and `protected static string $description`, the run() method takes (InputInterface, PolyOutput) and is concrete, and subclasses must implement `execute(InputInterface, PolyOutput): int`. Loading the task on SS6 produced a fatal at bootstrap. This change adds proper namespace, typed properties, and the new execute() signature. Rector's BuildTaskUpdateRector also renames the routing constant from `$segment` to `$commandName`. The DB columns and table name are now ANSI-quoted to match the rest of the codebase and avoid future schema-aware issues.
The previous Dockerfile shipped only a PHP 8.1 CLI container with no web
server or database. That was sufficient for static analysis but left
PHPUnit, Rector, and any integration testing without a real Silverstripe
runtime. The new setup mirrors silverstripe-menustructure and gives us:
- FrankenPHP-based app container (php:8.3 by default, configurable via
PHP_VERSION arg) with intl, gd, mysqli, pdo_mysql, zip, bcmath, exif,
pcov for coverage.
- MySQL 8 service with a db-init script that grants the silverstripe
user the privileges needed for SapphireTest's ss_tmpdb_* databases.
- Per-worktree port allocation (.docker/env.sh) so multiple checkouts
can run in parallel without colliding.
- The Silverstripe project skeleton (composer.json, phpstan.neon.dist,
phpunit.xml.dist, rector.php) lives under .docker/app/ and the module
itself is symlinked in via composer's path repository.
Makefile is rewritten with up/down/destroy/test/coverage/analyse/rector/
rector-dry/dev-build/sh targets. PHPStan and php-cs-fixer configs at the
repo root are removed in favour of the new locations.
Covers the two production classes:
tests/Assets/SvgTest.php
- Empty-file branches for getTag/getWidth/getHeight return zero values.
- getTag/getWidth/getHeight parse a valid SVG fixture and report the
authored 100x50 dimensions.
- manipulate() returns the wrapped DBFile unchanged.
- onBeforeWrite() strips <script>, onclick, and javascript: from a
malicious SVG fixture before the file is persisted.
- Lazy-loading config flag stays disabled.
- Malformed SVG content is logged via the injected LoggerInterface
(mocked) and does not throw.
tests/Task/MigrateCurrentSvgsTaskTest.php
- .svg files have their ClassName flipped to Svg.
- Non-svg files (.png, .pdf) are left untouched.
Three SVG fixtures (sample, malicious, invalid) and one YAML fixture for
the migrate task live under tests/fixtures/.
Two jobs:
- static-analysis: brings up the app container, then runs make analyse
(PHPStan at level max with 100% type-coverage) and make rector-dry.
- phpunit: matrixed across PHP 8.3, 8.4, and 8.5; brings up the full
stack, runs PHPUnit with junit output, and surfaces results via
dorny/test-reporter so failures are visible in PR checks.
Concurrency is scoped to the workflow + ref so superseded pushes are
cancelled.
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
1 participant
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.
Summary
Brings the module up to the same QA-gate parity as
silverstripe-menustructure: a real test runtime, automated checks, and the first PHPUnit suite. Two production bugs surfaced during test development and are fixed in dedicated commits.Production fixes (caught by tests)
$this->loggerwas accessed inside acatchblock, but Silverstripe DI only assigns$dependenciesafter the constructor returns. Any unparseable SVG would have thrown 'property accessed before initialization'. Parsing is now deferred to first access ofgetWidth/getHeight.MigrateCurrentSvgsTaskwas incompatible with SilverStripe 6 — typed property mismatch on$titleplus the wrong override target (run()is now concrete inBuildTask; subclasses implementexecute(InputInterface, PolyOutput): int). The recent Add SilverStripe 6 compatibility commit didn't catch this because there were no tests. Now namespaced, typed, and using the SS6 API.Infrastructure
.docker/— FrankenPHP + MySQL stack (mirrorssilverstripe-menustructure), per-worktree port allocation, db-init script that grantsss_tmpdb_*privileges..docker/app/— Silverstripe project skeleton: composer.json (recipe-cms + dev deps), phpstan.neon.dist (level max with 100% type-coverage), phpunit.xml.dist (PHPUnit 11), rector.php..github/workflows/ci.yml— two jobs: static-analysis (PHPStan + Rector dry-run) and phpunit (matrix on PHP 8.3 / 8.4 / 8.5, junit output, dorny/test-reporter).Makefile—up,down,destroy,test,coverage,analyse,rector,rector-dry,dev-build,sh.Composer narrowing
php^8.1→^8.3silverstripe/framework^5 || ^6→^6friendsofphp/php-cs-fixer(replaced by Rector viawernerkrauss/silverstripe-rectorin the dev project), dropssilverstripe/mimevalidatorfrom dev deps (still listed undersuggest).This is a breaking change for any consumer still on SilverStripe 5; in practice the prior
^5 || ^6claim was already inaccurate becauseMigrateCurrentSvgsTaskwas broken on SS6.Tests
12 tests, 25 assertions, 95% line / 86% method coverage for the production code:
tests/Assets/SvgTest.php(10 tests) — empty-file branches, getTag/getWidth/getHeight against a valid SVG fixture,manipulate()pass-through,onBeforeWrite()strips<script>/onclick/javascript:from a malicious fixture, lazy-loading config, and a mockedLoggerInterfaceto verify malformed content is logged without throwing.tests/Task/MigrateCurrentSvgsTaskTest.php(2 tests) — fixture with mixed.svg/.png/.pdfFile rows; verifies only.svgrows have ClassName flipped.tests/fixtures/— sample.svg, malicious.svg, invalid.svg, plus the migrate task YAML.Test plan
make up && make analyse— PHPStan greenmake rector-dry— Rector cleanmake test— 12 / 12 passingmake coverage— coverage summary printed, HTML atcoverage/html