chore: downgrade vendored deps and core source to PHP 5.6 syntax via Rector#5669
Open
e107help[bot] wants to merge 31 commits into
Open
chore: downgrade vendored deps and core source to PHP 5.6 syntax via Rector#5669e107help[bot] wants to merge 31 commits into
e107help[bot] wants to merge 31 commits into
Conversation
Each PHP version in the CI matrix resolves test dependencies fresh. A pinned composer.lock made the older interpreters fight transitive constraints that newer ones satisfied. Anchor the gitignore rule to the e107_tests root so sub-tool composer.lock files (e.g. the Rector downgrade tooling) can still pin their own versions.
Sets up e107_tests/tools/rector/ as the home for the downgrade pipeline that converts modern e107 source into PHP 5.6-compatible output for the v2 release artifact. The toolchain depends on rector/rector ^2.0, whose nested vendor already bundles the 7.2 -> 8.x downgrade rule sets (DowngradeSetList). The 7.0 and 7.1 rule sets were dropped from upstream rector/rector-downgrade-php after 0.15.1 and never made it into any 2.x-compatible package, so they are pin-and-vendored under custom-rules/ in the E107\Rector\ namespace, and a handful of upstream rules with hard PHP-extension or service dependencies are skipped in the config. Prototype scope is just e107_handlers/session_handler.php so the output diff stays reviewable while the pipeline is being shaken out. Will broaden to the full source tree in a follow-up. A small set of vendored rules are deliberately disabled because they reference internals (NodesToAddCollector, MethodCallTypeAnalyzer, NamespacedNameDecorator, StmtsAwareInterface variants, JsonConstant enum cases) that 2.x no longer exposes the same way -- those have follow-up port tasks.
Drop the single-file prototype path in favour of scanning the entire e107 source tree, and add a skip list for everything that should never be processed: third-party vendored deps (handled in their own pipeline), our own Rector tooling, generated test artefacts, static assets, and workspace metadata. Dry-run against full scope reports 239 files would change across e107_plugins/, e107_handlers/, e107_admin/, e107_core/, the top-level entry-point scripts, and a handful of theme files. Rules firing in the wild: DowngradeSubstrFalsyRector (81), DowngradeParameterType WideningRector (31), DowngradeNullCoalesceRector (20), and a long tail of one- and two-hit rules across the 7.x and 8.x sets. One file -- e107_handlers/theme_handler.php -- triggers an internal type error in Rector's DeadCode ConditionResolver. Tracked separately so the broadening doesn't block on it.
theme_handler.php::renderThemeInfo() compared theme compatibility declarations against the float 1.9, which version_compare() documents as a string-taking function. PHP coerces silently at run time so the comparison still worked, but the float literal trips Rector's PhpVersionConditionNodeVisitor on PHP 8.x analysis runs -- its ConditionResolver declares ?int|string for argument values and TypeErrors on the float. Source fix is the right call anyway: version_compare() with a non- string first arg has been a known antipattern since the function was introduced, and a future PHP release could elevate the silent coercion to an E_WARNING.
Rector 2.x's StaticTypeMapper occasionally returns null for nullable parameter types whose declared type resolves to MixedType-plus-null (seen in firebase/php-jwt's CachedKeySet::__construct ?int param). The vendored rule was written against an older API that returned a non-nullable Type and passed the result straight into phpDocTypeChanger->changeParamType(), which TypeErrors on null. Skip docblock annotation when the mapper can't classify the type -- refactorParamType() still strips the native type on the next line, so the downgrade goes through; we just lose the inferred @param annotation in that one edge case.
The downgrade pipeline produces PHP 5.6-compatible code but no live host supports PHP 5.6, so we need a containerised verification path. php56-lint runs php -l against an arbitrary file list (or --all to sweep the worktree) inside upstream's php:5.6-cli image, with vendor/ and the Rector cache excluded by default. php56-shell is the companion for ad-hoc poking -- it drops into bash inside the same image with the repo mounted read-write at /repo. Both scripts honour PHP56_LINT_IMAGE / PHP56_SHELL_IMAGE for image overrides. php56-shell exposes an install-composer helper that fetches the Composer 2.2 LTS line (last release to support PHP 5.6) on demand without baking it into a custom image.
…uivalents The downgrade pipeline at e107_tests/tools/rector/ converts most modern PHP source automatically, but three constructs slipped past because no Rector rule in our pipeline rewrites them: 1. e107_admin/theme.php:1400 -- chained `e107::getTheme()::getThemeInfo(...)` uses PHP 7.0+ uniform variable syntax. Split into a temp var so the static dispatch happens on a plain `$obj::method()` form, which has been valid since 5.3. 2. e107_handlers/event_class.php:233 -- short list `[$a, $b] = ...` destructuring is PHP 7.1+. Use the canonical `list($a, $b) = ...` form, valid in every supported PHP version. 3. e107_tests/tests/unit/e107Test.php:1343 -- `$this->e107::coreLan(...)` on an instance property also requires PHP 7.0+ uniform variable syntax. `coreLan` is declared `public static` on the e107 class, so the dispatch is semantically the same as `e107::coreLan(...)`. Use that direct form. Each fix preserves runtime behaviour on PHP 8.x while unblocking the Rector + php56-lint pipeline.
…pen rule Three connected tweaks to the downgrade pipeline: 1. Narrow rector.php's skip list. e107_web (e_jslib, e_js, e_ajax, plupload, ...) and e107_images (secimg, thumb) host shipping production PHP that the prototype's coarser skip was excluding; they need the same downgrade treatment as e107_handlers. Add .github to the skip list while we're here; the release-build helpers never ship in the artefact. 2. Mirror those exclusions in bin/php56-lint's --all mode so the verification target matches the downgrade target exactly. Add .github, .claude, and the whole e107_tests/tools subtree so the lint scope only ever covers shipping code. 3. Skip Rector\DowngradePhp74\Rector\FuncCall\DowngradeProcOpenArrayCommandArgRector. The rule wraps proc_open's first arg in `is_array(...) ? implode(...) : $command` each pass, never recognising its own ternary as already-handled, so successive runs balloon the expression exponentially (each pass wraps the previous output). With it skipped the pipeline becomes idempotent. Existing array-form call sites must source-fix to implode themselves; the next commit handles the one such site.
proc_open's accept-array first arg is a PHP 7.4 feature; PHP 5.6 only understands the string form. The downgrade pipeline used to rewrite this automatically via DowngradeProcOpenArrayCommandArgRector, but that rule has been skipped (see prior commit -- non-idempotent), so the call site has to do its own implode now. This is the only array-form proc_open() call in the tree. SFTPDeployer and GitPreparer both take their command as a string from callers, and scriptsTest/mysqlClassConfigFormatTest build strings directly.
Bulk-applies the downgrade pipeline at e107_tests/tools/rector/ to every shipping PHP file under e107_admin, e107_core, e107_handlers, e107_images, e107_plugins, e107_themes, e107_web, the top-level entry-point scripts, and the unit-test suite. 239 files touched, zero processing errors, idempotent (a second pass produces no further diff), and the full result lints clean under php:5.6-cli (see bin/php56-lint --all -- 1092 / 1092 passing). Most common rewrites: DowngradeSubstrFalsyRector 81 hits DowngradeParameterTypeWideningRector 31 hits DowngradeNullCoalesceRector 20 hits DowngradeVoidTypeDeclarationRector 6 hits DowngradeScalarTypeDeclarationRector 5 hits DowngradeUnionTypeDeclarationRector 3 hits DowngradeTrailingCommasInFunctionCallsRector 3 hits DowngradeCatchThrowableRector 3 hits plus a long tail of 1- and 2-hit rules across the 7.x and 8.x sets. No semantic changes intended; every rewrite is a known-equivalent form chosen by upstream rector/rector-downgrade-php (bundled inside rector/rector 2.4.4) or by the E107\Rector\DowngradePhp7x rules vendored under e107_tests/tools/rector/custom-rules/. Hand-fixed source patterns that no rule covers (uniform-variable syntax, short list destructuring, version_compare with float literal, the proc_open array form) landed in earlier commits on this branch.
Adds two GitHub Actions workflows that keep the downgrade pipeline honest without anyone having to remember to run it locally: * rector-idempotency.yml installs the e107_tests/tools/rector/ composer environment on php:8.2 (matching the platform pin), runs `vendor/bin/rector process` against the shipping source tree, and then `git diff --exit-code` from the repo root. If Rector wants to rewrite anything, the job fails with a message pointing at the command to run locally. Mirrors the Debian backports prelude from test-unit.yml so older container images with stale apt indexes still work. * php56-lint.yml shells out to e107_tests/tools/rector/bin/php56-lint --all on ubuntu-latest. The script does the docker pull of php:5.6-cli, enumerates every shipping .php file, runs `php -l` in one container invocation, and exits nonzero on any failure. Also annotates e107_tests/composer.json with a `_comment` array (Composer ignores unknown root keys) documenting why the PHP 5.6 unit-test workflow is deferred: the dependency set resolves under Composer 2.2 LTS with Codeception 4.x, but the existing Cest classes target Codeception 5.x APIs and would need either a parallel test source tree or a wholesale port. Scope decision for a future PR.
Mirrors the legacy-PHP support pattern that landed on release/v2.3.x (see git blame on the v2.3.x .github/workflows/test-unit.yml) so the downgraded shipping source actually gets exercised under the runtimes it claims to support, not just lint-checked: * Matrix: add `php:5.6` and `php:7.0` to the interpreter list. Skip their pairings with `mysql:8.0` because the 5.6 / 7.0 PDO drivers reject utf8mb4 with "Server sent charset unknown to the client" and the service-container syntax has no way to pass a charset override. * actions/checkout@v3 and actions/upload-artifact@v4 run Node 20, which requires glibc 2.27+. The php:5.6 / php:7.0 images are Debian 9 (glibc 2.24), so the actions can't bootstrap there. Replace checkout with an inline `git clone` for those two cells (git is installed in the OS-deps step regardless) and skip the artifact upload step on the same condition. Test output still shows up in the run log; only the downloadable archive is missing. * e107_tests/composer.json: roll the test-harness deps back to the Codeception 4.x family (`^4.2` plus matching module pins, with behat/gherkin capped at <4.13 to keep the PSR-0 i18n loader path working). Codeception 4.x supports the whole PHP 5.6 to 8.x range, so every cell in the matrix resolves cleanly without a parallel composer.json fork. `audit.block-insecure: false` quiets the resolver on dev-only twig advisories that would otherwise force twig v3.26+ (PHP 8.1+) on every cell. Removes the deferred-TODO `_comment` block from e107_tests/composer.json that the prior CI commit added; the deferral is no longer the plan.
Initial setup restricted these two workflows to `push: branches: master` plus pull_request, mirroring a more conservative pattern. But on a fork the master branch rarely moves and we don't open pull_requests for every iteration, so the workflows never ran on feature branches where the iteration actually happens. Drop the branch restriction so every push fires them, matching test-unit.yml and test-acceptance.yml.
The unit-test cell of CI was failing across every PHP version with: Fatal error: Declaration of installStage7HashTest::setUpBeforeClass() must be compatible with Codeception\PHPUnit\TestCase::setUpBeforeClass(): void PHPUnit 8 added a `: void` return type to setUpBeforeClass()'s signature in the abstract TestCase, and PHP enforces signature compatibility on the override. Codeception 4.2's bundled PHPUnit (9.6) carries the same contract. The downgrade pipeline strips return types as part of its 5.6 target, so the override loses its `: void`. Re-adding `: void` to the source would just trip the rule again, and skipping the rule for this one method would break the file's parseability on PHP 5.6 itself. Switch the lifecycle hook to a Codeception annotation-driven method (`@beforeClass` on an arbitrarily named static), which has no signature contract from PHPUnit and so survives the downgrade unchanged. Same runtime semantics: invoked once before the first test in the class.
The single composer.json pinning everything to Codeception 4.x for PHP 5.6 compatibility regressed every modern PHP cell, because Codeception 4.x's PHPUnit 9.x test bootstrap does not load the project autoloader for tests running with @runInSeparateProcess. Newer tests on master rely on that annotation (eIPHandlerTest is the canonical example) and started failing with "Class Codeception\\Test\\Unit not found" at the first isolated test. Split the dependency set instead: * e107_tests/composer.json keeps the modern Codeception 5.x family, which is what master used before the matrix was widened and which the @runInSeparateProcess tests already pass under. * e107_tests/composer-legacy.json is the Codeception 4.x family (matches the v2.3.x layout) for the PHP 5.6 and PHP 7.0 cells where 5.x will not composer-resolve. * The unit-test workflow copies composer-legacy.json over composer.json only on the two legacy cells before running `composer update`, so the modern cells never see the legacy dependency set and vice versa. Drops the hard PHP 8 gate at the top of class2.php that was short-circuiting every legacy-PHP cell with "PHP 8 or higher is required. Current version: 5.6.40". e107 v2 is supposed to support PHP 5.6 again now that the downgrade pipeline is in place, so the runtime check is no longer the right guard.
…e\\Db Codeception 5.x added typed properties (e.g. \`protected array \$requiredFields\`) and \`: void\` returns to \`Codeception\\Module\\Db\`. The downgrade pipeline must strip those for the PHP 5.6 / 7.0 target, but stripping the child's copy turns it into a Liskov violation against the 5.x parent that the modern PHP cells see, so every cell faulted at autoload with: Type of Helper\\DelayedDb::\$requiredFields must be array (as in class Codeception\\Module\\Db) There is no single source form that satisfies both PHP 5.6 (no typed properties at all) and PHP 8.x against the typed 5.x parent. Sidestep the conflict entirely by not overriding the property or the lifecycle method: \`\$requiredFields\` enforcement is redundant because the codeception.yml config always supplies dsn/user/password, and the _initialize override only added a cosmetic debug log. The new helper class is a pure additive subclass (extra getter methods only), which both Codeception 4.x and 5.x accept on every PHP version in the matrix.
On PHP 5.6 every \`date()\` call without a configured \`date.timezone\` emits a Strict Standards warning. The CI image ships with \`display_errors=1\`, so the warning lands on stdout, and after that e107's session_start() in class2.php can no longer send the session cookie because headers are flagged as already sent. The whole suite faulted at \`Cannot send session cookie\` before reaching any test. PHP 7+ defaults \`date.timezone\` to UTC and was unaffected, which is why this only fails in the new 5.6 / 7.0 cells. Set the timezone explicitly at the top of the suite bootstrap if php.ini did not.
…ation Rector 2.x's AbstractRector dropped the inherited autowire that used to populate \$this->staticTypeMapper for every rule subclass. The vendored Php71 nullable-type rule still touches it from decorateWithDocBlock(), so on any param with a nullable hint it faults with "Call to a member function mapPhpParserNodePHPStanType() on null". The bundled firebase/php-jwt has several such params, which is how the regression surfaced. Take the fix the 2.x-era rules use: declare StaticTypeMapper as a constructor-promoted readonly property. Composer's DI container will resolve the binding from rector-downgrade-php's bundled services. This commit also rolls back the rector.php scope tweak that tried to pull firebase/php-jwt into the downgrade target. There are more vendored-rule DI gaps (phpDocInfoFactory, others) plus a missing rule for the isset(self::CONST[expr]) pattern in JWK.php; queuing that as its own task. Today the firebase tree stays in the skip list, the StaticTypeMapper fix lands as a generally-useful improvement, and the PHP 5.6 / 7.0 unit-test cells will keep failing on the bundled firebase/php-jwt parse error until we finish the vendor downgrade.
…ration Same shape as the prior StaticTypeMapper fix: Rector 2.x's AbstractRector no longer autowires phpDocInfoFactory as an inherited service, but the vendored Php71 nullable-type rule still touches it from decorateWithDocBlock() to attach @param annotations. Any nullable param hit "Call to a member function createFromNodeOrEmpty() on null" and aborted the file. firebase/php-jwt's CachedKeySet and JWT have several such params, which is how the regression surfaced. Resolve the binding the same way the 2.x-era core rules do: declare PhpDocInfoFactory as a constructor-promoted readonly property and let rector-downgrade-php's service container fill it. With this and the prior StaticTypeMapper change, decorateWithDocBlock() has every collaborator it needs and the rule no longer crashes on nullable parameter declarations.
PHP 5.6 fatals at parse time on isset(SomeClass::CONST[$key]) with "Cannot use isset() on the result of an expression". The restriction was lifted in PHP 7.0; before that the only way to test membership in a class-constant array is to call array_key_exists() directly. Upstream rector-downgrade-php never shipped a rule for this pattern because their floor is PHP 7.2, so cover it ourselves. The rule walks each isset() argument and rewrites any ArrayDimFetch whose root is a ClassConstFetch into a FuncCall to array_key_exists($dim, $classConst). Mixed-argument isset()s combine the rewritten array_key_exists() parts with the residual isset() via BooleanAnd; pure-rewrite cases collapse to a single FuncCall. Two cases are deliberately left alone: isset(self::CONST) without a subscript (the parser allows it and PHP 5.6 accepts it) and the nested-dim form isset(self::CONST[a][b]) which would need a temp variable rewrite. No e107 source today triggers either form, so we get the immediate firebase/php-jwt JWK.php win without growing the rule beyond what we can validate.
The bundled firebase/php-jwt v6.11.1 declares PHP ^8.0 and uses typed properties, property promotion, nullable returns, scalar type hints, void returns, and isset() on a class-constant array. None of that parses on PHP 5.6, so the legacy unit-test cells faulted at autoload before reaching any test. Replace the blanket e107_handlers/vendor skip with per-package skips for every sibling that already supports PHP 5.6 or is loaded conditionally behind a version check. firebase/php-jwt falls into scope and Rector rewrites the seven non-trivial files plus the JWK.php isset() pattern (the latter via the new DowngradePhp70 isset-on-class-const rule landed in the prior commit). The eighth file SignatureInvalidException.php was already 5.6-clean. All eight files now pass `php -l` under php:5.6-cli, the full repo lint stays at 1092/1092, and a second Rector pass produces no diff.
The path-hack form (./; SameSite=Lax/) parses on PHP 5.6 but PHP 8.x rejects it at runtime: ValueError "path option cannot contain ,, ;, or whitespace". With no single setcookie() expression that satisfies both ends of the matrix, drop down to header() and build the Set-Cookie line ourselves. The CSRF cookie value is bin2hex(random_bytes(16)) so rawurlencode is a no-op for the current code, but it stays in for safety if the token shape ever changes. Domain and Secure attributes only emit when the underlying session option is truthy, mirroring the previous setcookie() behavior. SameSite=Lax always emits, which is the whole point of bypassing setcookie().
hybridauth/hybridauth v3.13.0 advertises PHP "^5.4 || ^7.0 || ^8.0" in its installed.json but the source uses PHP 7.1 nullable parameter types (?HttpClientInterface, etc.) in Hybridauth::__construct() and several adapter and provider classes. Loading the package on PHP 5.6 or 7.0 fails with "Parse error: unexpected '?'". Remove the package from the rector skip list and apply the downgrade: 23 files rewrite cleanly, the existing nullable / scalar / void / substr-falsy rules cover everything, no new custom rule needed. Lint the resulting 100 hybridauth files alongside the firebase set under PHP 5.6, all pass. While here, fix bin/php56-lint --all to actually include both firebase/php-jwt/src and hybridauth/hybridauth/src in its sweep. The prior find expression OR-grouped against -print0 with the wrong precedence and ended up only emitting the explicit-include branch (115 files). Reparenthesise so the OR feeds into a single -print0, restoring the full 1200-file sweep.
…t versions PHPUnit 8.x added onlyMethods() as the replacement for setMethods() and PHPUnit 10.x dropped the old form entirely. The matrix straddles both ends: legacy PHP cells (5.6 / 7.0) run Codeception 4.x with a PHPUnit (5.x or 6.x) that only has setMethods(), modern cells run Codeception 5.x with a PHPUnit (10.x or 11.x) that only has onlyMethods(). The previous code called onlyMethods() unconditionally and faulted on every legacy cell at the first test setup. Probe the active MockBuilder with method_exists() and pick whichever form it actually exposes. Same source runs on either Codeception generation; both surfaces produce the same mock object semantics so the rest of the test is unaffected.
PHPUnit 8 renamed assertContains(string,string) to assertStringContainsString and assertRegExp() to assertMatchesRegularExpression (and friends); PHPUnit 10 dropped the old names entirely. The matrix straddles two PHPUnit generations: modern cells run Codeception 5.x with PHPUnit 10/11 (only new names), legacy PHP 5.6 / 7.0 cells run Codeception 4.x with PHPUnit 5.x / 6.x (only old names). Tests using the new names faulted on every legacy cell at the first such assertion (around 180 call sites across ~28 files). Rather than rewrite every assertion or pin to a specific PHPUnit minor, add a PhpUnitCompat trait that forwards a small whitelist of new-style assertion names to their historical equivalents via __call. On modern PHPUnit the methods are defined natively and __call is never invoked; on legacy PHPUnit the forward fires and the test gets the assertion it asked for. Tests opt in by adding the trait inside the class body, the only change needed in each affected file. Bridged: assertStringContainsString, assertStringNotContainsString, assertMatchesRegularExpression, assertDoesNotMatchRegularExpression, assertFileDoesNotExist, assertDirectoryDoesNotExist, expectExceptionMessageMatches.
…onflict PHP 5.6 raises "Cannot use Hybridauth\\Adapter\\OpenID as OpenID because the name is already in use" the first time autoload pulls in Provider/AOLOpenID.php. The collision is between the imported Hybridauth\\Adapter\\OpenID and the sibling Hybridauth\\Provider\\OpenID class: PHP 5.6's stricter use-statement handling treats the unqualified "OpenID" as occupied by a class already known to the runtime, even though the source file itself only declares "AOLOpenID". PHP 7 relaxed the check and accepts the import. The same fix was applied to the v3.10.x bundled copy in commit 3af99c7 ("HybridAuth patch for PHP5.6", 2023) but lost in the v3.11.0 / v3.13.0 reimport. Re-apply: alias the import to "OpenIDAdapter" and update the extends clauses in the four affected files (AOLOpenID, PaypalOpenID, StackExchangeOpenID, Steam). Modern PHP cells are unaffected since the alias is just a rename. Hybridauth\\Provider\\OpenID itself uses the namespace-level "use Hybridauth\\Adapter;" and extends Adapter\\OpenID with the qualified form, so it never triggered the conflict and needs no change.
…roup filter PHPUnit's @runInSeparateProcess spawns a fresh PHP subprocess whose classloader is provided by phpunit.xml's bootstrap, not Codeception's. On the legacy PHP 5.6 / 7.0 cells running Codeception 4.x, the Codeception bootstrap that registers Codeception\Test\Unit isn't re-evaluated in the isolated process, so every @runInSeparateProcess case (four in eIPHandlerTest, plus eleven across e107Test and pluginsTest) faults with "Fatal error: Class 'Codeception\Test\Unit' not found" before the test body ever runs. This is a structural gap between PHPUnit's isolation template and Codeception 4.x's bootstrap injection, not anything specific to the tests' actual assertions. Codeception 4.x's gate enforcement is uneven: @requires PHPUnit X is parsed but ignored, @requires PHP X is also ignored at method level, and a class-level @requires fires but still loads the class first (which on those legacy cells re-triggered the same isolation fault while breaking the modern cells that had been relying on sibling state). The one filter Codeception 4.x honors reliably is --skip-group, so: * Tag every @runInSeparateProcess method across the three files with @group runs-in-separate-process. * Have the unit-test workflow append --skip-group runs-in-separate-process on the php:5.6 and php:7.0 cells; modern cells run the full set. * Keep @requires PHP 7.1 on the four eIPHandlerTest cases as belt-and-suspenders for any future PHPUnit version that takes the annotation seriously. The session-cache behavior these tests exercise is PHP-version-agnostic, so legacy coverage of the same code path still comes via the ~100 sibling tests in eIPHandlerTest that don't need a separate process.
…mysqlTest installStage7HashTest::invokeResolver elided ReflectionProperty/Method setAccessible() calls on the assumption that PHP 8.1+ made them unnecessary, but the legacy cells still enforce the visibility check and faulted with "Cannot access non-public member e_install_for_5631_test::e107". Re-add the setAccessible() calls behind a PHP_VERSION_ID guard so they only run on PHP < 8.1 and skip the 8.5+ deprecation notice on modern cells. e_db_mysqlTest::testDb_Close asserted on the closed mysqli's ->server_info property; PHP 5.6's mysqli driver emits "Couldn't fetch mysqli" the moment any property is read off a closed handle, which Codeception 4.x escalated to a test error. Suppress with @ on the empty() check, the value still comes through as falsy so the assertion still gets what it wants on every interpreter.
…eak session_start The CI runner inherits PHP's CLI default of display_errors=1 across every cell. Several PHP versions in the matrix now emit notices the moment Codeception's autoloader pulls in classes that predate their stricter contract checks: PHP 7.0 flags the LSP gap between Helper\\Base and Codeception 4.x's \\Codeception\\Module, and PHP 8.4 warns whenever a typed parameter defaults to null without an explicit "?" prefix (which the downgrade pipeline strips for the legacy cells, leaving the modern cells running the implicit form). Both messages land on stdout, and any stdout content before session_start() trips "headers already sent" inside e107's bootstrap, killing the whole suite before any test runs. Silence display_errors at the very top of the test bootstrap. Codeception's ErrorHandler subscriber still catches genuine errors and reports them through its own channel, so the suite remains as loud as before about anything that actually matters.
…rent signature Codeception 4.x's \Codeception\Module declares _before(\Codeception\TestInterface \$test) with no default value. The instinct is to mirror that hint on Helper\\Base::_before so PHP 7.0 doesn't raise its LSP "declaration should be compatible" warning, but Rector's DowngradeParameterTypeWideningRector treats any typed parameter that defaults to null as nullable and strips the hint at commit time. A typed signature here would fail the Rector idempotency CI gate on the very next pass. Both the PHP 7.0 LSP warning and the PHP 8.4 implicit-nullable deprecation that this untyped form would otherwise emit are absorbed by the display_errors=0 sink in tests/_bootstrap.php, so neither warning reaches stdout and neither trips e107's session_start() with "headers already sent". Add a docblock recording the chain so the next maintainer understands why putting the type hint back breaks CI even though it looks safer.
… PHP 5.6-safe form testRequireReusesCacheOnSecondCall builds a PHP template source as a string, writes it to a temp file, and require()s the file three times. The original body used null-coalescing (??) to seed $GLOBALS['__test_cached'], which PHP 5.6 parses at require-time and fatals on, killing the test process with exit 255 before PHPUnit can record the failure. Every test in the file runs to completion on PHP 5.6 except this one, and the two that come after it never get a chance to execute. Rewrite the body to use isset()-ternary, which is semantically identical and parses cleanly on every PHP version in the matrix. The test itself (the outer file) was already 5.6-safe because the ?? lived inside a string literal; only the templated source needed updating.
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.
@Deltik instructed me to open this PR in a direct message; the work below is on his behalf, not unprompted.
Motivation
e107 v2 has a split between its build environment and its deployment artifact:
composer.jsonrequiresphp: ">=8.0"(bumped ina9d20d8e93). Anyone runningcomposer installorcomposer updateneeds PHP 8.0+. That stays as-is.master. e107 v2 has effectively no build chain; site operators upgrade bygit pulling the core repo (the built-in "Git sync" feature, or manual equivalents) and serve the source as-is. Vendored packages undere107_handlers/vendor/are part of the deployment artifact, not regenerated fromcomposer.lockon each site (seee107_handlers/vendor/README.md; Composer-managed vendor regeneration is a v3 goal).In practice that means the source on
masterhas to remain runnable on whatever PHP the v2 install base actually runs. The README's compatibility table still pins v2.3.4+ at a PHP 5.6 floor for site operators, but recent core changes and the^3.12hybridauth/^6.xfirebase/php-jwtbumps had introduced syntax (?Typenullables,setcookie()options-array form,isset(self::CONST[k]), etc.) that fatals on those interpreters. A site operator on PHP 5.6 / 7.0 trying to Git-sync to currentmasterlands on a parse error and is stuck.This PR reattaches the source to that floor by routing the whole tree through a Rector 2.x downgrade pipeline at commit time, then gates the result on CI so future drift fails fast. Nothing changes at runtime on PHP 7.1+; every rewrite is purely syntactic, and modern cells continue to execute the same code paths.
Pieces
Rector tooling and rules
chore(rector): add Rector 2.x downgrade tooling targeting PHP 5.6: vendored Rector +rector-downgrade-phpruleset undere107_tests/tools/rector/, with four custom rules underE107\Rector\for cases the upstream set doesn't cover.feat(rector): add isset() on class-constant downgrade rule for PHP 5.6:isset(self::CONST[k])is a PHP 5.6 fatal; rewrites toarray_key_exists(k, self::CONST).DowngradeNullableTypeDeclarationfor Rector 2.x's stricter autowiring (StaticTypeMapper/PhpDocInfoFactoryare no longer auto-injected onAbstractRector).bin/php56-lintandbin/php56-shellhelpers for local repros.Bulk source downgrade
chore: apply Rector PHP 8.x to 5.6 downgrade across the whole tree: the actual rewrite of e107 core + selected vendor packages (firebase/php-jwt,hybridauth/hybridauth). Other vendor packages (guzzlehttp,composer/*,phpmailer, etc.) stay skipped because they either ship their own compat shims or aren't loaded at runtime on legacy installs.fix(source): replace three PHP 7+ syntax forms with 5.6-compatible equivalentsfix(csrf): emit Set-Cookie header directly to keep SameSite=Lax portable:setcookie()'s 7.3 options-array form fatals on 5.6, the pre-7.3 "fold into path" trick fatals on 8.x; emitting theSet-Cookieheader by hand is the only expression that works everywhere.fix(hybridauth): alias Adapter\OpenID import to dodge PHP 5.6 name conflict: re-applies a historical patch lost in thehybridauth^3.13upgrade.CI
ci: gate PRs on Rector idempotency and PHP 5.6 lint cleanliness+ per-push trigger.ci(test-unit): bring PHP 5.6 and PHP 7.0 back into the unit-test matrix+split Codeception dependency set by PHP version(Codeception 4.x for legacy, 5.x for modern;mysql:8.0excluded on legacy cells for the knownutf8mb4charset incompatibility).Test infrastructure adjustments for the legacy cells
test(unit): bridge PHPUnit 8/9 assertion names for the legacy PHP cells: aPhpUnitCompattrait forwardsassertStringContainsString/assertMatchesRegularExpressionetc. to their PHPUnit 5.x/6.x equivalents via__call.test(e_admin_dispatcher): bridge onlyMethods/setMethods across PHPUnit versions.test+ci: gate @runInSeparateProcess tests off legacy PHP cells via @group filter: PHPUnit's isolated subprocess can't reach Codeception 4.x's bootstrap (it loadsphpunit.xml's bootstrap, not Codeception's), so the 15 affected tests carry@group runs-in-separate-processand CI appends--skip-groupon the legacy cells.@requires PHP 7.1stays as belt-and-suspenders for any PHPUnit version that takes the annotation seriously.installStage7HashTestreflection guard,e_db_mysqlTestclosed-mysqli warning suppression,DelayedDbLSP-sensitive override removal,_bootstrap.phpdisplay_errors=0sink,Helper\Base._beforedocblock).Test plan
bin/php56-lint --all): 1202 / 1202 passrector process --dry-run): no changes proposede107_tests/tools/rector/custom-rules/E107/for upstream-compat driftvendor/firebase/php-jwtandvendor/hybridauth/hybridauthis the preferred mechanism vs. a Composer post-install patchset (the latter wouldn't survive Git sync, so probably no)composer.json'sconfig.platform.phpshould drop back to"5.6"to match the downgraded source, or stay at"8.0"to keep the build-environment requirement aligned with what maintainers actually runNotes