Skip to content

Fix: unserialize error#153

Open
ahuininga-orisha wants to merge 2 commits into
developfrom
fix/unserialize-error
Open

Fix: unserialize error#153
ahuininga-orisha wants to merge 2 commits into
developfrom
fix/unserialize-error

Conversation

@ahuininga-orisha

Copy link
Copy Markdown
Collaborator

Summary

  • Fixes constant DB queries on every request caused by UrlFinder::loadPageLookupArray() never successfully populating its cache entry. The root cause was that LandingPage::getUnserializedFilterAttributes() called unserialize() without error handling. On PHP 8.x, malformed or truncated filter_attributes rows trigger a Warning that Magento's ErrorHandler converts into a thrown Exception. This exception escaped the foreach loop in loadPageLookupArray(), preventing $this->cache->save() from ever being reached. On every new request (new worker/process) the in-memory guard was null again, so the heavy emico_attributelanding_page JOIN query ran unconditionally.
  • Fixes unserialize(): Error at offset warnings (issue [Performance/bug] UrlFinder::loadPageLookupArray causes constant DB lookups and unserialize warnings (Emico_AttributeLanding) #135) by wrapping unserialize() in a try/catch (\Throwable) and validating the result is an array. Corrupt or legacy rows now return [] instead of throwing. ['allowed_classes' => false] is also added as a security hardening measure.
  • Adds a per-iteration guard in UrlFinder::loadPageLookupArray() so that if any single landing page's getFilters() call throws for any future reason, that page is skipped and the rest of the lookup array is still built and cached normally.

How to test

Scenario 1 — Normal operation: cache is populated and reused

  1. Ensure at least one active landing page exists with valid filter_attributes.
  2. Enable query logging (e.g. MySQL general log or a profiler toolbar).
  3. Load a category page that has layered navigation.
  4. Verify the emico_attributelanding_page JOIN query runs exactly once.
  5. Reload the page; verify the query does not run again (cache hit).
  6. Save the landing page in the admin; reload the category page and verify the query runs once more (cache invalidated by InvalidateCacheObserver), then is cached again on the next load.

Scenario 2 — Corrupt filter_attributes row: graceful degradation, cache still populated

  1. Directly update a landing page row in the database to set filter_attributes to a malformed string, e.g.:
    UPDATE emico_attributelanding_page SET filter_attributes = 'CORRUPTED' WHERE page_id = <id>;
  2. Flush the Magento cache (bin/magento cache:clean).
  3. Load a category page with layered navigation.
  4. Verify no unserialize warning or exception appears in var/log/exception.log.
  5. Verify the corrupted landing page is silently skipped; all other valid landing pages are still present in the lookup and their filter links render correctly.
  6. Verify the JOIN query runs only once per cache lifetime (cache was successfully populated despite the corrupt row).

Scenario 3 — Admin form renders correctly for corrupt row

  1. With the corrupt row from Scenario 2 still in place, open the landing page edit form in the admin.
  2. Verify the page loads without a 500 error.
  3. Verify the filter_attributes dynamic rows field is empty (reflecting what is actually stored).
  4. Save the form with valid filter attributes; verify the row is repaired and the frontend lookup works correctly afterwards.

@ahuininga-orisha ahuininga-orisha changed the base branch from master to develop May 13, 2026 10:31
Comment thread src/Model/LandingPage.php

return unserialize($this->getFilterAttributes());
try {
$result = unserialize($raw, ['allowed_classes' => false]);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely a class could never be stored here? This is data retrieved from the admin page's POST request.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the data originates from admin POST requests (so objects aren't expected), it's a security best practice — if the database row were ever tampered with, allowed_classes => false prevents PHP object injection attacks by making unserialize() throw instead of instantiating arbitrary classes.

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.

2 participants