Tiled Gallery: source Photon-domain check from block editor settings filter - #50426
Tiled Gallery: source Photon-domain check from block editor settings filter#50426enejb wants to merge 8 commits into
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
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! Jetpack plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 23 files. Only the first 5 are listed here.
4 files are newly checked for coverage.
|
There was a problem hiding this comment.
Pull request overview
This PR fixes VIP-site image breakage in the Tiled Gallery (and Image Compare, which reuses its photonization logic) by making the “skip external Photon domain” decision reliably available to block editor code via block editor settings, rather than via a fragile window.load + global state approach.
Changes:
- Expose a
skip_photon_domainboolean through theblock_editor_settings_allfilter, defaulting totrueonly for VIP plans and allowing overrides viajetpack_skip_photon_domain. - Replace the per-block/version VIP detection with a single shared
skipPhotonDomain()helper that reads fromcore/editor’s editor settings. - Remove legacy inline localization (
jetpack_plan) and deduplicate deprecated block versions to import the shared helper.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| projects/plugins/jetpack/extensions/blocks/tiled-gallery/utils/index.js | Removes fragile load-time plan sniffing and adds skipPhotonDomain() sourced from core/editor settings. |
| projects/plugins/jetpack/extensions/blocks/tiled-gallery/tiled-gallery.php | Adds block_editor_settings_all filter to inject skip_photon_domain and introduces jetpack_skip_photon_domain override filter. |
| projects/plugins/jetpack/extensions/blocks/tiled-gallery/deprecated/v6/utils/index.js | Drops local VIP check and imports shared skipPhotonDomain() for consistent behavior. |
| projects/plugins/jetpack/extensions/blocks/tiled-gallery/deprecated/v4/utils/index.js | Drops local VIP check and imports shared skipPhotonDomain() for consistent behavior. |
| projects/plugins/jetpack/extensions/blocks/tiled-gallery/deprecated/v3/utils/index.js | Drops local VIP check and imports shared skipPhotonDomain() for consistent behavior. |
| projects/plugins/jetpack/extensions/blocks/tiled-gallery/deprecated/v2/utils/index.js | Drops local VIP check and imports shared skipPhotonDomain() for consistent behavior. |
| projects/plugins/jetpack/changelog/update-tiled-gallery-skip-photon-domain | Adds a Jetpack plugin changelog entry documenting the behavior change and new filter. |
coder-karen
left a comment
There was a problem hiding this comment.
Thanks for looking into this!
I can confirm everything works as expected following the testing steps.
However, I do see a block validation issue after just refreshing the page, after the post is saved (the saved content includes photonized URLs).
I believe the issue is that editor settings are runtime editor state, not block attributes. Since block validation operates on parsed block attributes and regenerated markup, code that runs during parsing or validation shouldn't depend on editor settings, which is my understanding based on the information here.
Could a possibility be to keep the same capability-based check, but PHP side? Then just change the delivery by extending Jetpack_Editor_Initial_State?
|
@coder-karen good catch — confirmed, and the diagnosis was exactly right. Went with your suggestion: the check stays in PHP on the same plan signal (with the Verified on a JN site with the plan forced to
Also added unit tests that set only the initial-state global, so a store-based read would fail them. PR description and testing instructions are updated with the reload / no-recovery-prompt step. |
… core/block-editor
…e so block validation sees it
e930611 to
e922bc6
Compare
coder-karen
left a comment
There was a problem hiding this comment.
Thanks for the follow-up!
The scenario resulting in block validation issues now no longer does -- though I notice there is still one scenario that does: a TG block added on a VIP site (or in my case a mock VIP site using the mu-plugin) before the branch is applied. Once the branch is applied and the post / page refreshed, the block validation issue is visible.
Ultimately the 'save' logic is still different (checking skipPhotonDomain vs true == isVIP) in the deprecations. Shouldn't only the current version be using skipPhotonDomain, and each deprecation be still forcing the old Photon path, perhaps then needing a new deprecation? 🤔
| * @return {boolean} True when the external Photon domain should be skipped. | ||
| */ | ||
| export function skipPhotonDomain() { | ||
| return true === getJetpackData()?.jetpack?.skip_photon_domain; |
There was a problem hiding this comment.
getJetpackData is deprecated, and getScriptData is recommended now:
There was a problem hiding this comment.
Switched to getScriptData. The flag now ships in JetpackScriptData via Jetpack_Script_Data::set_admin_script_data() and is read as getScriptData()?.jetpack?.flags?.skipPhotonDomain, matching how showJetpackBranding is read. Still a synchronous inline global, so it is available while the post is parsed — which is the constraint that ruled out editor settings.
…t the deprecated editor initial state
…ing galleries stay valid
|
@coder-karen both good catches — and your instinct on the deprecations was the fix for the remaining scenario, so thanks for pushing on it. Deprecations. You're right that they shouldn't have been following the site's setting: they exist to reproduce markup that was already saved, and content saved under them holds external Photon URLs. v2/v3/v4/v6/v8 now always use the Photon domain, and only the current version consults the setting. (In passing, that also means the old private The remaining validation scenario. Fixed, and it is the deprecation change that fixes it — a gallery saved on a (mock) VIP site before the branch is applied now stays valid after applying it, no recovery prompt, and the block is quietly re-serialized onto the site's own host the next time it is saved. I also added a dedicated deprecation mirroring the current markup with the Photon domain forced. Worth flagging why, since it is arguably redundant: I tested with it removed, and that content still validates — but only against v8, and only because block validation forgives the stray whitespace node in v8's wrapper ( Verified on a JN site with the plan forced to
688 JS tests pass, including new coverage for the save → reload round trip across a setting change. Two notes on limits: the new PHPUnit cases in PR description and testing instructions updated, including steps for the existing-content case. |
|
Correction to my last comment: I said the new deprecation was arguably redundant because v8 also matched the pre-fix markup, and offered to drop it. That was wrong on both counts. Re-tested each deprecation individually against pre-fix markup:
So it stays. Three follow-ups from that:
Also in this push: |
Fixes JETPACK-1878
Proposed changes
The Tiled Gallery block — and Image Compare, which reuses Tiled Gallery's photonization — must serve images from the site's own
files.wordpress.com-style host on VIP sites, instead of routing them through the public Photon domain (i0.wp.com), where VIP images are not accessible and return403. That decision was made byisVIP(), which read the site plan from inline-script globals via a fragilewindowloadlistener that only populated when an Image Compare block happened to be present on the page. As a result, a plain Tiled Gallery on a VIP site never saw the plan and early-photonized its images (broken on VIP).This PR makes the decision reliable and reusable:
skipPhotonDomainflag inJetpackScriptData(Jetpack_Script_Data::set_admin_script_data()), sourced from the same signal as before:'vip' === Jetpack_Plan::get()['product_slug'].jetpack_skip_photon_domainfilter so the decision can be overridden (defaults totrueon VIP only).isVIP()with a single exportedskipPhotonDomain()that reads that flag; removes the inlinejetpack_planlocalize inrender()and thewindow.loadlistener.src/srcSetit produces, and for the save → reload round trip across a setting change.Default behavior is unchanged:
skipPhotonDomainisfalseunless the plan slug isvip.Why the delivery mechanism matters
skipPhotonDomain()feeds the blocks'save()output, so it is also evaluated when the editor regenerates markup to validate saved content while parsing the post. That happens before the editor stores hold any settings, so anything read from a store (an earlier iteration of this PR read a custom key added throughblock_editor_settings_alloffcore/editor) comes back as the default on that first pass and every saved gallery on a VIP site is flagged as invalid — thanks @coder-karen for catching this.The value is therefore delivered as a plain global in the script data, which is printed inline before the editor scripts run and is available synchronously — the same channel as the neighbouring
isSimpleSite()check that already influences thissave()output.Why existing galleries need a deprecation
For the same reason: photonized URLs live in the saved markup, so honouring the setting changes what
save()emits, and galleries saved on a VIP site before this fix holdi0.wp.comURLs. Regenerating them now produces site-host URLs, which is a validation mismatch. The new deprecation reproduces the old output — same markup, external Photon domain — so those galleries parse as valid and are quietly re-serialized the next time they are saved. Attributes are untouched, so there is nothing to migrate.Unlike the previous mechanism, the value is available for every block on every editor load — not only when an Image Compare block is present — so a plain Tiled Gallery on a VIP site is handled correctly. This generalizes the narrower VIP fix from #39941.
Related product discussion/links
Does this pull request change what data or activity we track or use?
No. The same plan signal is used; it is delivered through the script data instead of a per-block inline script.
Testing instructions
Because the fix concerns VIP sites, the simplest way to verify is to simulate a VIP plan on a normal Jetpack-connected site (e.g. Jurassic Ninja):
src/srcSetstay on the site's own host (https://<site>/wp-content/uploads/…) and are not rewritten tohttps://i0.wp.com/….window.JetpackScriptData.jetpack.flags.skipPhotonDomainshould betrue.Existing-content check (the case the deprecation covers):
i0.wp.com, the way a VIP site's galleries were saved before this fix.Baselines to confirm unchanged behavior:
skipPhotonDomainisfalseand images use the standard Photon (i0.wp.com) path — unchanged.isSimpleSite()already forces thefiles.wordpress.compath, so behavior is unchanged there too.add_filter( 'jetpack_skip_photon_domain', '__return_true' );flips the flag totrueon any site.