fix(parallel): drop the deprecated SplObjectStorage::contains(), then stop suppressing deprecations in the suite - #40
Merged
Conversation
PHP 8.5 deprecates SplObjectStorage::contains() in favour of offsetExists(). PreforkTaskDirectory keyed its task-to-address map by an SplObjectStorage and probed it with contains() in both register() and addressOf(), so every prefork test raised the deprecation on 8.5. offsetExists() has the same semantics and has existed since PHP 5.3, so this needs no floor change. The suite never showed it: every .phpt suppresses E_DEPRECATED. Forcing error_reporting=E_ALL over each file's own --INI-- block on PHP 8.5.9 with z-engine 8.5.0 turned 12 of the 121 tests red, every one of them on this deprecation; with the call replaced the same forced-E_ALL run is 121/121 on both PHP 8.4.19 (z-engine 8.4.2) and PHP 8.5.9 (z-engine 8.5.0), and no deprecation of any kind is emitted. Closes #39 Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013foRd1XwLwqjUSkSWeWrMe
Every one of the 121 functional tests carried a third --INI-- line, error_reporting=E_ALL & ~E_DEPRECATED. It was added for a dependency: PHPUnit's .phpt runner forces display_errors=1, so a deprecation raised by z-engine — back when it was consumed from a development branch — was prepended to the captured output of every test and failed --EXPECT-- blocks over noise that had nothing to do with this library. The stable releases now required (~8.4.2 || ~8.5.0) raise none, and the line was hiding this package's own deprecations too. That is exactly how the SplObjectStorage::contains() call PHP 8.5 deprecated stayed green through 12 tests until it was fixed in the previous commit. Removing the suppression is only safe in that order, so it goes second. ffi.enable=1 and opcache.jit=off stay: FFI cannot be enabled at runtime and the JIT rewrites the executor internals the engine hooks depend on. The guard test is renamed to match and now checks both halves — that each file declares both required lines, and that no file sets error_reporting at all, so a filter cannot be reintroduced to silence a future deprecation instead of fixing it. tests/Support/childProcess.php spawns children with the same settings and drops the line for the same reason. AGENTS.md now documents two lines and why the third went away; README.md and .github/workflows/ci.yml never named it and are unchanged. Verified with a runner that reproduces PhptTestCase: 121/121 on PHP 8.4.19 (z-engine 8.4.2) and 121/121 on PHP 8.5.9 (z-engine 8.5.0), with no diagnostics of any kind in the output. Closes #38 Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013foRd1XwLwqjUSkSWeWrMe
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
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.
Two linked issues, in the order that makes the second one safe.
Closes #39
Closes #38
Why the order matters
Every
.phptin the suite carriederror_reporting=E_ALL & ~E_DEPRECATED. That line was added for a dependency: PHPUnit's.phptrunner forcesdisplay_errors=1, so a deprecation raised by z-engine — back when it was consumed from a development branch — was prepended to the captured output of every test and failed--EXPECT--blocks over noise that had nothing to do with this library.But it filtered this package's own deprecations just as effectively. That is exactly how #39 stayed green: PHP 8.5 deprecated
SplObjectStorage::contains(),PreforkTaskDirectorycalls it on every task registration and lookup, and 12 tests exercised it in silence.So removing the suppression first would have turned #38 into a 12-test regression. Fix the deprecation, then take the blindfold off.
Part 1 —
fix(parallel):offsetExists()instead ofcontains()src/Parallel/PreforkTaskDirectory.phpkeys its task-to-address map by an\SplObjectStorageand probed it withcontains()in bothregister()andaddressOf().offsetExists()has identical semantics and has existed since PHP 5.3, so there is no floor change and nothing else moves.I swept the rest of the tree rather than assuming this was the only one:
->contains(insrc/— only these two call sites, both fixed. The one remaining hit anywhere istests/Support/shared.php, which is$arena->arena()->contains($address, 8)on a shared-data extension object, not SPL.->attach(/->detach(— all this package's own methods (SharedArena::attach(),Context::detach()), never theSplObjectStorageones.$this->addresses[$task]), which is not deprecated.The strongest check is empirical, and it is in the numbers below: with the fix in place a forced-
E_ALLrun of the whole suite emits zero diagnostics of any kind on both minors, so there is no second deprecation hiding behind the first.Part 2 —
test(tests): drop the suppressionerror_reportingline is gone from all 121tests/Functional/*.phpt.ffi.enable=1andopcache.jit=offstay — FFI cannot be enabled at runtime, and the JIT rewrites the executor internals the engine hooks depend on.testEveryTestDeclaresTheThreeRequiredIniLines.phpt→testEveryTestDeclaresTheTwoRequiredIniLines.phptand now checks both halves: that each file declares both required lines, and that no file setserror_reportingat all. A filter cannot be quietly reintroduced to silence the next deprecation instead of fixing it.tests/Support/childProcess.phpspawned its children with the same three settings and explicitly described itself as mirroring the.phptfiles; it drops the line for the same reason.AGENTS.mddocumented three mandatory lines in two places (the INI-settings section and the test-writing rules). Both now describe two, and the historical note explains what the third was for and why it no longer applies, rather than leaving a dangling reference to a deprecation that no longer occurs.README.mdand.github/workflows/ci.ymlnever named the setting — the CIini-valuesareffi.enable=1, zend.assertions=1, opcache.jit=off— so neither needed a change. GreppingE_DEPRECATEDacross the tree now returns only the two deliberate historical mentions inAGENTS.mdand the guard test's comment.Measured results
PHPUnit and PHPStan could not be installed in this environment — network egress reaches only
lisachenko/*repos, socomposer installwithout--no-devfails. The suite was run instead with a small runner that reproduces what PHPUnit'sPhptTestCasedoes: it writes the--FILE--body next to the.phptso relative includes resolve, applies the file's own--INI--plusdisplay_errors=1, compares--EXPECT--exactly and matches--EXPECTREGEX--unanchored. CI runs the realcomposer teston both minors.Dependencies were resolved per minor (
rm -rf vendor composer.lockbetween runs): PHP 8.4.19 → z-engine 8.4.2, PHP 8.5.9 → z-engine 8.5.0.Step 1 — before any change, forcing
error_reporting=E_ALLover each file's own--INI--block:SplObjectStorage::contains()deprecation prepended to otherwise-correct outputStep 2 — after Part 1, same forced-
E_ALLrun:Step 3 — after Part 2, the ordinary run (each file's own
--INI--, no forcing):No segfault, bus error or hung child in any run. The known flake from #33 (
testAnAlreadyCompleteSlotIsAwaitedWithoutParking, ~1 in 4 full-suite runs) did not appear in any of the four full-suite runs, so nothing was retried or re-run.Files changed
src/Parallel/PreforkTaskDirectory.php— two call sitestests/Functional/*.phpt— 121 files, one line removed each; the guard test renamed and rewrittentests/Support/childProcess.php— the mirrored-dflag and its docblockAGENTS.md— two sectionsGenerated by Claude Code