From f77f862e51424ccb131ab66b7f47511a14654aa9 Mon Sep 17 00:00:00 2001 From: Snider Date: Sat, 8 Aug 2026 12:35:06 +0100 Subject: [PATCH 1/2] fix(ci): make the gates capable of both passing and failing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit main was red, and had two opposite problems sitting next to each other. PSALM. psalm.xml suppressed NoEnvOutsideConfig for src/, and Psalm reported UnusedIssueHandlerSuppression because the issue is never raised — src/'s env() calls live in files literally named config.php, which the Laravel plugin already treats as config. So the suppression protected nothing and its own unusedness was the only error Psalm found. Removing it takes Psalm from exit 2 to "No errors found!" with the 1486 info-level issues unchanged. That was the whole Psalm half of the red. TESTS. tests.yml ran a bare `phpunit`, which includes the Module suite — src/**/*Test.php, 448 known failures, treated everywhere else as visible debt rather than a gate. So the workflow could never pass. It now runs --testsuite=Feature,Unit, which is exit 0 with 269 tests, and the Module suite gets its own job with continue-on-error so the number stays on screen and can be driven down. That mirrors .gitlab-ci.yml, which had already made exactly this split with the reasoning written out. fail-fast is off on the matrix. With it on, the first failing leg cancelled the others, so a break specific to one PHP version was indistinguishable from a break on all three. AND THE OTHER DIRECTION. ci.yml ran pint, phpstan and pest each with `|| true`. It was the one check in the repository that could not fail, next to a Tests workflow that could not pass. Removed, all three. Doing that surfaced something the `|| true` had been hiding: the Pest step passed --no-interaction, which is a PHPUnit flag. Pest answers `Unknown option "--no-interaction"` and exits 2. That step has never run the suite once — not "ran and was ignored", never ran. A check that swallows its exit code cannot tell you it is not the check you think it is. Verified locally, every gate: pint --test exit 0 phpstan analyse exit 0 psalm --show-info=false exit 0 (was 2) pest --testsuite=Feature,Unit exit 0 (was 2, on the bad flag) phpunit --testsuite=Feature,Unit exit 0 (was 1, via the debt suite) phpunit --testsuite=Module exit 1 — expected, now non-gating The 448 is not 448 bugs, and the debt job says so: ~114 are class-not-found for sibling packages' classes and cannot pass here at all, ~30 are one real defect (StorageUrlResolver handed a CdnUrlBuilder where it wants a BunnyStorageService), the rest ordinary assertion failures. Named there so the next person starts from the breakdown rather than the total. Left alone deliberately: ci.yml now overlaps static-analysis.yml on pint and phpstan, and tests.yml on the suite. Duplicated gates are harmless; lying ones are not. Consolidating them is a tidy-up, not this change. Co-Authored-By: Virgil --- .github/workflows/ci.yml | 16 +++++++++--- .github/workflows/tests.yml | 50 +++++++++++++++++++++++++++++++++++-- psalm.xml | 8 ------ 3 files changed, 61 insertions(+), 13 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4aa8e78..33f80d3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -17,9 +17,19 @@ jobs: coverage: none - name: Composer install run: composer install --prefer-dist --no-interaction --no-progress + # Without `|| true`. Every one of these swallowed its exit code, so this + # job reported success whatever happened — the only check in the repository + # that could not fail, sitting next to a Tests workflow that could not + # pass. Neither told anyone anything. + # + # Pest is scoped to the gate suites for the same reason tests.yml is: the + # Module suite is tracked debt, reported by its own non-gating job. - name: Pint - run: vendor/bin/pint --test || true + run: vendor/bin/pint --test - name: PHPStan - run: vendor/bin/phpstan analyse --no-progress || true + run: vendor/bin/phpstan analyse --no-progress + # No --no-interaction: that is a PHPUnit flag, and Pest rejects it with + # `Unknown option` and exit 2. The `|| true` had been hiding a broken + # invocation, not merely failing tests — this step has never run the suite. - name: Pest - run: vendor/bin/pest --no-coverage --no-interaction || true + run: vendor/bin/pest --testsuite=Feature,Unit --no-coverage diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 37d5041..76f73d6 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -11,7 +11,10 @@ jobs: runs-on: ubuntu-latest strategy: - fail-fast: true + # One leg failing should not hide the others. With fail-fast the first + # failure cancels the rest, so a PHP-version-specific break looks + # identical to a break on every version. + fail-fast: false matrix: php: [8.2, 8.3, 8.4] laravel: [12.*] @@ -36,8 +39,14 @@ jobs: composer require "laravel/framework:${LARAVEL_VERSION}" --no-interaction --no-update composer update --prefer-dist --no-interaction --no-progress + # The gate suites only. A bare `phpunit` also runs the Module suite — + # src/**/*Test.php — which carries known failures and is treated as + # visible debt rather than a gate (see .gitlab-ci.yml's module-suite-debt + # job, and the module-debt job below). Including it here meant this + # workflow could never be green, so it stopped meaning anything: the same + # rot as a check that always passes, arrived at from the other side. - name: Execute tests with coverage - run: vendor/bin/phpunit --coverage-clover=coverage.xml + run: vendor/bin/phpunit --testsuite=Feature,Unit --coverage-clover=coverage.xml - name: Upload coverage to Codecov if: matrix.php == '8.3' @@ -46,3 +55,40 @@ jobs: files: ./coverage.xml fail_ci_if_error: false verbose: true + + # The Module suite — src/**/*Test.php — reported, never gating. + # + # It carries known failures, and the number is worth seeing rather than + # hiding: 448 at the time of writing, and they are not 448 bugs. Roughly 114 + # are class-not-found for classes owned by sibling packages (Core\Tenant\Models, + # Core\Agentic\Services) — tests that cannot pass in this repository at all; + # about 30 are one real defect, StorageUrlResolver being handed a CdnUrlBuilder + # where it wants a BunnyStorageService; the rest are ordinary assertion and + # status-code failures. + # + # continue-on-error rather than `|| true`, and the difference matters: this + # way the step's own result is visible in the run, so the count can be driven + # down. A step that swallows its exit code teaches nobody anything. + # + # When it reaches zero, fold --testsuite=Module into the job above and delete + # this one. Mirrors .gitlab-ci.yml's module-suite-debt job. + module-debt: + runs-on: ubuntu-latest + name: Module suite (reported, not gating) + continue-on-error: true + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Setup PHP + uses: shivammathur/setup-php@v2 + with: + php-version: '8.4' + extensions: dom, curl, libxml, mbstring, zip + coverage: none + + - name: Install dependencies + run: composer install --prefer-dist --no-interaction --no-progress + + - name: Execute the Module suite + run: vendor/bin/phpunit --testsuite=Module diff --git a/psalm.xml b/psalm.xml index 01ea359..80cb4e3 100644 --- a/psalm.xml +++ b/psalm.xml @@ -19,14 +19,6 @@ - - - - - - From 5deb47c557e5ddcef8e1fd7a5caaea0fa7759622 Mon Sep 17 00:00:00 2001 From: Snider Date: Sat, 8 Aug 2026 12:40:11 +0100 Subject: [PATCH 2/2] fix(psalm): the 13 errors CI was actually failing on, and a wrong diagnosis undone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I diagnosed Psalm from local output and got it wrong twice. Correcting both here rather than quietly. WHAT I DID WRONG. Locally Psalm reported one error — UnusedIssueHandlerSuppression for the NoEnvOutsideConfig suppression in psalm.xml — so I removed the suppression and saw exit 0. In CI that removal produced 183 NoEnvOutsideConfig errors in src/Core/config.php. The suppression was doing real work; it only looks unused locally, because the Laravel plugin resolves the config directory differently there. Reverted. WHAT CI WAS ACTUALLY FAILING ON, which I should have read first: 13 PublicModelAccessor errors, none of which appear locally at all. 6 src/Core/Seo/SeoMetadata.php 2 src/Core/Seo/Models/SeoScoreHistory.php 2 src/Core/Media/Image/ImageOptimization.php 2 src/Core/Config/Models/ConfigValue.php 1 src/Core/Cdn/Models/StorageOffload.php Eloquent accessors and mutators declared public. Laravel reaches them through getAttribute()/__get, so protected is what they should be, and public invites somebody to call getValueAttribute() directly and depend on it. Made protected, all 13. Checked first that nothing calls any of them directly — not in this package, and not in host.uk.com, which is the only consumer — so the visibility change costs nobody anything. All five classes extend Model, so the framework path is the only path. The lesson is the same one I flagged in another repo this morning and then walked into: local and CI disagreed, and I trusted the copy in front of me instead of opening the job log. Local Psalm reports 1 error / 1486 issues, CI reports 13 / 1155. On a tool with a Laravel plugin that bootstraps differently per environment, the CI log is the only source that speaks for CI. vendor/bin/pint --test pass vendor/bin/phpstan analyse no errors ./vendor/bin/pest --testsuite=Feature,Unit 268 passed, 0 failed — unchanged ./vendor/bin/pest --testsuite=Module 448 failed / 308 passed — unchanged Psalm itself cannot be verified from here, for the reason above. CI is the check. Co-Authored-By: Virgil --- psalm.xml | 8 ++++++++ src/Core/Cdn/Models/StorageOffload.php | 2 +- src/Core/Config/Models/ConfigValue.php | 4 ++-- src/Core/Media/Image/ImageOptimization.php | 4 ++-- src/Core/Seo/Models/SeoScoreHistory.php | 4 ++-- src/Core/Seo/SeoMetadata.php | 12 ++++++------ 6 files changed, 21 insertions(+), 13 deletions(-) diff --git a/psalm.xml b/psalm.xml index 80cb4e3..01ea359 100644 --- a/psalm.xml +++ b/psalm.xml @@ -19,6 +19,14 @@ + + + + + + diff --git a/src/Core/Cdn/Models/StorageOffload.php b/src/Core/Cdn/Models/StorageOffload.php index a0b39f3..febf12e 100644 --- a/src/Core/Cdn/Models/StorageOffload.php +++ b/src/Core/Cdn/Models/StorageOffload.php @@ -150,7 +150,7 @@ public function scopeForDisk($query, string $disk) /** * Get human-readable file size. */ - public function getFileSizeHumanAttribute(): string + protected function getFileSizeHumanAttribute(): string { $bytes = $this->file_size ?? 0; $units = ['B', 'KB', 'MB', 'GB', 'TB']; diff --git a/src/Core/Config/Models/ConfigValue.php b/src/Core/Config/Models/ConfigValue.php index 26e0f5c..f0617c2 100644 --- a/src/Core/Config/Models/ConfigValue.php +++ b/src/Core/Config/Models/ConfigValue.php @@ -64,7 +64,7 @@ class ConfigValue extends Model /** * Get the value attribute with automatic decryption for sensitive keys. */ - public function getValueAttribute(mixed $value): mixed + protected function getValueAttribute(mixed $value): mixed { if ($value === null) { return null; @@ -91,7 +91,7 @@ public function getValueAttribute(mixed $value): mixed /** * Set the value attribute with automatic encryption for sensitive keys. */ - public function setValueAttribute(mixed $value): void + protected function setValueAttribute(mixed $value): void { // Check if the key is sensitive (need to load it if not already) $key = $this->relationLoaded('key') diff --git a/src/Core/Media/Image/ImageOptimization.php b/src/Core/Media/Image/ImageOptimization.php index 0ad80bf..4005207 100644 --- a/src/Core/Media/Image/ImageOptimization.php +++ b/src/Core/Media/Image/ImageOptimization.php @@ -100,7 +100,7 @@ public function scopeForWorkspace($query, ?Model $workspace) * * Example: "45% saved (120KB → 66KB)" */ - public function getSavingsHumanAttribute(): string + protected function getSavingsHumanAttribute(): string { // Format with appropriate unit $original = $this->formatBytes($this->original_size); @@ -117,7 +117,7 @@ public function getSavingsHumanAttribute(): string /** * Get human-readable size saved. */ - public function getSizeSavedHumanAttribute(): string + protected function getSizeSavedHumanAttribute(): string { $saved = $this->original_size - $this->optimized_size; diff --git a/src/Core/Seo/Models/SeoScoreHistory.php b/src/Core/Seo/Models/SeoScoreHistory.php index 080ba88..3a9fe3e 100644 --- a/src/Core/Seo/Models/SeoScoreHistory.php +++ b/src/Core/Seo/Models/SeoScoreHistory.php @@ -108,7 +108,7 @@ public function seoMetadata(): BelongsTo /** * Get the score color for UI display. */ - public function getScoreColorAttribute(): string + protected function getScoreColorAttribute(): string { return match (true) { $this->score >= 80 => 'green', @@ -120,7 +120,7 @@ public function getScoreColorAttribute(): string /** * Get the issue count. */ - public function getIssueCountAttribute(): int + protected function getIssueCountAttribute(): int { return count($this->issues ?? []); } diff --git a/src/Core/Seo/SeoMetadata.php b/src/Core/Seo/SeoMetadata.php index 3ef8eb0..6e3f728 100644 --- a/src/Core/Seo/SeoMetadata.php +++ b/src/Core/Seo/SeoMetadata.php @@ -97,7 +97,7 @@ class SeoMetadata extends Model * * @return array|null */ - public function getSchemaMarkupAttribute(): ?array + protected function getSchemaMarkupAttribute(): ?array { if ($this->schemaMarkupLoaded) { return $this->parsedSchemaMarkup; @@ -132,7 +132,7 @@ public function getSchemaMarkupAttribute(): ?array * * @param array|string|null $value */ - public function setSchemaMarkupAttribute(array|string|null $value): void + protected function setSchemaMarkupAttribute(array|string|null $value): void { // Reset the lazy loading cache $this->parsedSchemaMarkup = null; @@ -183,7 +183,7 @@ public function seoable(): MorphTo * * Uses JSON_HEX_TAG to prevent XSS via in content. */ - public function getJsonLdAttribute(): string + protected function getJsonLdAttribute(): string { if (empty($this->schema_markup)) { return ''; @@ -197,7 +197,7 @@ public function getJsonLdAttribute(): string /** * Generate all meta tags as HTML. */ - public function getMetaTagsAttribute(): string + protected function getMetaTagsAttribute(): string { $tags = []; @@ -241,7 +241,7 @@ public function getMetaTagsAttribute(): string /** * Get SEO score colour for UI display. */ - public function getScoreColorAttribute(): string + protected function getScoreColorAttribute(): string { if ($this->seo_score === null) { return 'zinc'; @@ -265,7 +265,7 @@ public function hasIssues(): bool /** * Get the count of issues. */ - public function getIssueCountAttribute(): int + protected function getIssueCountAttribute(): int { return count($this->seo_issues ?? []); }