Skip to content

Modernize codebase for PHP 8.5+ and fix five known bugs#20

Open
tecpromotion wants to merge 3 commits into
joomlagerman:masterfrom
tecpromotion:refactor-php-85
Open

Modernize codebase for PHP 8.5+ and fix five known bugs#20
tecpromotion wants to merge 3 commits into
joomlagerman:masterfrom
tecpromotion:refactor-php-85

Conversation

@tecpromotion
Copy link
Copy Markdown
Member

Summary

Modernizes the bot to run on PHP 8.5+ idiomatically, bumps the Joomla framework dependencies from ^1/~1 to ^4, fixes five long-standing bugs, and introduces a small tooling stack (PHPStan max, PHP-CS-Fixer, PHPUnit). The architecture stays the same: CLI entry point + helpers, configured via includes/constants.php.

Verified end-to-end on a production-equivalent server running PHP 8.5.5 — both the once-a-day guard path and the full GitHub-API-fetch path execute cleanly.

⚠ Breaking Changes

  1. PHP 8.5+ required (composer.json platform constraint).
  2. vendor/ is no longer committed. Installations must run composer install --no-dev after pulling.
  3. Three constants were renamed (typo fix). Existing includes/constants.php files must be updated:
    • NOTIFYER_SLACK_ENABEDNOTIFYER_SLACK_ENABLED
    • NOTIFYER_MATTERMOST_ENABEDNOTIFYER_MATTERMOST_ENABLED
    • NOTIFYER_TELEGRAM_ENABEDNOTIFYER_TELEGRAM_ENABLED
  4. includes/github-base.php was removed — replaced by joomlagerman\Helper\Bootstrap. Anyone embedding the bot from custom CLI scripts must update accordingly.

Bug fixes

  • Latest-tag selection inverted in getLatestGithubReleaseByBranch(): the version_compare check picked the oldest matching tag instead of the newest. Now extracted into a pure, unit-tested static helper GithubApiHelper::pickLatestTag().
  • Default release seed typo: lastrelease<branch>.data was initialized with <branch>.0v0 instead of <branch>.0.0.
  • Branch-label parser broken on multi-digit minors: substr($targetBranch, 0, 3) mangled 4.10-dev into Joomla! 4.1. Now handled by joomlagerman\Enum\JoomlaBranch::labelFor(), with explicit tests for historical (3.10-dev, 4.10-dev) and modern (5.4-dev, 6.1-dev, 6.4-dev) branches.
  • Missing Registry import in NotifyerHelper: a fallback path referenced new Registry without use Joomla\Registry\Registry;.
  • Auth token leaked through global constant: getSourcePullDiff() read GITHUB_AUTHTOKEN directly instead of going through the options Registry. The token is now wired in via Bootstrap.

PHP 8.5+ modernization

  • declare(strict_types=1) in every PHP file.
  • final classes, readonly properties, constructor property promotion.
  • match expressions for the notifier channel routing.
  • Enums:
    • joomlagerman\Enum\NotificationChannel (Slack / Mattermost / Telegram) — each case carries its own isEnabled(), endpoint(), payload(). Replaces three near-duplicate if-blocks.
    • joomlagerman\Enum\JoomlaBranch — pure branch-label parsing.
  • \DateTimeImmutable instead of \DateTime.
  • New joomlagerman\Helper\Bootstrap factory replaces the require-with-side-effects pattern in includes/github-base.php. The CLI script stays procedural but is now type-safe.
  • All Registry reads go through typed accessors (e.g. optString()); no more (string) $registry->get(...) casts on mixed values.
  • Fixed an additional latent bug surfaced during refactoring: HttpFactory::getHttp() was previously called statically, which fatals on PHP 8.4+. Now (new HttpFactory())->getHttp().

Dependency bumps

Package Before After
php 7.3 ^8.5
joomla/github ^1.7 ^4.0
joomla/http ~1.3 ^4.0
joomla/registry (transitive) ^4.0
joomla/uri (transitive) ^4.0

Includes a small adapter for the PSR-7 response shape that joomla/http 4.x exposes ($response->getBody() instead of the old $response->body).

Tooling

  • composer test — PHPUnit 11, 14 tests covering branch-label parsing, latest-tag selection (incl. the bug-A fix), notifier channel routing.
  • composer stan — PHPStan at level: max, 0 errors.
  • composer cs-check / composer cs-fix — PHP-CS-Fixer with PSR-12 + tab indent (matching .editorconfig).
  • tools/phpstan-stubs/constants.stub.php — type stubs so PHPStan knows the runtime constants.

Migration guide for existing deployments

cd /repo/path/jgerman-bot
git pull
composer install --no-dev

# Update the three renamed constants in includes/constants.php:
#   NOTIFYER_SLACK_ENABED      → NOTIFYER_SLACK_ENABLED
#   NOTIFYER_MATTERMOST_ENABED → NOTIFYER_MATTERMOST_ENABLED
#   NOTIFYER_TELEGRAM_ENABED   → NOTIFYER_TELEGRAM_ENABLED

# Verify PHP 8.5+:
php -v

# Smoke test (won't create issues — exits via the once-a-day guard):
echo "$(date +%Y-%m-%d)" > data/lastrun.data
php cli/jgerman-github-bot.php
tail -5 logs/$(date +%Y%m)_jgerman.log

Test plan

  • composer test — 14/14 green
  • composer stan[OK] No errors
  • composer cs-check — clean
  • Server smoke test on PHP 8.5.5 — bootstrap, autoload, constants loading, logging all verified
  • Server end-to-end run with real merged PRs — issue created in the translation repo, Slack notification fired

Notes

  • joomla/github 4.0.1 and joomla/http 4.0.2 emit a handful of E_DEPRECATED warnings on PHP 8.5 (implicitly nullable parameters, dynamic property creation). These are upstream issues in those packages, not introduced by this PR. They will not break execution on PHP 8.5 but will need fixing before PHP 9.
  • setOption() on the helpers is unused externally. Kept in place for now; can be deprecated/removed in a follow-up if there are no embedders.

@tecpromotion tecpromotion requested a review from zero-24 May 1, 2026 13:52
Replace the four per-event notifier posts (Start / count / date / End)
  plus the per-issue messages with one consolidated message sent at the
  end of the run. Slack and Mattermost get an attachment with a colored
  bar — green on success, yellow when the daily guard skipped the run.
  Telegram keeps a plain-text variant with HTML links.

  - Add RunStatus + RunSummary under joomlagerman\Notification\
  - NotificationChannel::summaryPayload() builds the channel payload
  - NotifyerHelper exposes a single sendRunSummary() method
  - Drop the unused NOTIFYER_GITHUB_ISSUE_MESSAGE_TEMPLATE constant
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant