Drop the ISecureRandom carve-out: upstream took the deprecation back - #255
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
nursoda
force-pushed
the
docs/drop-the-securerandom-carve-out
branch
from
September 5, 2026 08:03
2722250 to
fa5c522
Compare
nursoda
changed the title
nursoda
force-pushed
the
docs/drop-the-securerandom-carve-out
branch
from
September 5, 2026 08:06
fa5c522 to
efa5ea0
Compare
The suppression was right when it was written. Nextcloud deprecated ISecureRandom::generate on 24 June 2026 in favour of Randomizer::getBytesFromString (server#61538), which needs PHP 8.3 while this app's floor is 8.2. Nextcloud reverted it on 20 August (server#63412): the interface is a service and therefore mockable in tests, the default character list would have to be copied to every caller, and it is used all over the code base. The annotation lived on master only and reached no stable branch — stable34 had already branched off, stable35 branched on 5 September, after the revert. So the carve-out never suppressed anything on any OCP the app analyses against. This removes the last piece of it. The psalm handler and its CompatibilityShimsTest guard went with the Nextcloud 35 change; the REVIEW.md entry was left behind and was the only place still stating the deprecation as current. The path itself was worth keeping. doc/developers.md now describes it: show the annotation and name the pull request that added it, suppress the one method by name rather than the issue type, register it in CompatibilityShimsTest with the condition that ends it, and watch the branch the suppression is actually needed for. That last step is the one that failed here: the guard was tied to this repository's PHP floor while the real condition was an upstream decision. Reading the installed vendor/nextcloud/ocp instead would have been wrong too — the annotation only ever lived on the server's master, so such a guard would have demanded the suppression's removal from day one. The section also carries the search that finds a reversal, so a missing annotation is not mistaken for one that never existed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Olav Seyfarth <olav@seyfarth.de>
nursoda
force-pushed
the
docs/drop-the-securerandom-carve-out
branch
from
September 5, 2026 08:19
efa5ea0 to
76f1a98
Compare
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.
What this does
Removes the
ISecureRandom::generateentry fromREVIEW.md, the last piece of acarve-out whose reason upstream withdrew.
The short history
ISecureRandom::generatein favour ofRandom\Randomizer::getBytesFromString()(server#61538). That call needs
PHP 8.3; this app's floor is 8.2 for as long as it supports Nextcloud 33.
CompatibilityShimsTestholds the exception until the PHP floor reaches 8.3, and
REVIEW.mdrecords it.(server#63412): the interface is a
service and therefore mockable in tests, the default character list would have to be
copied to every caller, and it is used throughout the code base.
The annotation only ever lived on
master.stable34had branched off before itlanded, and
stable35branched on 5 September, after it was gone — so the suppressionnever actually suppressed anything on an OCP version this app analyses against.
Why it still had to go
The psalm handler and its test went with #253. This
REVIEW.mdentry was left behind,and a
REVIEW.mdentry exists to stop a reviewer raising a settled question — one thatstates a withdrawn deprecation as current does the opposite.
Should Nextcloud deprecate the method again,
DeprecatedMethod errorLevel="error"saysso on the next run; that was counter-proved in #253.
🤖 Generated with Claude Code, verified, tweaked and approved by @nursoda.
The path is written down now
doc/developers.mdgains a section on what to do when an OCP method the app needs isdeprecated: show the annotation and name the pull request that added it, suppress the one
method by name rather than the issue type, register it in
CompatibilityShimsTestwiththe condition that ends it, and watch the branch the suppression is
actually needed for. The last step is the one that failed here: the guard was tied to this
repository's PHP floor while the real condition was an upstream decision. Reading the
installed
vendor/nextcloud/ocpinstead would have been wrong too — the annotation onlyever lived on the server's
master, so such a guard would have demanded the suppression'sremoval from day one, and its verdict would change with the OCP version of each matrix
job. The section also carries the search that finds a reversal, so that a missing
annotation is not read as one that never existed.