Repository navigation
Secreats API: Code quality improvements - #15481
Closed
peterwilsoncc wants to merge 28 commits into
Closed
peterwilsoncc wants to merge 28 commits into
peterwilsoncc wants to merge 28 commits into
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
peterwilsoncc
force-pushed
the
follow-up/secrets-code-quality
branch
from
November 5, 2026 00:49
97470af to
9803bce
Compare
peterwilsoncc
marked this pull request as ready for review
November 5, 2026 01:08
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. Props Bot commandsYou can trigger Props Bot actions by commenting on this PR:
|
Contributor
Author
|
Spent a couple of hours with Claude Code to modify the tests and improve docs: All changes were made by AI and reviewed by me, both prior to commit and in the changes tab of this pull request. Various requests included:
|
There was a problem hiding this comment.
🟡 Changes recommended
Interface-only @covers targets lose implementation coverage, and missing multisite guards can cause network tests to fail when group exclusions are overridden in single-site runs.
3 open issues
What changed in this PR
This PR improves documentation and PHPUnit tests for WordPress’s Secrets API, strengthening regression checks without changing its storage or encryption logic.
Changes:
- Adds test summaries, coverage annotations, ticket references, and clearer assertion messages.
- Expands checks for error contracts, key misconfiguration, hooks, and extension-point failures while removing duplicate tests.
- Cleans up production comments, documents globals, and updates the unnamespaced-name diagnostic.
| File | Description |
|---|---|
tests/phpunit/tests/secrets/wpSetSecretAndWpGetSecret.php |
Strengthens write and hook assertions; removes duplicate cases. |
tests/phpunit/tests/secrets/wpSecretVersion.php |
Documents constant and class-shape tests. |
tests/phpunit/tests/secrets/wpSecretsValidateName.php |
Clarifies validation tests and renames data providers. |
tests/phpunit/tests/secrets/wpSecretsOptionStore.php |
Improves storage assertions and scope-test naming. |
tests/phpunit/tests/secrets/wpSecretsMemzero.php |
Clarifies memory-clearing tests and removes a redundant assertion. |
tests/phpunit/tests/secrets/wpSecretsConfigKeyProvider.php |
Strengthens key-source and salt-fallback checks. |
tests/phpunit/tests/secrets/wpSecretsCipher.php |
Documents cipher tests and checks tampering error codes. |
tests/phpunit/tests/secrets/wpSecret.php |
Clarifies masking and lifecycle tests; removes a duplicate case. |
tests/phpunit/tests/secrets/wpListSecrets.php |
Strengthens listing assertions and uses the public network-write API. |
tests/phpunit/tests/secrets/wpImportOptionAsSecret.php |
Adds explicit import-success checks and test documentation. |
tests/phpunit/tests/secrets/wpDeleteSecret.php |
Improves deletion and hook assertions. |
tests/phpunit/tests/secrets/threeStateContract.php |
Adds a wrong-valid-key regression test. |
tests/phpunit/tests/secrets/siteHealth.php |
Strengthens diagnostic checks and adds Multisite group annotations. |
tests/phpunit/tests/secrets/readonlyProviderConformance.php |
Adds the ticket reference. |
tests/phpunit/tests/secrets/providerConformance.php |
Adds the ticket reference. |
tests/phpunit/tests/secrets/provider.php |
Documents provider tests and removes duplicate failure coverage. |
tests/phpunit/tests/secrets/networkScope.php |
Clarifies scope tests and replaces runtime skips with group annotations. |
tests/phpunit/tests/secrets/mockKeyringConformance.php |
Adds the ticket reference. |
tests/phpunit/tests/secrets/extensionPoints.php |
Strengthens unavailable-backend and plaintext-exclusion checks. |
tests/phpunit/tests/secrets/errorCodes.php |
Expands error-code checks and verifies completeness. |
tests/phpunit/tests/secrets/configKeyringConformance.php |
Adds the ticket reference. |
tests/phpunit/tests/secrets/capabilities.php |
Documents capability tests and introduces role datasets. |
tests/phpunit/tests/secrets/brokenDropinFallbacks.php |
Adds explicit fallback error-code assertions. |
tests/phpunit/includes/secrets/trait-wp-secrets-assertions.php |
Adds optional context to slot-decryption assertions. |
tests/phpunit/includes/secrets/class-wp-secrets-provider-conformance.php |
Adds coverage annotations and clearer conformance assertions. |
tests/phpunit/includes/secrets/class-wp-secrets-keyring-conformance.php |
Adds coverage annotations and clearer keyring assertions. |
src/wp-includes/secrets/class-wp-secrets-option-store.php |
Cleans up comment formatting and whitespace. |
src/wp-includes/secrets/class-wp-secrets-libsodium-provider.php |
Clarifies hook documentation and removes proposal commentary. |
src/wp-includes/secrets/class-wp-secrets-key-manager.php |
Standardizes ID capitalization in documentation. |
src/wp-includes/secrets/class-wp-secrets-cipher.php |
Standardizes ID capitalization in parameter documentation. |
src/wp-includes/secrets/class-wp-secrets-broken-store.php |
Removes an extra blank line. |
src/wp-includes/secrets/class-wp-secret.php |
Revises debug-output documentation and ID terminology. |
src/wp-includes/secrets.php |
Updates documentation, global annotations, and the naming diagnostic. |
src/wp-includes/load.php |
Documents globals used by the secrets drop-in loader. |
src/wp-admin/includes/secrets-site-health.php |
Clarifies Site Health function descriptions. |
src/wp-admin/includes/class-wp-site-health.php |
Removes an extra blank line. |
Review effort: Balanced
| /** | ||
| * Ensure that the provider reports a label that is neither empty nor only whitespace. | ||
| * | ||
| * @covers WP_Secrets_Provider::get_label |
| * must read back identically from another blog's context. Requires multisite. | ||
| * | ||
| * @group multisite | ||
| * @group ms-required |
Comment on lines
+229
to
+231
| * Note that var_export() has no equivalent hook. It is safe regardless, because | ||
| * the plaintext is never a declared property for it to enumerate; it will surface | ||
| * the (non-sensitive) name and fingerprint rather than this placeholder. |
wporg-sync
pushed a commit
that referenced
this pull request
Nov 5, 2026
Brings the Secrets API tests in line with the PHPUnit test-writing guidelines and tidies the inline documentation. Documentation: * Remove references to the feature plugin, its proposal and its decision records, including in the `_doing_it_wrong()` notice for unnamespaced names. * Add `@global` annotations for the drop-in globals, rewrite the Site Health test summaries as statements, and correct the `WP_Secret::__debugInfo()` description, which also applies to `print_r()`. * Use `/* */` for multi-line comments, "ID" rather than "id", and `@subpackage Secrets` in the test files. Tests: * Add `@ticket 66187` to each test class, a summary and `@covers` annotation to each test, and order annotations consistently. * Move `@covers` targets in the conformance suites from the interfaces to the implementations under test, declared on the concrete test classes, so coverage is attributed to the code that runs. The base suites no longer filter coverage for extending classes. * Add a unique failure message to every assertion within each test, including a new `$message` parameter for `assertRecordSlotDecryptsTo()`. * Assert the result of every setup write. This exposed two separate-process tests that passed without testing anything because no usable key was available; both now define a test key and check the specific error returned. * Remove duplicated tests, rename tests whose names did not match what they check, and name single-use data providers after their test. * Add tests for a store that cannot list its secrets, for every `WP_SECRETS_ERROR_*` constant being pinned, and for a valid but different `WP_SECRETS_KEY`. * Strengthen assertions that could pass without checking their stated behavior, such as missing error codes, counts and the salt-derived key. * Use the `ms-required` and `multisite` groups with `skipWithoutMultisite()` for multisite-only tests, and `skipWithMultisite()` for the `ms-excluded` test, instead of skipping with `markTestSkipped()`. Developed in #15481. Follow-up to r64337, r64344, r64345, r64674. See #66187, #66353. git-svn-id: https://develop.svn.wordpress.org/trunk@65272 602fd350-edb4-49c9-b593-d223f7449a82
Contributor
Author
wporg-sync
pushed a commit
to WordPress/WordPress
that referenced
this pull request
Nov 5, 2026
Brings the Secrets API tests in line with the PHPUnit test-writing guidelines and tidies the inline documentation. Documentation: * Remove references to the feature plugin, its proposal and its decision records, including in the `_doing_it_wrong()` notice for unnamespaced names. * Add `@global` annotations for the drop-in globals, rewrite the Site Health test summaries as statements, and correct the `WP_Secret::__debugInfo()` description, which also applies to `print_r()`. * Use `/* */` for multi-line comments, "ID" rather than "id", and `@subpackage Secrets` in the test files. Tests: * Add `@ticket 66187` to each test class, a summary and `@covers` annotation to each test, and order annotations consistently. * Move `@covers` targets in the conformance suites from the interfaces to the implementations under test, declared on the concrete test classes, so coverage is attributed to the code that runs. The base suites no longer filter coverage for extending classes. * Add a unique failure message to every assertion within each test, including a new `$message` parameter for `assertRecordSlotDecryptsTo()`. * Assert the result of every setup write. This exposed two separate-process tests that passed without testing anything because no usable key was available; both now define a test key and check the specific error returned. * Remove duplicated tests, rename tests whose names did not match what they check, and name single-use data providers after their test. * Add tests for a store that cannot list its secrets, for every `WP_SECRETS_ERROR_*` constant being pinned, and for a valid but different `WP_SECRETS_KEY`. * Strengthen assertions that could pass without checking their stated behavior, such as missing error codes, counts and the salt-derived key. * Use the `ms-required` and `multisite` groups with `skipWithoutMultisite()` for multisite-only tests, and `skipWithMultisite()` for the `ms-excluded` test, instead of skipping with `markTestSkipped()`. Developed in WordPress/wordpress-develop#15481. Follow-up to r64337, r64344, r64345, r64674. See #66187, #66353. Built from https://develop.svn.wordpress.org/trunk@65272 git-svn-id: http://core.svn.wordpress.org/trunk@64421 1a063a9b-81f0-0310-95a4-ce76da25c4cd
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.
Trac ticket: https://core.trac.wordpress.org/ticket/66187
Use of AI Tools
AI assistance: Yes
Tool(s): Claude
Model(s): Opus 5.5 Medium
Used for: Full notes incoming. Initial prompt:
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.