Automattic / Automattic/newspack-content-diff-migrator
Featured image and block-content remap silently skipped on ID collision (regression from a7a16e7)
- Dominant language
- PHP
- Stars
- 0
- Forks
- 1
- Avg merge
- 5h 6m
- Merged PRs (30d)
- 1
Description
## Summary
The ID-collision overlap protection added in [`a7a16e7`](https://github.com/Automattic/newspack-content-diff-migrator/commit/a7a16e7) introduces a false-positive skip in `update_featured_image` and in five places in `BlockUpdater`. On sites where a post's stale live attachment ID numerically coincides with the local ID assigned to a different imported attachment, the affected remap is silently skipped and the post ends up displaying the wrong attachment.
Discovered in production during a content refresh: 153 posts displayed unrelated images as their featured image. Investigation traced the regression to commit `a7a16e7`.
## Severity / suggested labels
- **bug** + **regression** — affects any site that has accumulated NCDM history dense enough for cross-source ID overlap (single-source flows can also hit it once a site's local ID space crosses a previously-used source range).
- Visible-but-silent: no log entry, no warning — the affected posts simply render the wrong image.
## Reproducer
Given two imported attachments where one's *new local ID* numerically matches another's *old live ID*:
| Imported attachment | Live ID | Local ID |
|---|---|---|
| Att A | `1000` | `2000` |
| Att B | `2000` | `3000` |
…and a post whose `_thumbnail_id` still references live attachment B verbatim (`2000`):
```php
// $imported_attachment_ids_map contains:
// 1000 => 2000 (Att A)
// 2000 => 3000 (Att B)
// Post currently has _thumbnail_id = 2000 (the LIVE id, never remapped).
\$logic->update_featured_image( \$post_id, \$imported_attachment_ids_map );
// Bug: the broad in_array( 2000, array_values($map), true ) === TRUE
// because 2000 appears as a *value* (it's Att A's local id).
// Function returns early. _thumbnail_id stays at 2000.
// On staging, post 2000 happens to be an unrelated attachment.
```
Same pattern fires inside `BlockUpdater::resolve_new_attachment_id` and four other locations whenever an attachment ID embedded in `post_content` collides with another imported attachment's new local ID.
## Why existing tests didn't catch it
The test added in the same commit (`test_update_featured_image_should_skip_when_thumbnail_is_already_local_id`) only exercises the *intended* scenario — the post's `_thumbnail_id` is set to the **already-remapped** local target — and asserts that on a subsequent run the value is not re-mapped via the chain. It doesn't cover the case where the post's `_thumbnail_id` is still a stale live ID that happens to numerically match a local target assigned to a different imported attachment. That's the production scenario.
Same gap in the three `BlockUpdaterTest` cases added by the commit.
## Proposed fix
Branch: [`fix/attachment-id-collision-false-skip`](https://github.com/Automattic/newspack-content-diff-migrator/tree/fix/attachment-id-collision-false-skip)
Draft PR will be linked once opened.
**Featured-image path (`ContentDiffLogic::update_featured_image`)**:
Replace the broad `in_array( values )` check with a precise double-remap guard. The function now accepts an optional third argument — the post's source-side `_thumbnail_id` (i.e. the live attachment ID the source post referenced). When supplied, the skip fires only when the current local `_thumbnail_id` already equals the local target that `live_thumbnail_id` maps to. The Command-layer caller `update_featured_image_ids` batch-fetches these values from `{live_table_prefix}postmeta` in one query and passes them per-post.
**Block-content path (`BlockUpdater`)**:
Remove the five broad value-membership checks. Without parsing the source post's content there is no reliable way to distinguish "already-remapped local ID" from "stale live ID coinciding with another local target." The outer `update_attachment_ids_in_blocks` loop already filters by run-state, preventing double-processing of already-handled posts on subsequent runs — the same protection the original commit intended to add at a different layer.
**Tests**:
- New regression test covering the FI false-skip both with and without the optional source-side argument.
- Three existing `BlockUpdaterTest` cases that asserted the old (buggy) behavior were updated to assert the corrected behavior, with docblocks explaining what changed and why.
- Full unit + integration suite: 676 tests, all green.
## Workaround for affected sites
For sites that have already run the buggy version of the diff, a SQL one-shot can remap any remaining stale `_thumbnail_id` values:
```sql
UPDATE wp_postmeta pm
JOIN wp_postmeta map
ON map.meta_key = 'newspackcontentdiff_oldid_'
AND CAST(map.meta_value AS UNSIGNED) = CAST(pm.meta_value AS UNSIGNED)
JOIN wp_posts target_attach
ON target_attach.ID = map.post_id
AND target_attach.post_type = 'attachment'
SET pm.meta_value = map.post_id
WHERE pm.meta_key = '_thumbnail_id'
AND CAST(pm.meta_value AS UNSIGNED) > 0
AND map.post_id <> CAST(pm.meta_value AS UNSIGNED);
```
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with ContentDiffLogic::update_featured_image and BlockUpdater::resolve_new_attachment_id, then inspect the update_featured_image_ids caller and the existing BlockUpdaterTest cases. Run the focused tests first and verify the stale-live-ID collision is remapped while already-remapped IDs are not processed twice; finish by running the full unit and integration suite.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- php, wordpress
- Domain
- backend, databases
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 45/100