fix: two silent setters, and the warnings that were pointing at them - #3354
Merged
Merged
Conversation
The suite reported 10 PHP warnings and 23 deprecations. Two of the warnings were not hygiene: they were production bugs, and both had been written into the tests as known behaviour. ORGANISATION::SETACTIVE(FALSE) NEVER DEACTIVATED ANYTHING It called parent::setActive(active: $value) with a NAMED argument. Entity has no such method — it is reached through __call(array $args), where a named argument lands under its name, so Entity::setter() read an undefined $args[0] and the value never arrived. OrganisationController has an endpoint whose whole job is deactivating an organisation, and it could not work. OrganisationTest asserted assertTrue() on a value that should have been false and called it "the current actual behavior". ENDPOINTLOG::SETRESPONSE() STORED NULL ON EVERY CALL LOG Same root cause, same shape. setStatusCode and setStatusMessage were positional and worked; the response payload was dropped on every endpoint call ever logged. EndpointServiceTest said "setResponse uses named arg in source code (known issue), so response may be null. We verify it was attempted" — and then verified nothing. Both are fixed and both now assert the correct behaviour. Reintroducing either bug reddens its test; that was checked, not assumed. I swept for the rest of the class rather than stopping at two: 94 Entity subclasses, 686 magic setter names, 256 named-argument call sites in lib/. Every other match is legitimate — $objectService->setRegister() is a real service method, ->setObject(type:, id:) is OCP's INotification, and the last two candidates were the regex matching self:: in a class constant. The class is bounded at these two. THE REST - SettingsController: `while ($row !== false)` never terminates if fetch() answers null, which it can. Two other loops in the same file already used is_array(); this one did not. - BsnFormat: str_pad() was handed a null, deprecated in 8.1 and a TypeError in 9. Cast once up front; an array/object is now refused explicitly, which is the contract str_pad() used to enforce further down by accident. - SaveObject: a schema with a null slug cannot be found BY slug, and feeding the null to strtolower()/strcasecmp() is deprecated. Skipped instead. - Four test files: mocks that answered [] or null where the single real implementation always fills the contract (rechainAll, processSourceChunks, handlePreValidationCascading). The mocks were made to keep the contract rather than the services made defensive about a fake. RechainAuditTrail also gained the tombstone-count assertion its message never had. - Dynamic properties declared; ReflectionProperty::setValue() given its explicit null object argument at 14 sites. Suite: 19046 tests. Warnings 10 -> 0, deprecations 23 -> 2, risky 3 -> 0 (the risky three landed separately in #3351). The last two deprecations are ZipArchive::open() on an empty fixture file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| test-l10n-parity | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 174/174 | |||
| 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-03 08:10 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
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.
The suite reported 10 PHP warnings and 23 deprecations. Two of the warnings were not hygiene — they were production bugs, and both had been written into the tests as known behaviour.
Organisation::setActive(false)never deactivated anythingIt called
parent::setActive(active: $value)with a named argument.Entityhas no such method: it is reached through__call(array $args), where a named argument lands under its name, soEntity::setter()read an undefined$args[0]and the value never arrived.OrganisationControllerhas an endpoint whose whole job is deactivating an organisation, and it could not work.OrganisationTestassertedassertTrue()on a value that should have been false, and called it "the current actual behavior".EndpointLog::setResponse()stored NULL on every call logSame root cause, same shape.
setStatusCodeandsetStatusMessagewere positional and worked; the response payload was dropped on every endpoint call ever logged.EndpointServiceTestsaid "setResponse uses named arg in source code (known issue), so response may be null. We verify it was attempted" — and then verified nothing.Both fixed, both now assert correct behaviour. Negative controls run, not assumed: reintroducing either bug reddens its test.
The class, not just the instances
94
Entitysubclasses, 686 magic setter names, 256 named-argument call sites inlib/. Every other match is legitimate:$objectService->setRegister()is a real service method,->setObject(type:, id:)is OCPsINotification, and my last two candidates were the regex matchingself::` in a class constant. The class is bounded at these two sites.Worth knowing for next time: the cause surfaced as
Undefined array key 0pointing intovendor/nextcloud/ocp/.../Entity.php, where it reads as somebody else`s problem. It was ours both times.The rest
SettingsControllerwhile ($row !== false)never terminates iffetch()answers null, which it can. Two other loops in the same file already usedis_array().BsnFormatstr_pad()was handed a null — deprecated in 8.1, a TypeError in 9. Cast once up front; array/object now refused explicitly, which is the contractstr_pad()used to enforce further down by accident.SaveObjectstrtolower()/strcasecmp()is deprecated. Skipped instead.[]ornullwhere the single real implementation always fills the contract (rechainAll,processSourceChunks,handlePreValidationCascading). The mocks were made to keep the contract rather than the services made defensive about a fake.RechainAuditTrailCommandTestsetValue()ReflectionProperty::setValue()given its explicit null object argument at 14 sites.Result
19046 tests. Warnings 10 → 0, deprecations 23 → 2, risky 3 → 0 (the risky three landed separately in #3351).
The last two deprecations are
ZipArchive::open()on an empty fixture file, left alone deliberately: the fixture is empty on purpose and changing it would change what the test exercises.🤖 Generated with Claude Code