docs: correct an inverted numeric-key example and a stale test docblock - #668
Open
rjzondervan wants to merge 1 commit into
Open
docs: correct an inverted numeric-key example and a stale test docblock#668rjzondervan wants to merge 1 commit into
rjzondervan wants to merge 1 commit into
Conversation
Two nits from Wilco's approving review on #656, which merged before they were addressed. Comments only - no shipped behaviour changes. THE NUMERIC-STRING KEY EXAMPLE WAS BACKWARDS. normaliseUserIds() explained why a keyed set is unsafe for user ids and then gave the wrong example: it claimed "0123" would come back from array_keys() as 123. PHP coerces an array key that is a CANONICAL decimal integer string, so it is "123" and "-7" that become ints, while "0123", "007" and "1e3" stay strings. Verified rather than reasoned: "123" -> integer "0123" -> string "-7" -> integer "007" -> string, "1e3" -> string The conclusion held - a keyed set is still the wrong tool here - but a wrong example is worse than none, because it teaches the opposite rule to the next reader. Corrected, and it now notes the sharper reason: coercion depends on the SHAPE of the id, so some ids would survive and others would not, which is worse than breaking uniformly. The same error appeared in the PR discussion; a correction is posted there. THE TEST DOCBLOCK STILL DESCRIBED THE OLD CONTRACT. testRecipientCertificatesPreservesOrderAndDeduplicates said the caller zips the response against the request and that order is "part of the contract" - the claim that was removed from the endpoint's own documentation in the same PR. The test still asserts order, which is right; it just is not a guarantee anyone may rely on. Assisted-by: ClaudeCode:claude-opus-5
Contributor
Quality Report — ConductionNL/keepiq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-l10n-js | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| composer | ✅ | ✅ 111/111 | |||
| npm | ✅ | ✅ 543/543 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ✅ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-09-08 14:16 UTC
Download the full PDF report from the workflow artifacts.
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.
Follow-up to the two nits in @WilcoLouwerse's approving review on #656, which merged before they were addressed. Comments and one test docblock only — no shipped behaviour changes.
The numeric-string key example was backwards
normaliseUserIds()explains why a keyed set is the wrong tool for deduplicating user ids, and then gave the wrong example — it claimed"0123"would come back fromarray_keys()as123. Wilco caught it. PHP coerces an array key only when it is a canonical decimal integer string:Verified by running it rather than reasoning about it.
The conclusion still stood — a keyed set really is unsafe here — but a wrong example is worse than no example, because it teaches the next reader the opposite rule. The corrected comment also states the sharper reason: coercion depends on the shape of the id, so some ids would survive intact and others would silently change type. That's worse than breaking uniformly, because it wouldn't show up in testing with the wrong sample of ids.
I repeated the same error in the #656 discussion, so a correction is posted on that thread too.
Stale test docblock
testRecipientCertificatesPreservesOrderAndDeduplicatesstill said the caller zips the response against the request and that order is "part of the contract, not an accident" — the exact claim removed from the endpoint's own documentation in the same PR, after Wilco pointed out it doesn't survive deduplication. The test still asserts order, which is correct; it just isn't a guarantee anyone may depend on.1192 tests pass, phpcs 0 errors, phpmd clean.
🤖 AI disclosure: prepared with Claude Code (Opus 5). Commits carry
Assisted-by:trailers. Reviewed and submitted by @rjzondervan.