docker: resolve monorepo plugins_url via composer.json name#48542
Merged
docker: resolve monorepo plugins_url via composer.json name#48542
Conversation
The plugins_url filter assumed every monorepo package's vendor symlink
was automattic/jetpack-{dirname}, which doesn't hold for packages whose
composer name diverges (e.g. packages/scan is automattic/jetpack-scan-page).
Read composer.json for the canonical name so realpath matching succeeds
and gated pages like Scan can load assets in the Docker dev environment.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
dhasilva
added
[Status] Needs Review
Contributor
Author
|
From a Claude review done locally: |
tbradsha
previously approved these changes
May 6, 2026
Contributor
tbradsha
left a comment
tbradsha
left a comment
There was a problem hiding this comment.
One non-blocking comment. Thanks for the fix!
Comment on lines
+80
to
+83
| static $cache = array(); | ||
| if ( array_key_exists( $package_dir, $cache ) ) { | ||
| return $cache[ $package_dir ]; | ||
| } |
| $composer_path = $package_dir . '/composer.json'; | ||
| if ( is_readable( $composer_path ) ) { | ||
| $contents = json_decode( file_get_contents( $composer_path ), true ); | ||
| if ( is_array( $contents ) && isset( $contents['name'] ) && is_string( $contents['name'] ) ) { |
Contributor
There was a problem hiding this comment.
I don't think we need the is_array() check, as the isset() on a string key handles that. You could take it one step further and guard against an empty key too (this'd include "0", which is fine in this scenario):
Suggested change
| if ( is_array( $contents ) && isset( $contents['name'] ) && is_string( $contents['name'] ) ) { | |
| if ( ! empty( $contents['name'] ) && is_string( $contents['name'] ) ) { |
Drop the redundant is_array() check — isset() with a string key already short-circuits when the variable isn't an array — and switch to !empty() so a missing key, null, or empty string all fail the same guard. Per review feedback from @tbradsha. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tbradsha
approved these changes
May 6, 2026
LiamSarsfield
pushed a commit
that referenced
this pull request
May 6, 2026
* docker: resolve monorepo plugins_url via composer.json name
The plugins_url filter assumed every monorepo package's vendor symlink
was automattic/jetpack-{dirname}, which doesn't hold for packages whose
composer name diverges (e.g. packages/scan is automattic/jetpack-scan-page).
Read composer.json for the canonical name so realpath matching succeeds
and gated pages like Scan can load assets in the Docker dev environment.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docker: simplify composer name guard
Drop the redundant is_array() check — isset() with a string key already
short-circuits when the variable isn't an array — and switch to !empty()
so a missing key, null, or empty string all fail the same guard. Per
review feedback from @tbradsha.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Fixes #
Proposed changes
plugins_urltranslation for monorepo packages whose Composernamediverges from their directory name.composer.jsonto determine the canonical vendor symlink name (e.g.packages/scan/→automattic/jetpack-scan-page), instead of hard-codingautomattic/jetpack-{dirname}.composer.jsonfor every URL the package generates.composer.jsonis missing/malformed.Without this,
realpath()matching in the filter never succeeds for divergent packages, soplugins_urlreturns the unresolved monorepo path and asset URLs 404. The most visible symptom is the gated Scan page (wp-admin/admin.php?page=jetpack-scan) failing to load its React app.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
jetpack docker up -d) ontrunk(i.e. without this fix).wp-admin/admin.php?page=jetpack-scanand confirm the page fails to render (blank container / 404s on the package's assets in DevTools → Network)./wp-content/plugins/jetpack/jetpack_vendor/automattic/jetpack-scan-page/....packages/*assets in dev) to confirm there are no regressions inplugins_urlresolution.