Sync master from upstream - #122
Merged
Merged
Conversation
Two WP 7.1 regressions, both from platform behaviour changes rather than our own
recent edits. Measured on a 7.1 install (7.0.4 locally shows neither).
1. Form block preview never loads (stuck spinner)
On 7.1 the editor page is cross-origin isolated (window.crossOriginIsolated
=== true) and the canvas is a blob: document, so the preview iframe is opaque
to us: contentDocument is null and contentWindow access throws SecurityError.
modifyIframeContent() returned at its `if ( ! iframeDocument )` guard, which
sat *before* setLoading( false ) — so the loader never cleared and the form
appeared to never load.
- Clear the loader unconditionally; height measurement is now best-effort.
- Add a 3s fallback: loading="eager" means the load event can fire before the
effect attaches onload, and a policy-blocked frame never fires it at all.
- Take height over a new srfm-preview-height postMessage from the preview page
(the only channel that survives a cross-origin preview), with the sender
verified by contentWindow reference rather than origin, since the embedder
origin can be opaque.
- Guard the IntersectionObserver path, which did
`iframeRef.current.contentDocument.querySelector()` and threw a TypeError.
- Add credentialless so an isolated document may embed the preview at all.
Only published forms are ever framed, so no cookies are needed.
- Replace canUserEditEntityRecord() (deprecated 6.7, and called with the wrong
arity — the single argument landed in `kind`) with canUser( 'update', … ).
2. Settings modal overlapped the admin bar
Up to 7.0 fullscreen mode hid the admin bar; on 7.1 the bar stays visible
(measured: is-fullscreen-mode set, #wpadminbar 32px). The dialog removed its
top offset whenever fullscreen was active, sliding its header and close button
under the bar. Offset is now the measured bar height instead of inferred from
fullscreen, and the panel's height compensates so h-full no longer pushes the
same pixels off the bottom. Pre-7.1 the bar measures 0, so the old layout is
preserved exactly.
Verified: ESLint clean, build:script passes; on 7.0.4 the preview still loads
(iframe present, spinner cleared, height 390, form rendered) and the modal is
unchanged in fullscreen, correct in both branches including a simulated 7.1
(bar visible while fullscreen -> 32px offset, no overlap, no bottom overflow).
The credentialless part needs confirming on a real 7.1 install: Chrome reports
COEP frame blocks to the Issues panel rather than the console, so the block
itself could not be captured directly.
Follow-up to the first commit on this branch, after testing on a live WP 7.1-RC4 install rather than only on 7.0.4: - Height feedback loop: the first attempt observed the preview container with a ResizeObserver, but the editor resizes the frame in response, which resizes the container, which fires the observer again. That pegged the editor's main thread. Now reports on load/resize only, with a 4px tolerance on the sender and an appliedHeightRef no-op on the receiver so a repeated height cannot re-render. - Dropped credentialless: added on the theory that COEP was blocking the frame, but the preview loads without it on 7.1, so it is not shipped on a hunch. - Reverted the canUser( 'update', … ) migration. Still worth doing — canUserEditEntityRecord() is deprecated since 6.7 and is called with the wrong arity — but it touches a resolver-backed selector I could not validate safely on 7.1, and it is cosmetic to these bugs. Left for its own PR. - header-styles.scss: margin: 0 on .editor-header__back-button, which 7.1 gives its own margin, breaking the logo tile's flush fit. Confirmed on 7.1: the stuck spinner is gone and the form renders. Full-height behaviour still needs a human visual check (see the PR description).
…view-modal fix: WP 7.1 editor regressions — form block preview and settings modal
…into master-dev-2.12.4-post
Master to Dev 2.12.4
Dev to Next Release 2.12.4
wp.apiFetch only exists once the separately-loaded wp-api-fetch script has executed. JS-combining/deferring optimizer plugins can drop or reorder that dependency while still shipping form-submit.js, leaving window.wp.apiFetch undefined and silently breaking every submission on the page. Neither REST call this file makes needs anything wp.apiFetch provides beyond URL resolution: submit-form authenticates via the X-WP-Submit-Token header (Submit_Token::verify()), not a nonce, and after-submission takes its nonce as an explicit query arg. So PHP now localizes the two fully-resolved REST URLs via rest_url() (same pattern already used for phone.js's geo_endpoint), and the frontend calls them with a plain fetch() — a native browser API with no script dependency of its own to drop. The wp-api-fetch script dependency is removed from form-submit's registration since nothing in it is used anymore.
The bundle imports __() from @wordpress/i18n and applyFilters() from
@wordpress/hooks, and @wordpress/scripts externalises both to the wp.i18n /
window.wp.hooks globals rather than inlining them — assets/build/
formSubmit.asset.php declares array('wp-hooks', 'wp-i18n') accordingly.
They were previously satisfied by accident: 'wp-api-fetch' pulled both in
through its own dependency graph. Removing that dependency removed them too,
leaving the handle with no declared dependencies at all. Under a JS-combining
or deferring optimizer — the exact condition this PR exists to fix — an
undeclared wp.hooks is undefined and every submission dies on the same class
of TypeError, so the change would have reproduced the original bug by a
different route.
Comment records why, so the array is not trimmed back later.
…ead-of-apifetch fix: replace wp.apiFetch with fetch() for form submission
…to dev-nr-2.12.5
Dev to NR 2.12.5
Version Bump 2.12.5
Auto-generated by /i18n command on PR #3074
chore: update i18n translations
…rding Docs: reword the 2.12.5 changelog entry for the submission fix
afterSubmit() concatenated the submission id and nonce onto the localized REST
base. That is only correct when the site uses pretty permalinks. With plain
permalinks rest_url() returns
https://site/index.php?rest_route=/sureforms/v1/after-submission
so the route is a query parameter, not a path — and only the first "?" in a URL
delimits the query string. The concatenation produced
...?rest_route=/sureforms/v1/after-submission/123?after_submit_nonce=abc
which the server parses as a single parameter:
[rest_route] => /sureforms/v1/after-submission/123?after_submit_nonce=abc
The route therefore did not match at all (rest_no_route, 404), and even had it
matched, the nonce never arrived as its own parameter.
The consequence was larger than a failed request. handle_after_submission()
hard-verifies after_submit_nonce (background-process.php:108), so it bailed and
do_action( 'srfm_after_submission_process' ) never fired — the hook every native
integration runs on. Entries still saved, so this presented as integrations
silently stopping rather than as a submission error, and the route's
permission_callback is __return_true so nothing else surfaced it.
buildAfterSubmitUrl() now uses URL/URLSearchParams and handles both shapes: the
id extends the rest_route value when the route is in the query, and the path
otherwise. URLSearchParams also encodes the nonce once, via the API, instead of
by hand.
Verified against a live install:
OLD /index.php?rest_route=/sureforms/v1/after-submission/1?after_submit_nonce=dummy
-> {"code":"rest_no_route", ... "status":404}
NEW /index.php?rest_route=/sureforms/v1/after-submission/1&after_submit_nonce=dummy
-> {"code":"rest_nonce_failed", ... "status":403}
404 to 403 is the fix: the route now matches and reaches the handler, which
rejects only because that nonce was a dummy. Pretty-permalink and subdirectory
installs were also checked and are unchanged in output.
… submitters
Public form endpoints authenticate with the HMAC Submit_Token rather than a
nonce, because the form markup is page-cacheable and core answers a nonce that
fails verification with a hard 403 — a value baked into a cached page would
break submissions once it aged out.
The trade-off is that rest_cookie_check_errors() reads a cookie-carrying REST
request with no nonce as anonymous and calls wp_set_current_user( 0 ) before
dispatch. get_current_user_id() therefore returns 0 mid-submission even when the
visitor is signed in, which silently:
- dropped user_id from the entry (form-submit.php), so entries submitted by
logged-in users were recorded as anonymous;
- blanked every {user_*} smart tag (smart-tags.php), since parse_user_props()
read wp_get_current_user() and got the ID-0 placeholder. These tags are
resolved during submission for notification to/subject/body/reply-to/cc, so a
notification using {user_email} as Reply-To rendered empty.
Helper::get_submitting_user_id() falls back to wp_validate_auth_cookie(), which
reads the logged-in cookie directly and is unaffected by the reset. The cookie's
HMAC is verified, so the identity is authenticated rather than asserted — the
same check core uses for cookie auth, and the pattern Pro's login route already
uses for this exact reason (business/user-registration/init.php:515).
Verified against a live install, reproducing the real condition (current user
reset to 0, valid logged_in cookie present):
get_current_user_id() : 0 <- what the old code saw
get_submitting_user_id() : 1 <- what the fix sees
tampered cookie : 0
no cookie : 0
{user_id} | {user_email} | {user_login}
anonymous : [ | | ]
signed in : [1 | dev-email@wpengine.local | admin]
Revert-tested: removing the cookie fallback fails on "A valid logged_in cookie
must recover the user."
check-test-coverage flagged this rather than the new method, because the script attributes changed lines to the preceding function and get_submitting_user_id() was inserted directly after it. It is a genuine gap rather than a quirk to work around: this gate guards several admin REST routes and had no test at all. Covers the anonymous refusal (WP_Error with an authorization status), a subscriber refusal, and the administrator pass, so the failure mode that matters — returning true for someone who should not pass — is pinned.
Replaces the JS URL builder from the previous commit. Same bug, smaller fix, and
no URL logic in the client at all.
The server already knows the submission id and the nonce at the moment it builds
the submit response, and WordPress already has the two functions that get this
right: rest_url() knows whether the route is a path or a `?rest_route=` query
arg, and add_query_arg() knows whether the nonce needs `?` or `&`. So the
response now carries a finished `after_submit_url` and afterSubmit() just fetches
it.
That deletes the 40-line buildAfterSubmitUrl() helper and both of its regexes,
which reimplemented core's knowledge of rest_route in JavaScript — the kind of
thing that rots the next time core changes.
Also removes the localized `after_submit_url`, now dead. It was a base URL that
invited exactly the concatenation that caused the bug; the only consumer was the
code being replaced here, and nothing in pro referenced it.
`after_submit_nonce` stays in the response despite no longer being read by our
JS: it is an exposed response field that integrations may consume, and keeping
it costs one line.
Verified in both permalink modes:
PRETTY : /wp-json/sureforms/v1/after-submission/123?after_submit_nonce=…
PLAIN : /index.php?rest_route=%2F…%2Fafter-submission%2F123&after_submit_nonce=…
plain parses as [rest_route] => /sureforms/v1/after-submission/123
[after_submit_nonce] => 3715c81518
Both shapes reach the handler on a live install (403 rest_nonce_failed on a
deliberate dummy nonce), where the old concatenated URL returned a 404
rest_no_route.
…n-attribution Fix: entries and user smart tags lose the signed-in user on submission
…ain-permalinks Fix: after-submission process never runs on plain-permalink sites (regression from #3064)
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.
Sync of
brainstormforce/sureforms@masterinto the public mirror.Range
db0c3aa3..24ccb4ffThe branch is capped with a merge commit whose first parent is
master, so this diff is computed against the mirror and contains only real upstream changes — no internal-file deletions. The 57-file diff is byte-identical in file set togit diff c043bea36..24ccb4ff4upstream, confirming the strip introduced no extra deletions.Highlights (new since the last sync)
This window is the 2.12.5 release:
wp.apiFetchwithfetch()for form submissionStrip
Removed before signing: all
CLAUDE.mdat any depth (root,inc/abilities/,tests/play/specs/),.claude/,docs/,internal-docs/,.scripts/git-hooks/, the five internal release workflows, andbin/{build-zip.sh,checkout-and-build,i18n.sh}.Post-strip check confirms the only markdown published is
README.md,inc/abilities/ABILITIES.md,modules/gutenberg/readme.md,tests/play/README.md, and third-partyinc/lib/**.Signatures
All 741 commits in the re-signed range carry signatures (verified by grepping
gpgsigon every commit object — zero unsigned), with a single committer identity across the range. GitHub reportsverified=true, reason=validon the branch tip and the strip commit.