Skip to content

fix: coerce object-shaped page profiles before viewport lookup - #1133

Open
selul wants to merge 3 commits into
developmentfrom
fix/1122-object-shaped-profile-data
Open

fix: coerce object-shaped page profiles before viewport lookup#1133
selul wants to merge 3 commits into
developmentfrom
fix/1122-object-shaped-profile-data

Conversation

@selul

@selul selul commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Viewport lazyload fatals when stored page-profile data is stdClass instead of an array (Cannot use object of type stdClass as array at Profile.php:369). That is the usual shape when an object-cache backend JSON-decodes without associative arrays.
  • Normalize storage get() through Storage\Base::normalize_value(), coerce again when loading current profile data, and guard af/lcp/global lookups plus personalized background CSS so a bad shape never 500s the frontend.
  • Valid object-shaped payloads are converted and still used (above-fold detection keeps working); corrupt scalars are treated as a cache miss.

Fixes #1122

Test plan

  • composer phpunit -- --filter='Test_Page_Profiler_Shape|Test_Lazyload_Viewport' (42 tests)
  • PHPCS and PHPStan on the changed v2 files
  • Confirm a Redis/Memcached drop-in that JSON-decodes objects no longer fatals frontend viewport lazyload
  • With viewport lazyload on, a page with stored profile data still skips above-fold images after an object-cache roundtrip
  • Missing/corrupt cache values still fall back to lazyloading all images

Made with Cursor

Object-cache backends that JSON-decode without associative arrays store profiler payloads as stdClass, which fatals on ['af'] access during frontend lazyload. Normalize storage reads and guard device lookups.

Co-authored-by: Cursor <cursoragent@cursor.com>
@pirate-bot

pirate-bot commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Plugin build for 801b351 is ready 🛎️!

Co-authored-by: Cursor <cursoragent@cursor.com>
@selul

selul commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Reviewed the fix against #1122, with the full suite run locally. The diagnosis and the fix are correct — but about 75% of the diff is provably redundant, and the PR can be trimmed to the storage boundary alone.

What checks out

  • The diagnosis matches the telemetry in Viewport lazy-load profiling crashes on object-shaped profile data #1122: the fatal is the ['af'][$image_id] lookup in is_in_all_viewports(), reached when a JSON-decoding object-cache backend returns stdClass.
  • normalize_value() is correctly written: recursive get_object_vars(), scalars treated as a cache miss, numeric string keys canonicalize to int on array insertion, so af lookups by int image ID keep working.
  • Normalizing both backends is right — transients also ride the object cache when a drop-in is present.
  • Full suite passes on the branch (275 tests locally), and the change quietly fixes a pre-existing unguarded ['lcp']['imageId'] access.

The fix can be ~30 lines instead

Profile::$current_profile_data has exactly two writers: set_current_profile_data() (fed by storage->get()) and reset_current_profile() ([]). exists(), get_profile_data(), and get_personalized_css() (via get_current_profile_data()) all trace back to the same two storage get() methods. So the complete production fix is layer 1 only:

  • Base::normalize_value()
  • the one-line wrap in ObjectCache::get()
  • the one-line wrap in Transients::get()
  • plus the ?? null guard in is_lcp_image_in_all_viewports() (genuine pre-existing bug, worth keeping)

Verified empirically: with only the Storage/* changes kept and Profile.php + BgOptimizer/Lazyload.php reverted to the development versions, every new test that drives data through a real entry point still passes — object cache, transients, set_current_profile_data(), get_profile_data(), exists(), and the end-to-end replace_content() with object-shaped transients. The only 8 failures are the tests that inject stdClass into the private static via reflection — a state production cannot reach once the boundary normalizes. No other suite regressions.

The extra layers also aren't free: get_device_member()get_device_data() runs per image per device inside can_lazyload_for() — the hottest loop of viewport lazyload — replacing a direct ?? false chain with two function calls plus is_array checks per lookup. The old lookup code is already safe against false/missing values (empty() guard + ?? isset semantics emit no warnings on false['af']).

Suggested trim: keep Storage/* + the lcp guard, drop the other Profile.php/BgOptimizer changes, and rewrite the 8 reflection-injection tests to seed through storage instead — they currently exercise states the fix itself makes impossible.

Process flags

  1. Base branch is master while the repo flow targets development (dependabot chore(deps): bump codeinwp/themeisle-sdk from 3.3.58 to 3.3.61 #1128, the sync commits). Fine if this is an intentional hotfix release; otherwise retarget.
  2. The composer.lock hunk (themeisle-sdk 3.3.58 → 3.3.61) is unrelated to the fix and duplicates 405dee1 already on development. If retargeted it conflicts; if it stays a master hotfix, bundling a dependency bump into a crash fix adds release risk. Suggest dropping it either way.

🤖 Generated with Claude Code

Drop per-lookup Profile/Lazyload guards now that storage get() normalizes stdClass. Keep the LCP imageId null coalesce. Seed tests through transients instead of reflection.

Co-authored-by: Cursor <cursoragent@cursor.com>
@selul
selul changed the base branch from master to development September 1, 2026 13:00
@selul

selul commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the review:

  • Production fix is now the storage boundary (normalize_value() on ObjectCache::get() / Transients::get()) plus the ?? null LCP imageId guard.
  • Reverted the extra Profile / BgOptimizer lookup layers.
  • Reflection-injection tests now seed object-shaped payloads through transients and load them via set_current_profile_data().
  • Retargeted the PR from master to development, so the unrelated themeisle-sdk bump is no longer in the diff.

Test_Page_Profiler_Shape + Test_Lazyload_Viewport pass (43 tests).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Viewport lazy-load profiling crashes on object-shaped profile data

2 participants