Repository navigation
Plugins: Avoid undefined activateUrl key for active plugins on multisite - #14541
Open
Aayushkalikote wants to merge 2 commits into
Open
Aayushkalikote wants to merge 2 commits into
Aayushkalikote wants to merge 2 commits into
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the 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. |
Aayushkalikote
restored the
fix/66346-plugin-dependencies-activate-url
branch
October 20, 2026 09:02
…isite. On multisite, `WP_Plugin_Dependencies::check_plugin_dependencies_during_ajax()` appended `networkwide=1` to `$status['activateUrl']` for every user who can manage network plugins, even when the URL was never set because the plugin is already active. This raised an "Undefined array key" warning and returned a broken `activateUrl` of `?networkwide=1`. The network-wide argument is now only added when an activation URL exists, matching the single site behavior. Follow-up to r57545. Fixes #66346.
Aayushkalikote
force-pushed
the
fix/66346-plugin-dependencies-activate-url
branch
from
October 20, 2026 09:04
bb3f9c9 to
43463bf
Compare
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. |
ryanwelcher
added a commit
to WordPress/contributor-toolkit
that referenced
this pull request
Oct 20, 2026
…unk is behind (#702) ## Why When a site's local `trunk` is older than the commit a pull request branched from, the pull request preview lists every file trunk changed in between as well as the pull request's own. @zaerl hit this on #677: WordPress/wordpress-develop#14541 changes 2 files, and the app listed dozens. ## What changes The root cause: `describePullRequestHead` measured the list from the merge base with the site's local `trunk`, which only moves when the contributor updates trunk. - The `git:preview-pr` handler now fetches origin's `trunk` before the pull request head and measures the list from that commit. It reads the commit from `FETCH_HEAD`, so it works on a site with no remote-tracking ref. The site's own `trunk` and the checkout are not moved. - If that fetch fails, the preview falls back to the site's trunk, logs the error, and the preview dialog says: "Could not download the latest trunk, so this list may also include changes from trunk that are not part of this pull request." Deliberately not in this PR: the checkout itself and the recorded base of `pr/N` are unchanged. Only the preview list was wrong. ## How to test this **Platforms:** any. Use the Buildkite build for the current head, `0f91e22`. **Starting state:** a wordpress-develop site on `trunk` with no edits, whose trunk is older than WordPress/wordpress-develop#14541. To make one, quit the app and run this from the site's folder (needs Git on your machine): ``` git reset --hard HEAD~300 ``` Write down the commit it prints, or run `git rev-parse trunk` and keep the output. 1. Open the app and select the site. Do not click to update trunk. 2. On the **Apply a patch or PR** card, on the **Pull request** tab, type `14541` in **Pull request URL or number** and click **Apply PR**. - Expected: the dialog titled **Apply PR #14541** says "PR #14541 changes 2 files." - Expected: the "Could not download the latest trunk…" line is not shown. 3. Click **Cancel**. 4. From the site's folder, run `git rev-parse trunk`. - Expected: the same commit you wrote down before step 1. To see the bug, run the same steps on a trunk build: step 2 says "changes" followed by a much larger number of files. **What must not have happened:** - The site's `trunk` moved (step 4). The preview downloads origin's trunk but must not update the site's own. - The checkout changed, or a rebuild or `npm install` started, when you only opened the preview and cancelled. **Tests:** the two `git:preview-pr` tests in `tests/unit/ipc-wiring.test.cjs` fail on the old handler. The trunk-download failure case is covered by those tests and by the new `describePrPreview` test in `tests/unit/pr-checkout-renderer.test.cjs`. ## Risks and limitations - The fallback notice can't be tested by hand. It needs the trunk fetch to fail while the pull request fetch straight after it succeeds, and I could not stage that. - Every preview now makes one more network round trip. The fetch brings commits and trees only, with no file contents. - A pull request based on a release branch is still measured against trunk's merge base, so its list also carries that branch's own commits. That is unchanged and documented in `describePullRequestHead`. - Review: 0 to fix here, 1 follow-up, fixed in this PR. ## Related Fixes #701. Reported on #677. --- <details> <summary>Design decisions and alternatives considered</summary> - **Fetch only on the preview, not on checkout.** The checkout records `pr/N`'s base from the fetched head and does not use the list, so the bug was confined to the preview. - **Fall back instead of refusing.** A failed trunk fetch should not block testing a pull request. The preview still works and says its list may be too long. - **One fetch for both refs** would save the round trip, but it needs a new multi-line `FETCH_HEAD` reader in `git-read.cjs`. Not worth it for now. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> 0 [fix here] · 1 [follow-up]. The follow-up is fixed in `0f91e22`. - **Fixed:** when the trunk fetch failed, the preview fell back without saying so, which brought #701 back silently. The handler now returns `measuredFromSite`, and the dialog shows a sentence when it is true. - **Fixed (style):** the `fetchOriginTrunk` comment said the list was measured against "the trunk it was written on". It now says origin's current trunk. - Checked with no finding: the trunk commit is read before the PR fetch overwrites `FETCH_HEAD`; the partial clone fetches no file contents; the wiring tests fail without the fix; bundled Git only, the quit sweep covers the new fetch, no store writes. Records: - **Review:** completed. Separate read-only agent context, following `.github/instructions/code-review.instructions.md`. Reviewed head `991f291` / base `e43e9c8`. Lint clean, unit suite 2113 pass / 0 fail, PR journeys (`pr-checkout`, `apply-card`, `patch-apply`, `gutenberg-site`) 17 passed. Outcome: 1 follow-up. - **Since review:** `991f291 → 0f91e22` checked. This commit is the follow-up fix and the comment correction. Lint clean, unit suite 2114 pass / 0 fail, the same 17 journeys pass. Not re-reviewed by a separate context. </details> <details> <summary>Implementation notes</summary> - `fetchOriginTrunk` in `src/pr-checkout.js` is `fetchBranch(dir, origin, trunk)`. It goes through `streamGit` on the bundled binary and is registered with `trackGitChild`, so quitting ends it. - `measuredFromSite` travels in the preview result, which `use-apply-patch.jsx` already spreads into `applyPreview`. `describePrPreview` turns it into `trunkNote`, and `previewWords` passes that to the dialog. </details> <details> <summary>Screenshots or recording</summary> None. The only visible change is the fallback sentence, which can't be staged by hand (see Risks). The file count in step 2 is the before and after. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…ependencies-activate-url # Conflicts: # tests/phpunit/tests/ajax/wpAjaxCheckPluginDependencies.php
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.
On multisite,
WP_Plugin_Dependencies::check_plugin_dependencies_during_ajax()appendsnetworkwide=1to$status['activateUrl']for every user who can manage network plugins. When the plugin is already active, the URL was never set, so this raisedUndefined array key "activateUrl"and returned"activateUrl":"?networkwide=1".This adds an
isset()check so the network-wide argument is only added when an activation URL exists, matching single site behavior.Tests (
@group ajax,@group ms-required):activateUrl. Fails on trunk atclass-wp-plugin-dependencies.php:481, passes with the fix.activateUrlcontainingnetworkwide=1.Note: #14521 adds broader test coverage for this method in the same file path (
tests/phpunit/tests/ajax/wpAjaxCheckPluginDependencies.php) and leaves this defect out on purpose. Happy to move these cases into that file if it lands first.Trac ticket: https://core.trac.wordpress.org/ticket/66346
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5.5
Used for: Locating the ticket, writing the fix and the tests. I reviewed the change, confirmed the test fails without the fix and passes with it, and ran phpcs and the existing plugin dependencies tests on single site and multisite.
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.