Increase tracked ob_get_level() only in ob_start()'s truthy branch instead of unconditionally#6087
Conversation
…h instead of unconditionally
- Add `ObStartFunctionTypeSpecifyingExtension` that raises the tracked
`ob_get_level()` by one only in the branch where `ob_start()` is known to have
returned a truthy value (`if (ob_start())`, `if (!ob_start()) { return; }`,
`if (ob_start() === false) { return; }`, ...). An unchecked/bare `ob_start()`
no longer assumes an active buffer, so `ob_get_contents()`, `ob_get_clean()`,
`ob_get_flush()` and `ob_get_length()` keep `false`/`int|false` in their
return type and comparisons like `ob_get_clean() === false` are no longer
reported as always-false.
- `OutputBufferHelper::opensBufferOnSuccess()` marks the level-incrementing
functions; `FuncCallHandler` skips the unconditional level increase for them
(the extension handles the truthy branch). Level-decrementing functions keep
their unconditional behavior - assuming a close succeeded only reduces
narrowing, so it cannot introduce false positives.
- The fix covers the whole `ob_get_*()` family at once, since all of them derive
their non-`false` narrowing from the tracked buffer level.
- Update `output-buffering.php` to establish the active buffer with a checked
`ob_start()`, and add `bug-14985.php`.
|
IIRC we did intentionally not implement it this way, because a |
This create issues like phpstan/phpstan#14985 I don't have strong opinion about this topic. I wonder if we
I also wonder if the whole ob_ inferences was worth it or should just be removed... |
|
@VincentLanglet wdyt about the alternative in #6097 ? |
Summary
ob_start()returnsfalsewhen it fails to open an output buffer, but PHPStan treated everyob_start()call as if it always succeeded: it unconditionally raised the trackedob_get_level(), which in turn removedfalsefrom the return type ofob_get_contents(),ob_get_clean(),ob_get_flush()andob_get_length(). As a result, code that checks the buffer contents without checkingob_start()'s result got a false positive:The fix ties the buffer-opening side effect to
ob_start()'s return value: the level is only raised in the branch whereob_start()is known to have returned a truthy value.Changes
src/Type/Php/ObStartFunctionTypeSpecifyingExtension.php, aFunctionTypeSpecifyingExtensionthat, in the truthy context ofob_start(), overwrites the trackedob_get_level()expression withlevel + 1. Because it goes throughTypeSpecifier, it works for every condition form:if (ob_start()),if (!ob_start()) { return; },if (ob_start() === false) { return; }, etc.src/Analyser/ExprHandler/Helper/OutputBufferHelper.php: addedopensBufferOnSuccess()to identify the level-incrementing functions (ob_start).src/Analyser/ExprHandler/FuncCallHandler.php: no longer applies the unconditional level increase for functions that only open the buffer on success — that increase is now applied to the truthy branch by the extension. Level-decrementing functions are unchanged.tests/PHPStan/Analyser/nsrt/output-buffering.phpto open the buffer via a checkedob_start()where an active buffer is expected, and added explicit assertions for the new unchecked-ob_start()behavior.tests/PHPStan/Analyser/nsrt/bug-14985.php.Root cause
The output-buffer level tracking modeled
ob_start()'s side effect (buffer opened →ob_get_level()increased) as unconditional, whileob_start()actually reports success/failure through itsboolreturn value. Since thefalse-removal narrowing of the wholeob_get_*()family is derived from the tracked level, an uncheckedob_start()incorrectly narrowed all of them to non-false. Applying the level increase only toob_start()'s truthy branch fixes the entire family in one place.Analogous constructs probed:
ob_get_contents(),ob_get_clean(),ob_get_flush(),ob_get_length()— all fixed by the single root-cause change (they read the shared tracked level).ob_get_clean,ob_get_flush,ob_end_clean,ob_end_flush): assuming a close succeeded only lowers the level and therefore reduces narrowing, which cannot produce this class of false positive, so they intentionally keep their unconditional behavior.Test
tests/PHPStan/Analyser/nsrt/bug-14985.phpreproduces the reported case: after a bareob_start(),ob_get_clean()is inferred asstring|false(the=== falsecheck is no longer flagged), while after a checkedif (!ob_start()) { return; }it isstring.tests/PHPStan/Analyser/nsrt/output-buffering.phpgained assertions for uncheckedob_start()(buffer not assumed active) and for narrowing through the truthy branch ofif (ob_start()), the!ob_start()guard, and theob_start() === falseguard.Fixes phpstan/phpstan#14985