Skip to content

Add CI and the shared Release workflow, and revive the test suite - #24

Merged
harikt merged 2 commits into
2.xfrom
claude/add-ci-and-release-workflow
Sep 9, 2026
Merged

harikt merged 2 commits into
2.xfrom
claude/add-ci-and-release-workflow

Conversation

@harikt

@harikt harikt commented Sep 9, 2026

Copy link
Copy Markdown
Member

This package had neither continuous integration nor a release workflow. Releasing it meant tagging by hand — which is what the shared workflow exists to prevent: Packagist derives versions from tags, so a tag is a published version the moment it exists, and checks that run after a tag push cannot un-publish anything.

Adding the release workflow alone would not have worked, so this does the whole job.

Why the test suite had to be revived

The shared workflow's pre-tag gate runs ./vendor/bin/phpunit. This package declared no test runner at all — no require-dev — so composer install produced no phpunit binary. Adding one exposed the real problem: the suite was written against the PHPUnit 4/5 API that PHPUnit 6 removed, so it could not run on any supported PHP version.

I revived it rather than substituting a weaker gate, because a release gate that runs nothing is not a gate.

  • Tests extend PHPUnit\Framework\TestCase, and setUp() now declares : void as PHPUnit 8+ requires — without the return type the declaration is outright incompatible with the parent and fatals.
  • phpunit/phpunit: ^11.0 added to require-dev; phpunit.xml.dist updated to the current schema.

A real bug this surfaced

testAddPrefix could never have passed. The nds() helper normalizes path separators with str_replace(), but it is handed the nested array from getPrefixes(), and str_replace() on a nested array stringifies the inner arrays to 'Array':

-    'Foo\Bar\' => Array &1 [
-        0 => '/path/to/foo-bar/src/',
-        1 => '/path/to/foo-bar/tests/',
-    ],
+    'Foo\Bar\' => 'Array',

I checked the library itself before touching anything — getPrefixes() returns exactly the right structure in the right prepend order. The bug is in the test helper, which now recurses. This went unnoticed for nine years precisely because there was no CI.

Change log

CHANGES.md held a single unheaded line, so the release workflow would have failed at release-notes extraction (no version heading to release from). It now has headings.

The heading over the existing line is not a guess: HEAD is the 2.0.4 tag, and that line was written by that tag's own release commit (update for release). A 2.0.5 section describes this change.

Verified on PHP 8.4

  • composer validate --no-check-lock --no-check-publish — passes.
  • ./vendor/bin/phpunit — OK (11 tests, 17 assertions), exit 0.
  • Change log extraction resolves to 2.0.5 with no unreleased marker.

No library code changed and require.php is untouched, so nothing changes for consumers. CI covers PHP 8.2–8.4; require.php still says >=5.3.0, which is now broader than what is tested — worth a separate look.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Lgup8xLZHLQmavt7JoEQ4C


Generated by Claude Code

This package had neither continuous integration nor a release workflow, so
releasing it meant tagging by hand -- which is what the shared workflow
exists to prevent: Packagist derives versions from tags, so a tag is a
published version the moment it exists.

Adding a release workflow alone would not have worked. Its pre-tag gate
runs ./vendor/bin/phpunit, and this package declared no test runner at all:
composer install produced no phpunit binary. Adding one then exposed why --
the suite was written against the PHPUnit 4/5 API that PHPUnit 6 removed,
so it could not run on any supported PHP version.

Revived rather than skipped, because a release gate that runs nothing is
not a gate:

- Tests now extend PHPUnit\Framework\TestCase, use expectException(), and
  declare setUp(): void as PHPUnit 8+ requires. Without the return type the
  declaration is outright incompatible with the parent and fatals.
- ADD: phpunit/phpunit ^11.0 to require-dev.
- phpunit.xml.dist updated to the current schema.
- The change log gains version headings. It held a single unheaded line, so
  the release workflow would have failed at release-notes extraction. The
  heading names the release that line describes -- HEAD is that tag, and the
  line was written by that tag's own release commit.
- ADD: .gitignore covering vendor, composer.lock and the phpunit cache.

Verified on PHP 8.4: composer validate passes and the suite is green
(11 tests, 17 assertions).

No library code changed, and require.php is untouched, so nothing changes
for consumers.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lgup8xLZHLQmavt7JoEQ4C
@coderabbitai

coderabbitai Bot commented Sep 9, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 2321cd47-ab59-41a9-ace7-79ae3b45c195

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

The suite ran only on PHP 8.2-8.4, while require.php claims support far
older than that. Testing a narrower range than the package advertises means
the advertised range is unverified.

yoast/phpunit-polyfills bridges the gap: one set of tests runs on PHPUnit
5.7 through 11, so a single suite covers PHP 5.6 to 8.5.

- Tests extend Yoast\PHPUnitPolyfills\TestCases\TestCase and define set_up()
  instead of setUp(): void. The void return type is a parse error before PHP
  7.1, so the modern signature cannot even be parsed on older versions --
  the polyfill TestCase bridges set_up() to whichever signature the running
  PHPUnit expects.
- require-dev widens to phpunit ^5.7.21 || ^6.4.4 || ^7.5 || ^8.5 || ^9.6 ||
  ^11.0 and polyfills ^2.0.5 || ^3.1.2 || ^4.0.0, so each PHP version
  resolves a combination that supports it. CI runs composer update rather
  than install for exactly this reason; no lock file is committed.
- phpunit.xml.dist drops the version-pinned schema and the attributes added
  after 5.7, so one config file is valid across the whole range. Verified
  locally on both PHPUnit 11.5 and 9.6, same results.
- CI selects Composer 2.2 LTS below PHP 7.2, since Composer 2.3+ requires
  PHP 7.2.5.

Note that 5.6 is the floor CI can reach, not the floor require.php claims:
setup-php does not offer PHP below 5.6, and the polyfills need 5.4+. PHP
5.3-5.5 therefore stay untested.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lgup8xLZHLQmavt7JoEQ4C
@harikt
harikt merged commit a1d576c into 2.x Sep 9, 2026
25 of 26 checks passed
@harikt
harikt deleted the claude/add-ci-and-release-workflow branch September 9, 2026 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant