Fix counting *scanf() format string placeholders

[hakre]

Update PrintfHelper.php:

Make public function getScanfPlaceholdersCount(string $format): ?int returning the sscanf() vetted number of placeholders that give/return/assign conversions.

refs:

• Truncate sscanf/fscanf format string at NUL byte before counting placeholders #5591
• sscanf NUL terminators in format phpstan#14567

[phpstan-bot]

This pull request has been marked as ready for review.

[hakre]

Do you know by chance why https://github.com/phpstan/phpstan-src/actions/runs/25431166468/job/74597885107 keeps failing on my changes? I'd like to better understand @staabm

[staabm]

Do you know by chance why phpstan/phpstan-src/actions/runs/25431166468/job/74597885107 keeps failing on my changes?

we see the same errors on others PRs targeting 2.1.x -> this means these errors are not related to your PR

[staabm]

these 2 tests look identical. can't we just merge them and remove the #[RequiresPhp(...)]?

[hakre]

These two tests cover different internal paths of getScanfPlaceholdersCount:

• PHP < 8.0 uses @sscanf and converts the suppressed warning to null.
• PHP ≥ 8.0 catches a ValueError and also returns null.

The #[RequiresPhp] attributes guarantee each branch is exercised in the correct CI environment. Merging them would lose the version‑gated execution and the guarantee that both paths are actually tested where they matter (the original request specifically asked for a test that runs on CI PHP 7.x with a warning‑emitting pattern).

So the tests are intentionally symmetric – a “Gegenprobe” to make sure the legacy and the modern path behave identically.

I’m happy to add a short comment above each test to make that explicit, or to merge them if you still prefer – just let me know.

[staabm]

We should merge the tests if they are identical

[hakre]

Fair point – the assertion body is identical. The reason I kept them separate is the test report itself. With #[RequiresPhp] and distinct names, the test runner says:

• testReturnsNullForInvalidPatternOnLegacyPhpVersion – skipped on PHP 8 (→ we know the @ path wasn't testable here)
• testReturnsNullForInvalidPatternOnPhp8 – passed on PHP 8 (→ confirms the exception path)

A single merged test would just show "passed" on both, without making it visible that the other branch couldn't be tested in that environment. It turns the "skipped" signal into silence.

If you value that clarity, I can reduce duplication by extracting the assertion into a private method but keep the two named, version‑gated test cases. If you'd rather have a single test for simplicity, that works too – just say the word. 😊

[staabm]

I think this can be simplified

Suggested change

	if ($this->phpVersion->throwsValueErrorForInternalFunctions()) {
	try {
	$result = sscanf('', '%*n' . $format);
	} catch (ValueError) {
	return null;
	}
	} else {
	$result = @sscanf('', '%*n' . $format);
	}
	if ($result === null) {
	return null;
	}
	return count($result);
	try {
	$result = sscanf('', '%*n' . $format);
	} catch (ValueError) {
	return null;
	}
	if ($result === null) {
	return null;
	}
	return count($result);

which would also simplify the tests IMO

[hakre]

I see the appeal of simplifying, but the @ operator is essential on PHP < 8.0 to suppress the E_WARNING that sscanf emits for invalid formats. On PHP 7.x, catch (ValueError) cannot catch warnings – they'd propagate and cause test failures or unwanted noise. The version check (throwsValueErrorForInternalFunctions()) guards exactly this: on newer PHP we can rely on exceptions, on older PHP we need @.

The tests mirror that – they ensure both branches (suppressed warning on 7.x, caught exception on 8.x) return null as expected. If we drop the @ and always use try‑catch, the PHP 7.x test would see an uncaught warning and likely fail.

I’m happy to add a code comment clarifying the reasoning, but I’d recommend keeping the current implementation – it's the minimal correct cross‑version approach.

[staabm]

the if ($this->phpVersion->throwsValueErrorForInternalFunctions()) { condition does not only depend on the php runtime php-version, but can also be defined via phpstan.neon phpVersion.

[hakre]

Thanks for the clarification about the config. Since I'm not familiar with how the DI wiring and CI setup interact, I'd rather not guess.

Could you please give me concrete instructions for both?

1. Production code – keep the current version‑check + @, or simplify to a plain try/catch?
2. Tests – we currently have three cases: the two that look identical plus the one that expects a ValueError with a fake legacy version. With your proposed production change, I could reduce these to just one test, or leave them as is. Just tell me exactly which test(s) you want, and I'll make it so.

Happy to push whatever you prefer – just point me in the right direction. 😊 (Late here on Friday, so I'll pick this up over the weekend. If you don't hear back right away, feel free to give me a nudge—notifications have a mind of their own sometimes.)

[staabm]

my suggestion is in the first comment

[hakre]

Ah, right — your first comment. Got it.

Will remove the version branch + @, plain try/catch, merge tests into one.
Pushing later, will ping. Thanks.

[hakre]

Please find the changes pushed as requested.
The PHP 7.4 CI now fails with the expected warning from sscanf—the very warning the @ was suppressing.
What would you like me to do next?