From 416c74ac5bb040e610dbd16b97e72303317e2b66 Mon Sep 17 00:00:00 2001 From: kristianwind Date: Wed, 16 Sep 2026 14:00:21 +0200 Subject: [PATCH] Answer the details modal, and make two dead checks check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit wp-admin builds "View version details" as plugin-install.php?tab=plugin-information&plugin=, which calls plugins_api() — which asks wordpress.org. This plugin is not there, so the link opened a modal reading "Plugin not found." on the same screen that was telling the shop owner an update was ready. Seen on the live shop, not inferred. The filter is answered here now: name, version from the feed, author, links, compatibility, and the release notes as the changelog — escaped rather than rendered, because a release body is not a template language. Another plugin's slug falls through untouched, with an assertion on that: answering for somebody else would replace their modal with ours. Two checks in ci/update-smoke.sh could not fail, both found by the subscriptions port, both sitting under a comment explaining exactly what they were for: - the plugin directories were counted into a variable that was never read. The comment above it describes a zip installing a SECOND copy beside the first; nothing tested it. Now: directories starting with the slug, exactly one. - the last step ran `wp eval 'echo WC_EDM_VERSION;'`, hard-coding this plugin's constant into a script that is copied between plugins. Elsewhere it printed an empty value and carried on. The name is read out of the plugin header now and the value is compared against the version that was installed, because files can be replaced while the running code still reports the old version. Both verified failing before being believed: copies=2 and copies=0 fail, running != expected fails, and a plugin with no *_VERSION constant takes the branch that says so instead of comparing against an empty string. CORRECTION to the note added with the release-id fix. It said those four releases published correctly "because this instance's author id is -1". That is wrong in a way that invites relaxing: gitea.nolimit.dk is one instance and both plugins publish to it. The difference is WHO publishes. A release created by the Actions job token has the ghost user as its author ("id":-1) and the minus sign hides it from [0-9]+; a release created by a real user — a cross-repo PAT, or one published by hand — has "id":1 and the greedy match lands on it. Verified: this repo's latest release reports {'id': -1, 'login': 'Ghost'}. Corrected in CLAUDE.md and docs/REUSE.md. docs/REUSE.md gains "Make a check fail once before you believe it", listing all three dead checks found today. A green build is exactly what a broken check looks like. Co-Authored-By: Claude Opus 5 (1M context) --- ci/update-smoke.sh | 36 +++++++++++++++++++---- docs/REUSE.md | 28 ++++++++++++++++-- includes/class-updater.php | 58 ++++++++++++++++++++++++++++++++++++++ tests/smoke.php | 50 ++++++++++++++++++++++++++++++++ 4 files changed, 164 insertions(+), 8 deletions(-) diff --git a/ci/update-smoke.sh b/ci/update-smoke.sh index 3dce184..cac320f 100755 --- a/ci/update-smoke.sh +++ b/ci/update-smoke.sh @@ -92,13 +92,39 @@ echo " status: $status" [ "$now" = "$expected" ] || { echo "Expected $expected after the update." >&2; exit 1; } [ "$status" = active ] || { echo "The plugin is no longer active after updating itself." >&2; exit 1; } -# A zip whose top-level directory is not the slug installs a SECOND copy -# beside the first instead of replacing it, and the old one stays active. -count=$(docker exec "$id-cli" sh -c 'ls /var/www/html/wp-content/plugins | wc -l') +ci_step "one copy of the plugin, not two" +# A zip whose top-level directory is not the slug installs a SECOND copy beside +# the first instead of replacing it, and the old one stays active. This used to +# count the directories into a variable and then never look at it — a check +# that could not fail, sitting under a comment explaining exactly what it was +# for. docker exec "$id-cli" ls /var/www/html/wp-content/plugins | sed 's/^/ /' +copies=$(docker exec "$id-cli" sh -c \ + "ls -d /var/www/html/wp-content/plugins/$PLUGIN_SLUG* 2>/dev/null | wc -l" | tr -d '[:space:]') +echo " directories starting with $PLUGIN_SLUG: $copies" +[ "$copies" = 1 ] || { + echo "Expected exactly one, got $copies. The zip unpacked to a directory that is not the slug." >&2 + exit 1 +} + +ci_step "the code that is now running reports the new version" +# Files can be replaced while the running code still reports the old version — +# an opcache that was not invalidated, or a constant defined somewhere else. +# The constant name is read out of the plugin header rather than hard-coded: +# this script is copied between plugins, and a name that does not exist over +# there prints an empty value and carries on cheerfully. +const=$(sed -n -E "s/^[[:space:]]*define\([[:space:]]*'([A-Z_]*VERSION)'.*/\1/p" "$PLUGIN_MAIN" | head -1) +if [ -n "$const" ]; then + running=$(wp eval "echo defined( '$const' ) ? $const : '';" | tr -d '[:space:]') + echo " $const = ${running:-}" + [ "$running" = "$expected" ] || { + echo "The files say $expected but the running code says '${running:-undefined}'." >&2 + exit 1 + } +else + echo " $PLUGIN_MAIN defines no *_VERSION constant — nothing to cross-check." +fi -ci_step "and it still works afterwards" -wp eval 'echo WC_EDM_VERSION;' | sed 's/^/ WC_EDM_VERSION: /' wp plugin list --name="$PLUGIN_SLUG" --field=update | grep -qx none \ && echo " WordPress no longer offers an update — the header and the feed agree." diff --git a/docs/REUSE.md b/docs/REUSE.md index 648df72..143c498 100644 --- a/docs/REUSE.md +++ b/docs/REUSE.md @@ -126,6 +126,20 @@ measures something else. It cost two rounds here, once on If a script reports something you just fixed as still missing, `git status` before you debug the script. +## Make a check fail once before you believe it + +Three checks in this directory turned out to be incapable of failing, and all +three sat under a comment explaining exactly what they were for: + +- `tests/smoke.php` printed `FAIL` and exited 0 for four releases. +- `ci/static.sh`'s PHP lint passed while linting an empty directory. +- `ci/update-smoke.sh` counted the plugin directories into a variable and never + looked at it — under a comment about a zip installing a second copy. + +None of them were caught by reading the code, and a green build is exactly what +a broken check looks like. So: break it on purpose once, watch it go red, then +put it back. It costs one run. + ## Two guards worth understanding before you move them **`.gitea/workflows/` hides `.github/workflows/` completely.** Gitea uses the @@ -144,9 +158,17 @@ the *last* id in the payload, and a release object carries an embedded `author` and, once anything is attached, an `assets` array — each with an id of its own. `ci/publish-release.sh` uses a four-line `json_field()` over `python3` instead. Measured here: the greedy pattern returns the asset ids 1/2/3/4 for -releases whose real ids are 416/417/419/421. Four releases were published -correctly anyway, purely because this instance's author id is **-1** and the -minus sign stops `[0-9]+` from matching. That is not a safety margin. +releases whose real ids are 416/417/419/421. + +Four releases were published correctly anyway, and the reason is **who +published them, not which instance**: a release created by the Actions job +token has the ghost user as its author (`"id":-1`), and the minus sign stops +`[0-9]+` from matching. A release created by a real user — a cross-repo PAT, or +one published by hand — has `"id":1` and the greedy match lands on it. Do not +read the `-1` as a safety margin; it lasts until the first release somebody +publishes by hand. And on a re-run the lookup fetches a response that *has* +assets, where the last id is an asset id: positive, on any instance, with any +token. **A forge's own download URL is not the address your users can reach.** `browser_download_url` is built from the forge's configured root URL; diff --git a/includes/class-updater.php b/includes/class-updater.php index 94c0c0b..e9c4d8e 100644 --- a/includes/class-updater.php +++ b/includes/class-updater.php @@ -72,6 +72,9 @@ class WC_EDM_Updater { // And "Check again" has to actually check again. add_action( 'admin_init', array( $this, 'forget_on_force_check' ) ); + + // "View version details" next to the update notice. + add_filter( 'plugins_api', array( $this, 'plugin_details' ), 10, 3 ); } /** @@ -155,6 +158,58 @@ class WC_EDM_Updater { ); } + /** + * Answer the "View version details" modal. + * + * WordPress builds that link as + * plugin-install.php?tab=plugin-information&plugin=, which calls + * plugins_api() — and plugins_api() asks **wordpress.org**. This plugin is + * not there, so without this filter the link opens a modal containing an + * error, on the same screen that is otherwise telling the shop owner an + * update is ready. Nothing warns you: the link looks like every other + * plugin's. + * + * @param false|object|array $result Response so far. + * @param string $action Requested action. + * @param object $args Request arguments. + * @return false|object|array + */ + public function plugin_details( $result, $action, $args ) { + $slug = dirname( plugin_basename( WC_EDM_PLUGIN_FILE ) ); + + if ( 'plugin_information' !== $action || empty( $args->slug ) || $args->slug !== $slug ) { + return $result; + } + + require_once ABSPATH . 'wp-admin/includes/plugin.php'; + $plugin = get_plugin_data( WC_EDM_PLUGIN_FILE, false, false ); + $release = $this->latest_release(); + + $notes = $release && '' !== $release['notes'] ? $release['notes'] : ''; + + return (object) array( + 'name' => $plugin['Name'], + 'slug' => $slug, + 'version' => $release ? $release['version'] : $plugin['Version'], + 'author' => $plugin['Author'], + 'author_profile'=> $plugin['AuthorURI'], + 'homepage' => $plugin['PluginURI'], + 'requires' => $plugin['RequiresWP'], + 'requires_php' => $plugin['RequiresPHP'], + 'tested' => $release ? $release['tested'] : '', + 'download_link' => $release ? $release['package'] : '', + 'icons' => array( 'svg' => WC_EDM_PLUGIN_URL . 'assets/icon.svg' ), + 'sections' => array( + 'description' => wpautop( esc_html( $plugin['Description'] ) ), + // Plain text from the release notes, not markdown: rendering it + // as HTML would mean trusting whatever is in a release body. + 'changelog' => '' !== $notes + ? wpautop( esc_html( $notes ) ) + : wpautop( esc_html__( 'Release notes are published with each release.', 'wc-email-discount' ) ), + ), + ); + } + /** * Drop the cached feed. */ @@ -288,6 +343,9 @@ class WC_EDM_Updater { 'package' => $base . '/releases/download/' . rawurlencode( $tag ) . '/' . rawurlencode( $zip ), 'url' => $base . '/releases/tag/' . rawurlencode( $tag ), 'tested' => $tested, + // The release notes are the changelog. Kept so the "View version + // details" modal has something true to show. + 'notes' => isset( $body['body'] ) ? (string) $body['body'] : '', ); } diff --git a/tests/smoke.php b/tests/smoke.php index 23f0a69..ca8903a 100644 --- a/tests/smoke.php +++ b/tests/smoke.php @@ -347,6 +347,56 @@ set_transient( $offered = wc_edm()->updater->offer_update( false, get_plugin_data( WC_EDM_PLUGIN_FILE, false, false ) ); wc_edm_is( 'the current version is answered too, not passed over', WC_EDM_VERSION, is_array( $offered ) ? $offered['version'] : null ); +// The "View version details" link next to the update notice. WordPress builds +// it as plugin-install.php?tab=plugin-information&plugin=, which calls +// plugins_api() — and plugins_api() asks wordpress.org, where this plugin does +// not exist. Unanswered, that link opens a modal reading "Plugin not found." +// on the same screen that is telling the shop owner an update is ready. +// Measured on a live shop before this was written. +set_transient( + WC_EDM_Updater::TRANSIENT, + array( + 'version' => '99.0.0', + 'package' => 'https://gitea.nolimit.dk/kw/demo/releases/download/v99.0.0/demo.zip', + 'url' => 'https://gitea.nolimit.dk/kw/demo', + 'tested' => '7.1', + 'notes' => "v99.0.0 — a headline\n\nA line of release notes.", + ), + 600 +); + +// plugins_api() lives in the admin includes, which a WP-CLI context does not +// load. wp-admin has it; this does not. +require_once ABSPATH . 'wp-admin/includes/plugin-install.php'; + +$wc_edm_slug = dirname( plugin_basename( WC_EDM_PLUGIN_FILE ) ); +$details = plugins_api( 'plugin_information', array( 'slug' => $wc_edm_slug ) ); + +wc_edm_ok( + 'the details modal is answered here, not by wordpress.org', + ! is_wp_error( $details ), + is_wp_error( $details ) ? $details->get_error_message() : '' +); +wc_edm_is( 'and it is about this plugin', $wc_edm_slug, is_wp_error( $details ) ? null : $details->slug ); +wc_edm_is( 'it reports the version from the feed', '99.0.0', is_wp_error( $details ) ? null : $details->version ); +wc_edm_ok( + 'the changelog is the release notes', + ! is_wp_error( $details ) && false !== strpos( $details->sections['changelog'], 'A line of release notes' ), + 'a modal with an empty changelog is barely better than an error' +); +wc_edm_ok( + 'and the notes are escaped rather than rendered', + ! is_wp_error( $details ) && false === strpos( $details->sections['changelog'], 'updater->plugin_details( false, 'plugin_information', (object) array( 'slug' => 'akismet' ) ), + 'answering for somebody else would replace their modal with ours' +); + +delete_transient( WC_EDM_Updater::TRANSIENT ); + /* * The "Enable auto-updates" link on the Plugins screen. *