Builder: Never Save a Layout That Failed to Load - #1374
Open
Misplon wants to merge 16 commits into
Open
Conversation
…r can add them A style that is not an array cannot be sanitized or rendered, and a null one crashes the builder's style checks when the layout loads. The removal now lives in one helper, remove_invalid_styles(), which sanitize_all() runs on the output of the data migration filter; the per-list checks it replaces used isset(), which is false for a key holding null, so the null case they were written for never reached them. The three save paths also run the helper on the output of the siteorigin_panels_data_pre_save filter. sanitize_all() runs before that filter, so a callback on it could store a null style that nothing cleaned; the Toggle Visibility migration in SiteOrigin Premium did exactly that. Refs #1366, #1367.
sanitize_all() removes a null or scalar style on a widget, a row and a cell, including one written by the data migration filter, and skips list items that are not arrays; the removal helper's own contract is covered separately. The real save_post() and save_home_page() are driven with a siteorigin_panels_data_pre_save callback that writes null styles, and the value handed to the meta write carries none of them; the ability write is shown to hand the filter's output to the same cleanup.
loadPanelsData() stores a copy of the layout before it builds the live rows, then swallows any exception thrown while building them. Everything that serialized the builder afterwards read the live rows, so a layout that threw part-way through, such as one with a null row or widget style, was written back truncated on the next save, dropping every widget after the point of failure. The model now records the failure. While it stands, refreshPanelsData() does nothing and getPanelsData() returns the layout that was being loaded instead of the incomplete rows, or null when the layout source could not be parsed. The inline dialog save and the copy-content request skip a failed builder. The flag clears only when a later load completes. Refs #1366.
When the model reports a failed load, the builder view replaces its controls with a notice carrying the error and a reload button, hides and inerts every control present at that moment, and disables the hidden panels_data field. A post save therefore carries no layout at all, and save_post() leaves the stored one untouched instead of writing back whatever rows had loaded. The classic editor no longer auto-switches to a locked builder, so the content editor stays usable; the Page Builder tab still shows the locked builder but never converts editor content into a layout while locked. The Live Editor, the welcome message and the Layout Block's attribute write-back are held off the same way. An unparseable data field is treated as a failed load rather than an empty layout, which used to delete the stored one on the next save. Refs #1366.
A small node:test setup loads the Backbone models the way main.js does, with backbone and underscore from npm and stubs for the registry globals. The first suite proves the fail-closed contract: a load that throws keeps the stored layout in the data attribute, the serializer hands that layout out instead of the partial rows, refreshes fire nothing, an unparseable source hands out nothing, and a later successful load or Revert to Editor clears the state. The tests run on a current Node; the gulp 3 release build under build/ keeps its own Node 10 toolchain, and the release copy excludes the new package files alongside package.json.
… malformed save_post() and save_home_page() indexed whatever json_decode() returned, so malformed or scalar JSON became an empty layout and the stored one was deleted; the home page handler had already written the post by then. Both now decode through one helper that accepts only a real layout object or the explicit empty layout that Revert to Editor submits, and return before any write otherwise. The home page success message is shown only when a layout was actually written. The Layout Builder widget keeps its stored nested layout when the submission carries no panels_data field, which is what a locked nested builder sends, and when the field holds something that is not a layout. Refs #1366.
The decoder's truth table: malformed JSON, scalars, lists and objects without a layout list are refused; false and an empty object become the empty layout that clears; a layout object passes unchanged, escaped quotes and backslashes included. save_post() with a refused payload never reaches the post, and save_home_page() writes nothing and reports no save, including a refused save after a successful one on the same instance. The Layout Builder widget keeps its stored nested layout when the field is absent or refused, still takes a fresh builder id, and still sanitizes an array value.
The builder submits either a layout object or the literal false that Revert to Editor sends. An empty object and an empty list decode to the same empty PHP array, and neither is something the builder produces, so the decoder now refuses both instead of turning them into a clear that deletes the stored layout.
Switching to a locked builder hid the content editor and, with the builder's own controls hidden by the lock, left no way back without a reload. The switch now shows the lock notice beneath the content editor instead, both on load and when the Page Builder tab is used. An admin-bar Live Editor link opens the editor before the layout has loaded, so a failed load closes it rather than trying to prevent it.
The builder always serializes grids and grid_cells, so an object carrying only some of the layout lists is a partial layout rather than an empty one. The decoder now refuses it instead of letting the save path read it as empty and delete the stored layout; a missing widgets list is filled in. The loader applies the same reading: an object with layout keys but no grid cells is a failed load that locks the builder, while false and an empty container remain the builder's own "no layout yet" values.
…lder's own empties Only false and an empty container mean "no layout yet"; a scalar, a list or an object without layout lists is not a layout, and building an empty builder from it would let the next save replace the stored layout with an empty one. Such values now take the failure path and lock the builder. The JS test script names its files explicitly so it runs on every Node in the declared range, and the decoder's docblock states the shape it accepts.
The loader now checks the same top-level shape the server accepts before building rows, so a stored value with a non-list member locks the builder instead of presenting an empty one that an edit could save over the stored layout. The nested widget test now proves sanitize_all() runs on an array layout and does not run on the preserving paths.
The save paths now call remove_invalid_styles() after the pre-save filter; the stand-in for the styles admin in this suite passes the layout through so the suite keeps observing what the save path itself does.
json_decode() turns a numeric-keyed JSON object into a PHP array as well, but the builder indexes rows, cells and widgets by position, so such an object is not a usable list even when its values are well formed. The decoder now refuses a layout whose grids, grid_cells or widgets is not a list with keys running from zero.
The builder addresses rows, cells and widgets by position, so a layout whose cell or widget references do not resolve cannot be loaded; storing it would only lock the builder on the next visit. The decoder now checks that every cell names an existing row and every widget an existing row and cell, reading the placement from panels_info or the older info key.
The loader reads each row's settings by position, so a row list holding a null or a scalar cannot be loaded; the decoder now refuses it with the other unresolvable shapes.
Misplon
changed the base branch from
develop
to
fix/1366-resolve-prevent-crash-null
September 5, 2026 17:44
Misplon
force-pushed
the
fix/1366-fail-closed-builder-save
branch
from
September 5, 2026 17:45
3e55270 to
0c0fef4
Compare
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.
Stacked on #1367. Refs #1366.
Reading order. The first two commits answer the review on #1367:
sanitize_all()now removes a null or scalar style through one helper that usesarray_key_exists()(theisset()blocks could never see a stored null), and the three save paths run that helper on the output ofsiteorigin_panels_data_pre_save, sincesanitize_all()runs before that filter and the Toggle Visibility migration writes its null there. Then the ten fail-closed commits from this PR's first version, unchanged apart from one context line. Then four small follow-ups: a test-stub compatibility line, and three decoder tightenings so a submitted layout the builder could not load (numeric-keyed objects for lists, cells or widgets pointing past the rows, a row that is not an array) is refused before it replaces stored data.What the fail-closed commits do. When a stored layout throws while the builder loads it, the builder no longer serializes the rows that did load. It records the failure, hands out the stored layout instead of the partial one, locks its controls behind a notice with the error and a Reload button, and disables the hidden
panels_datafield, so Update sends no layout andsave_post()leaves the stored one untouched while title and content save normally. The Live Editor, the in-dialog Save, the nested Layout Builder widget and the Layout Block are closed off the same way.save_post(),save_home_page()and the Layout Builder widget refuse a submission that is not a layout instead of decoding it to an empty one and deleting what is stored. Nothing changes for a layout that loads cleanly.Why. Reproduced on develop through the classic editor: a page with 3 widgets and 2 rows, one row style stored as null, saved as 0 widgets and 1 row after a single Update. With this stack the same Update leaves the stored layout byte-identical, and the null can no longer be written in the first place.
Merge path. Merge #1367 first, as-is; GitHub retargets this PR at develop; merge this next. The first two commits here are what make #1367's cleanup effective, so the two should ship in the same release. @AlexGStapleton if you'd rather carry those two commits inside #1367, cherry-pick
bbd27ce3and14960a26; nothing on your branch was touched.Tests. Node harness for the builder model (
nvm use 20 && npm ci && npm test, 14 tests), PHP: 90 + 6 + 28 + 58 + 3 across the five suites, including realsave_post()andsave_home_page()driven with a null-writing pre-save callback. Verified in the browser on the classic editor, the nested widget, the Layout Block, an unparseable field, the admin-bar Live Editor link, Revert to Editor and the inline Save, and on the live site through the realsave_post()with a Premium-shaped null-writing filter.